[NTP Snippets] Don't notify about new suggestion when in a not-available state

[NTP Snippets] Don't notify about new suggestion when in a not-available state

BUG=662179
Committed: https://crrev.com/fb76bb12a9fa5ee0024dc92d32de3c2a0e8c8e46
Cr-Commit-Position: refs/heads/master@{#432165}

[Marc Treib, 2016-11-14 11:17:48]

Description was changed from

==========
[NTP Snippets] Don't notify about new suggestion when in a not-available status

BUG=662179
==========

to

==========
[NTP Snippets] Don't notify about new suggestion when in a not-available state

BUG=662179
==========

[Marc Treib, 2016-11-14 11:18:16]

treib@chromium.org changed reviewers:
+ dgn@chromium.org

[Marc Treib, 2016-11-14 11:18:17]

PTAL!

[chromium-reviews, 2016-11-14 11:47:58]

Drive-by: any chance of a can cover this with a test? IMO all fixed should
come with a test verifying it does what we expect and more importantly,
don't regress in the future.

--Tim

On Monday, November 14, 2016, <treib@chromium.org> wrote:

> Reviewers: dgn
> CL: https://codereview.chromium.org/2496163002/
>
> Message:
> PTAL!
>
> Description:
> [NTP Snippets] Don't notify about new suggestion when in a not-available
> state
>
> BUG=662179
>
> Affected files (+5, -1 lines):
> M components/ntp_snippets/remote/remote_suggestions_provider.cc
>
>
> Index: components/ntp_snippets/remote/remote_suggestions_provider.cc
> diff --git a/components/ntp_snippets/remote/remote_suggestions_provider.cc
> b/components/ntp_snippets/remote/remote_suggestions_provider.cc
> index 5f3157ee076d523a41e6bf5ad0bab131ae33c98f..
> 9afb12005a5bfdb94d8cd794240b63d7b008b94c 100644
> --- a/components/ntp_snippets/remote/remote_suggestions_provider.cc
> +++ b/components/ntp_snippets/remote/remote_suggestions_provider.cc
> @@ -1037,7 +1037,11 @@ void RemoteSuggestionsProvider::FinishInitialization()
> {
> // don't know how long the fetch will take or if it will even complete.
> for (const auto& item : category_contents_) {
> Category category = item.first;
> - NotifyNewSuggestions(category, item.second);
> + const CategoryContent& content = item.second;
> + // Note: We might be in a non-available status here, e.g. DISABLED due to
> + // enterprise policy.
> + if (IsCategoryStatusAvailable(content.status))
> + NotifyNewSuggestions(category, item.second);
> }
> }
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "New Tab Page development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ntp-dev+unsubscribe@chromium.org
> <javascript:_e(%7B%7D,'cvml','ntp-dev%2Bunsubscribe@chromium.org');>.
> To post to this group, send email to ntp-dev@chromium.org
> <javascript:_e(%7B%7D,'cvml','ntp-dev@chromium.org');>.
> To view this discussion on the web visit https://groups.google.com/a/
> chromium.org/d/msgid/ntp-dev/94eb2c11bc021c08ff054140ffb6%40google.com
>
<https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/94eb2c11bc021c08ff05...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Marc Treib, 2016-11-14 13:17:02]

On 2016/11/14 11:47:58, chromium-reviews wrote:
> Drive-by: any chance of a can cover this with a test? IMO all fixed should
> come with a test verifying it does what we expect and more importantly,
> don't regress in the future.
> 
> --Tim

I wouldn't quite agree to the "all" - IMO there are situations where a
regression test doesn't really add any value.

That said, this isn't one of them :)  In fact, the test I added uncovered
another problem. So thanks for pushing me to be less lazy! ;)

[dgn, 2016-11-14 14:03:10]

Maybe also update SuggestionsSection#addSuggestions to assert for the status to
be available, since the alternative should not be a supported code path?

[Marc Treib, 2016-11-14 14:09:30]

On 2016/11/14 14:03:10, dgn wrote:
> Maybe also update SuggestionsSection#addSuggestions to assert for the status
to
> be available, since the alternative should not be a supported code path?

Hm. Is that quite the same thing, given that the UI isn't always fully in sync
with the backend?

[dgn, 2016-11-14 14:57:22]

On 2016/11/14 14:09:30, Marc Treib wrote:
> On 2016/11/14 14:03:10, dgn wrote:
> > Maybe also update SuggestionsSection#addSuggestions to assert for the status
> to
> > be available, since the alternative should not be a supported code path?
> 
> Hm. Is that quite the same thing, given that the UI isn't always fully in sync
> with the backend?

The UI is generally in sync. It just avoids clearing snippets outside of status
changes.
So if the user signs out for example, we would clear them. So status is one of
the
things the UI always reacts to in some way.

Except... We don't do anything for NOT_PROVIDED, and just stay
in the same state. Considering we clear the section for
CATEGORY_EXPLICITLY_DISABLED or
LOADING_ERROR, I'm not sure why we ignore NOT_PROVIDED. Any idea? Something with
how
we used to handle Sign out maybe?

[Marc Treib, 2016-11-14 15:04:56]

On 2016/11/14 14:57:22, dgn wrote:
> On 2016/11/14 14:09:30, Marc Treib wrote:
> > On 2016/11/14 14:03:10, dgn wrote:
> > > Maybe also update SuggestionsSection#addSuggestions to assert for the
status
> > to
> > > be available, since the alternative should not be a supported code path?
> > 
> > Hm. Is that quite the same thing, given that the UI isn't always fully in
sync
> > with the backend?
> 
> The UI is generally in sync. It just avoids clearing snippets outside of
status
> changes.
> So if the user signs out for example, we would clear them. So status is one of
> the
> things the UI always reacts to in some way.
> 
> Except... We don't do anything for NOT_PROVIDED, and just stay
> in the same state. Considering we clear the section for
> CATEGORY_EXPLICITLY_DISABLED or
> LOADING_ERROR, I'm not sure why we ignore NOT_PROVIDED. Any idea? Something
with
> how
> we used to handle Sign out maybe?

I think NOT_PROVIDED was supposed to mean "the category went away in the
backend, but there's no need to immediately purge it", so we prefer not updating
the UI.

[Marc Treib, 2016-11-15 09:15:08]

Ping! Is this okay to land? I'd like to get it into M56.

[dgn, 2016-11-15 10:37:26]

sorry for the delay, lgtm

[Marc Treib, 2016-11-15 10:50:43]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-11-15 10:50:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 11:35:24]

Description was changed from

==========
[NTP Snippets] Don't notify about new suggestion when in a not-available state

BUG=662179
==========

to

==========
[NTP Snippets] Don't notify about new suggestion when in a not-available state

BUG=662179
==========

[commit-bot: I haz the power, 2016-11-15 11:35:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-15 11:37:13]

Description was changed from

==========
[NTP Snippets] Don't notify about new suggestion when in a not-available state

BUG=662179
==========

to

==========
[NTP Snippets] Don't notify about new suggestion when in a not-available state

BUG=662179

Committed: https://crrev.com/fb76bb12a9fa5ee0024dc92d32de3c2a0e8c8e46
Cr-Commit-Position: refs/heads/master@{#432165}
==========

[commit-bot: I haz the power, 2016-11-15 11:37:14]

Patchset 2 (id:??) landed as
https://crrev.com/fb76bb12a9fa5ee0024dc92d32de3c2a0e8c8e46
Cr-Commit-Position: refs/heads/master@{#432165}

[tschumann, 2016-11-15 12:55:53]

On 2016/11/14 13:17:02, Marc Treib wrote:
> On 2016/11/14 11:47:58, chromium-reviews wrote:
> > Drive-by: any chance of a can cover this with a test? IMO all fixed should
> > come with a test verifying it does what we expect and more importantly,
> > don't regress in the future.
> > 
> > --Tim
> 
> I wouldn't quite agree to the "all" - IMO there are situations where a
> regression test doesn't really add any value.
please let me know when you come across one of those. I'm pretty sure the tests
would add value, it might just be super-hard to test (which can be a separate
issue).
> 
> That said, this isn't one of them :)  In fact, the test I added uncovered
> another problem. So thanks for pushing me to be less lazy! ;)
:-)