Enable news v2 by default for Desktop #15779
Conversation
We could but the issue there is that it only applies to restarts after getting finch updates. So, new users would get the old Brave News feature. |
petemill
force-pushed
the
news-desktop-v2-enable
branch
from
5ccab3e to
484b162
Compare
| << " is in channel " << locale_info->locale << "." | ||
| << channel_id << " which is subscribed to."; | ||
| return true; | ||
| if (channels.contains(channel_id)) { |
There was a problem hiding this comment.
@fallaciousreasoning I needed to add this conditional since I was experiencing a crash when I modified the tests to include multiple channels. Somehow that was giving me a CHECK crash to do with. the base::Contains concerning FlatMap. I'm not sure why we haven't been experiencing that crash in actual builds though.
There was a problem hiding this comment.
So I don't think it should be possible to hit this in production - channels is a set of all the Channels from all the LocaleInfos from all the Publishers. So if we get a channel id from a LocaleInfo it must exist in the set.
Obviously, that assumption isn't necessarily true in tests, as we can make up whatever data/channels we like. Another solution would be to make sure the list of publishers that the controller has include all possible channels.
There was a problem hiding this comment.
Do we still need this?
There was a problem hiding this comment.
As discussed, I'm inclined to leave that check in since, in that function specifically, channels doesn't necessarily have to be the full list of all channels. It could just be the channels which have a subscription. And it's comparing IDs from one function parameter with another. Since it causes a crash might be safer to defend against a scenario where we have a new Publisher with a new channel and for some reason we didn't update the in memory channels list.
There was a problem hiding this comment.
Mostly looks pretty good to me, so I'm adding a +1. Looking at this though, I'm wondering whether it would be better to just convert the methods to static on ChannelsController? I'm not sure what the separate file is buying us, and it makes it a bit less obvious where you need to go to find certain bits of information.
For example, to determine whether a channel is subscribed, I currently have to go through the prefs util. However, to subscribe to a channel, I need to go through the channels controller (though there is a subscribe method on the prefs util). What are your thoughts?
I don't think it's worth blocking on this though.
petemill
force-pushed
the
news-desktop-v2-enable
branch
from
484b162 to
d007387
Compare
|
I pushed again, removing prefs_util and associated changes! When I started I was supposing I'd need more access in to the prefs to get the tests appropriate for the V2 flag, but it seems like you've already done good work there @fallaciousreasoning and I only needed to add 1 test-accessible function. |
| << " is in channel " << locale_info->locale << "." | ||
| << channel_id << " which is subscribed to."; | ||
| return true; | ||
| if (channels.contains(channel_id)) { |
There was a problem hiding this comment.
Do we still need this?
Enable news v2 by default for Desktop
|
Verification was completed on |
Resolves brave/brave-browser#26494
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: