[Brave News]: Update to new locale format #15681

fallaciousreasoning · 2022-10-27T00:29:39Z

Note: This is backwards compatible with the old locales format. I've filed brave/brave-browser#26307 so we can remove the backwards compatible bit once we update the API.

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Brave News V2 should work the same as before

petemill

Everything looks great and seems to work well - just few pieces of feedback here.

There's a build error in Storybook due to stories/today.tsx which is still providing a string array for the locales field of a Publisher.

! [tsl] ERROR in /Users/jenkins/jenkins/workspace/pr-brave-browser-bn-locale-parsing-macos/src/brave/components/brave_new_tab_ui/stories/today.tsx(59,19)
[2022-10-27T02:03:28.456Z] ERR!       TS2322: Type 'string' is not assignable to type 'LocaleInfo'.

components/brave_new_tab_ui/api/brave_news/news.ts

components/brave_today/browser/channels_controller.cc

petemill · 2022-10-30T16:21:47Z

components/brave_new_tab_ui/components/default/braveToday/customize/Context.tsx

@@ -110,12 +111,17 @@ export const useBraveNews = () => {
 export const useChannels = (options: { subscribedOnly: boolean } = { subscribedOnly: false }) => {
  const { channels } = useBraveNews()
  return useMemo(() => Object.values(channels)
-    .filter(c => c.subscribed || !options.subscribedOnly), [channels, options.subscribedOnly])
+    .filter(c => c.subscribedLocales.length || !options.subscribedOnly), [channels, options.subscribedOnly])


Is this going to result in displaying all global channels in the initial screen of the Discover section? If so, I think we need to filter to the "current" locale.

So this is used by the sidebar to ensure we show channels the user has subscribed to in all locales. Thinking about it though, I think we're already showing all channels in all locales (because the channels system just gets all channels from all publishers).

I think (as the system already works like this) we should fix it in a separate PR, as we can't land Rana's backend changes until this is ready.

I've filed this issue:
brave/brave-browser#26385

fallaciousreasoning · 2022-10-31T00:58:57Z

@petemill could you PTAL. I fixed the storybook issues added by this PR but I think something in rewards is also broken on master. I had a quick look at fixing it, but there's some prod code which is breaking the Storybook build, and I wasn't sure how to fix it (on this line, https://github.com/brave/brave-core/blob/master/components/brave_vpn/resources/panel/state/store.ts#L47 observer should be implementing ServiceObserverInterface but doesn't).

brave-builds · 2022-10-31T21:55:44Z

A Storybook has been deployed to preview UI for the latest push

petemill · 2022-10-31T22:18:41Z

SonarCloud is only complaining about code duplication in the storybook story. I have again attempted to re-configure SonarCloud to ignore this, but we can't re-run without pushing again and that would interrupt the rest of CI. So if the rest passes then we can force merge.

brave-builds · 2022-10-31T22:41:11Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-11-01T01:11:23Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-11-01T05:27:22Z

A Storybook has been deployed to preview UI for the latest push

petemill · 2022-11-01T19:58:36Z

@fallaciousreasoning Please uplift to 1.46.x when you get a chance

stephendonner · 2022-11-01T21:41:39Z

Verified `PASSED` using

Brave	1.47.36 Chromium: 107.0.5304.91 (Official Build) nightly (x86_64)
Revision	3d5948960d62418160796d5831a4d2d7d6c90fa8-refs/branch-heads/5304@{#1097}
OS	macOS Version 11.7.1 (Build 20G918)

Steps:

installed 1.47.36
launched Brave
opened brave://flags
set brave://flags/#brave-news-v2 to Enabled
ran from the commandline: --brave-today-host=brave-today-cdn.bravesoftware.com
opened a new-tab page
clicked on Customize
clicked on Turn on Brave News
typed cnn
examined the results

Confirmed I only saw one `CNN` source on staging (`brave-today-cdn.bravesoftware.com`)

fallaciousreasoning · 2022-11-01T21:49:12Z

Uplift is here #15749

[Uplift] [Brave News]: Update to new locale format (#15681)

github-actions bot assigned fallaciousreasoning Oct 27, 2022

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Oct 27, 2022

fallaciousreasoning requested a review from petemill October 27, 2022 01:19

stephendonner mentioned this pull request Oct 28, 2022

Brave News Customize 2.0 UI brave/brave-browser#25801

Closed

petemill requested changes Oct 30, 2022

View reviewed changes

fallaciousreasoning force-pushed the bn-locale-parsing branch from c56f2ab to 79dcc1f Compare October 31, 2022 00:54

fallaciousreasoning requested a review from petemill October 31, 2022 00:54

fallaciousreasoning force-pushed the bn-locale-parsing branch from e95eb18 to 0677b41 Compare October 31, 2022 22:00

petemill approved these changes Oct 31, 2022

View reviewed changes

fallaciousreasoning added 11 commits November 1, 2022 12:54

Update mojom

67fcdd7

Update parser

81e9889

Get project compiling

e153f49

Get channels working again

5c94aa4

Always pass locale into subscribing to a channel

1e46b17

Get unittests compiling

7fc454e

Fix channels_controller unittests

66e80dc

Fix storybook build

8afef20

Fix potential issue with dots in channel names

9c265c8

Fix speculative availableLocales update

0f1c822

Fix failing test

2e8fd2b

fallaciousreasoning force-pushed the bn-locale-parsing branch 2 times, most recently from 2d5b9bc to 7e28e06 Compare November 1, 2022 00:27

Fix failing tests on Android

791d600

fallaciousreasoning force-pushed the bn-locale-parsing branch from 7e28e06 to 791d600 Compare November 1, 2022 04:42

fallaciousreasoning enabled auto-merge (squash) November 1, 2022 04:48

fallaciousreasoning merged commit 43ac272 into master Nov 1, 2022

fallaciousreasoning deleted the bn-locale-parsing branch November 1, 2022 05:43

github-actions bot added this to the 1.47.x - Nightly milestone Nov 1, 2022

fallaciousreasoning mentioned this pull request Nov 1, 2022

[Uplift] [Brave News]: Update to new locale format (#15681) #15749

Merged

fallaciousreasoning added a commit that referenced this pull request Nov 3, 2022

[Brave News]: Update to new locale format (#15681)

a30be3b

kjozwiak added a commit that referenced this pull request Nov 4, 2022

Merge pull request #15749 from brave/bn-locale-parsing-uplift

7b64ea5

[Uplift] [Brave News]: Update to new locale format (#15681)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Brave News]: Update to new locale format #15681

[Brave News]: Update to new locale format #15681

fallaciousreasoning commented Oct 27, 2022 •

edited

Loading

petemill left a comment •

edited

Loading

petemill Oct 30, 2022

fallaciousreasoning Oct 31, 2022

fallaciousreasoning Oct 31, 2022

fallaciousreasoning commented Oct 31, 2022

brave-builds commented Oct 31, 2022

petemill commented Oct 31, 2022

brave-builds commented Oct 31, 2022

brave-builds commented Nov 1, 2022

brave-builds commented Nov 1, 2022

petemill commented Nov 1, 2022

stephendonner commented Nov 1, 2022

fallaciousreasoning commented Nov 1, 2022

[Brave News]: Update to new locale format #15681

[Brave News]: Update to new locale format #15681

Conversation

fallaciousreasoning commented Oct 27, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

petemill left a comment • edited Loading

Choose a reason for hiding this comment

petemill Oct 30, 2022

Choose a reason for hiding this comment

fallaciousreasoning Oct 31, 2022

Choose a reason for hiding this comment

fallaciousreasoning Oct 31, 2022

Choose a reason for hiding this comment

fallaciousreasoning commented Oct 31, 2022

brave-builds commented Oct 31, 2022

petemill commented Oct 31, 2022

brave-builds commented Oct 31, 2022

brave-builds commented Nov 1, 2022

brave-builds commented Nov 1, 2022

petemill commented Nov 1, 2022

stephendonner commented Nov 1, 2022

Verified PASSED using

Steps:

Confirmed I only saw one CNN source on staging (brave-today-cdn.bravesoftware.com)

fallaciousreasoning commented Nov 1, 2022

fallaciousreasoning commented Oct 27, 2022 •

edited

Loading

petemill left a comment •

edited

Loading

Verified `PASSED` using

Confirmed I only saw one `CNN` source on staging (`brave-today-cdn.bravesoftware.com`)