Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Nov 1, 2022
14 changes: 12 additions & 2 deletions components/brave_new_tab_ui/api/brave_news/news.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,26 @@ class BraveNewsApi {
async setChannelSubscribed (channelId: string, subscribed: boolean) {
// While we're waiting for the new channels to come back, speculatively
// update them, so the UI has instant feedback.
// This will be overwritten when the controller responds.
let subscribedLocales = this.lastChannels[channelId]?.subscribedLocales ?? []
if (subscribedLocales.includes(this.locale)) {
// Remove this locale from the list of subscribed locales.
subscribedLocales = subscribedLocales.filter(l => l !== this.locale)
} else {
// Add this locale to the list of subscribed locales.
subscribedLocales.push(this.locale)
}

this.updateChannels({
...this.lastChannels,
[channelId]: {
...this.lastChannels[channelId],
subscribed
subscribedLocales
}
})

// Then, once we receive the actual update, apply it.
const { updated } = await this.controller.setChannelSubscribed(channelId, subscribed)
const { updated } = await this.controller.setChannelSubscribed(this.locale, channelId, subscribed)
this.updateChannels({
...this.lastChannels,
[channelId]: updated
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ export function BraveNewsContextProvider (props: { children: React.ReactNode })

const filteredPublisherIds = useMemo(() =>
sortedPublishers
.filter(p => p.type === PublisherType.DIRECT_SOURCE || p.locales.includes(api.locale))
.filter(p => p.type === PublisherType.DIRECT_SOURCE ||
p.locales.some(l => l.locale === api.locale))
.map(p => p.publisherId),
[sortedPublishers])

Expand Down Expand Up @@ -109,12 +110,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])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* Determines whether the channel is subscribed in the current locale.
* @param channelName The channel
* @returns A getter & setter for whether the channel is subscribed
*/
export const useChannelSubscribed = (channelName: string) => {
const { channels } = useBraveNews()
const subscribed = useMemo(() => channels[channelName]?.subscribed ?? false, [channels[channelName]])
const subscribed = useMemo(() => channels[channelName]?.subscribedLocales.includes(api.locale) ?? false, [channels[channelName]])
const setSubscribed = React.useCallback((subscribed: boolean) => {
api.setChannelSubscribed(channelName, subscribed)
}, [channelName])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,17 @@ function isPublisherContentAllowed (publisher: Publisher, channels: Channels): b
if (!newTabData.featureFlagBraveNewsV2Enabled) return publisher.isEnabled

// Otherwise, we're using the channels API - the publisher is allowed if it's
// in one of the channels we're subscribed to.
return publisher.channels.some(c => channels[c]?.subscribed)
// in any of the channels we're subscribed to, in any of the locales the
// channel is available in.
for (const localeInfo of publisher.locales) {
for (const channel of localeInfo.channels) {
if (channels[channel].subscribedLocales.includes(localeInfo.locale)) {
return true
}
}
}

return false
}

/**
Expand All @@ -47,7 +56,14 @@ function isPublisherContentAllowed (publisher: Publisher, channels: Channels): b
* @param publisher The publisher to get channels for.
*/
export function getPublisherChannels (publisher: Publisher) {
return newTabData.featureFlagBraveNewsV2Enabled ? publisher.channels : [publisher.categoryName]
const allChannels = new Set<string>()
for (const localeInfo of publisher.locales) {
for (const channel of localeInfo.channels) {
allChannels.add(channel)
}
}

return newTabData.featureFlagBraveNewsV2Enabled ? Array.from(allChannels) : [publisher.categoryName]
}

export const DynamicListContext = React.createContext<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,19 +104,13 @@ export default function Sources (props: SourcesProps) {
for (const publisher of Object.values(props.publishers)) {
// If the publisher has a locale (which can only happen in the V2 API) and
// it doesn't include the current locale, skip over it.
if (publisher.locales.length !== 0 && !publisher.locales.includes(locale)) {
if (publisher.locales.length !== 0 && !publisher.locales.some(l => l.locale === locale)) {
continue
}

// Do not include user feeds, as they are separated
if (publisher.type === PublisherType.DIRECT_SOURCE) continue

// If the publisher has a locale (which can only happen in the V2 API) and
// it doesn't include the current locale, skip over it.
if (publisher.locales.length !== 0 && !publisher.locales.includes(locale)) {
continue
}

const forAll = result.get(categoryNameAll)!
forAll.push(publisher)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: 'direct:https://example.com/feed1',
publisherName: 'My Custom Feed 1',
categoryName: 'User feeds',
channels: ['User feeds'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['User feeds'], rank: 0 }],
type: BraveNews.PublisherType.DIRECT_SOURCE,
isEnabled: true,
userEnabledStatus: BraveNews.UserEnabled.ENABLED
Expand All @@ -45,13 +44,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: 'direct:https://example.com/feed2',
publisherName: 'My Custom Feed 2',
categoryName: 'User feeds',
channels: ['User feeds'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['User feeds'], rank: 0 }],
type: BraveNews.PublisherType.DIRECT_SOURCE,
isEnabled: true,
userEnabledStatus: BraveNews.UserEnabled.ENABLED
Expand All @@ -60,13 +58,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: '5eece347713f329f156cd0204cf9b12629f1dc8f4ea3c1b67984cfbfd66cdca5',
publisherName: 'Test Publisher 1',
categoryName: 'Tech',
channels: ['Tech'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['Tech'], rank: 0 }],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: false,
userEnabledStatus: BraveNews.UserEnabled.ENABLED
Expand All @@ -75,13 +72,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: '4eece347713f329f156cd0204cf9b12629f1dc8f4ea3c1b67984cfbfd66cdca5',
publisherName: 'Test Publisher 2',
categoryName: 'Top News',
channels: ['Top News'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['Top News'], rank: 0 }],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: false,
userEnabledStatus: BraveNews.UserEnabled.NOT_MODIFIED
Expand All @@ -90,13 +86,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: '5eece347713f329f156cd0204cf9b12629f1dc8f4ea3c1b67984cfbfd66cdca5',
publisherName: 'Test Publisher 3',
categoryName: 'Tech 2',
channels: ['Tech 2'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['Tech 2'], rank: 0 }],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: false,
userEnabledStatus: BraveNews.UserEnabled.ENABLED
Expand All @@ -105,13 +100,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: '4eece347713f329f156cd0204cf9b12629f1dc8f4ea3c1b67984cfbfd66cdca5',
publisherName: 'Test Publisher 4',
categoryName: 'Top News 1',
channels: ['Top News 1'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['Top News 1'], rank: 0 }],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: false,
userEnabledStatus: BraveNews.UserEnabled.NOT_MODIFIED
Expand All @@ -120,13 +114,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: '5eece347713f329f156cd0204cf9b12629f1dc8f4ea3c1b67984cfbfd66cdca5',
publisherName: 'Test Publisher 5 has A very very very very very very very very very very very very very very very very very very very very long publisher name',
categoryName: 'Tech 2',
channels: ['Tech 2'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['Tech 2'], rank: 0 }],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: false,
userEnabledStatus: BraveNews.UserEnabled.ENABLED
Expand All @@ -135,13 +128,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: '4eece347713f329f156cd0204cf9b12629f1dc8f4ea3c1b67984cfbfd66cdca5',
publisherName: 'Test Publisher 6',
categoryName: 'Top News 2',
channels: ['Top News 2'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['Top News 2'], rank: 0 }],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: false,
userEnabledStatus: BraveNews.UserEnabled.NOT_MODIFIED
Expand All @@ -150,13 +142,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: '4eece347713f329f156cd0204cf9b12629f1dc8f4ea3c1b67984cfbfd66cdca5',
publisherName: 'Test Publisher 7',
categoryName: 'Top News 3',
channels: ['Top News 3'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['Top News 3'], rank: 0 }],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: false,
userEnabledStatus: BraveNews.UserEnabled.NOT_MODIFIED
Expand All @@ -165,13 +156,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: '5eece347713f329f156cd0204cf9b12629f1dc8f4ea3c1b67984cfbfd66cdca5',
publisherName: 'Test Publisher 8',
categoryName: 'Tech 3',
channels: ['Tech 3'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['Tech 3'], rank: 0 }],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: false,
userEnabledStatus: BraveNews.UserEnabled.ENABLED
Expand All @@ -180,13 +170,12 @@ export const publishers: BraveNews.Publishers = {
publisherId: '4eece347713f329f156cd0204cf9b12629f1dc8f4ea3c1b67984cfbfd66cdca5',
publisherName: 'Test Publisher 9',
categoryName: 'Top News 4',
channels: ['Top News 4'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{ locale: 'en_US', channels: ['Top News 4'], rank: 0 }],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: false,
userEnabledStatus: BraveNews.UserEnabled.NOT_MODIFIED
Expand Down Expand Up @@ -1995,7 +1984,11 @@ export const mockBraveNewsController: Partial<BraveNewsControllerRemote> = {

async getChannels () {
const channelNames = Object.values(publishers).reduce((prev, next) => {
for (const channel of next.channels) prev.add(channel)
for (const locale of next.locales) {
for (const channel of locale.channels) {
prev.add(channel)
}
}
prev.add(next.categoryName)
return prev
}, new Set<string>())
Expand Down
14 changes: 10 additions & 4 deletions components/brave_new_tab_ui/stories/today.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,16 @@ export const Publisher = () => (
publisherId: '123abc',
publisherName: text('Publisher Name', 'small'),
categoryName: 'Top News',
channels: ['Top News', 'Top Sources'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{
locale: 'en_US',
rank: 0,
channels: ['Top News', 'Top Sources']
}],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: true,
userEnabledStatus: BraveNews.UserEnabled.NOT_MODIFIED
Expand All @@ -68,13 +71,16 @@ export const Publisher = () => (
publisherId: '123abcdef',
publisherName: text('Publisher Name 2', 'The Miller Chronicle'),
categoryName: 'Top News',
channels: ['Top News', 'Top Sources'],
feedSource: { url: 'http://www.example.com/feed' },
backgroundColor: undefined,
coverUrl: undefined,
faviconUrl: undefined,
siteUrl: { url: 'https://www.example.com' },
locales: ['en_US'],
locales: [{
locale: 'en_US',
rank: 0,
channels: ['Top News', 'Top Sources']
}],
type: BraveNews.PublisherType.COMBINED_SOURCE,
isEnabled: true,
userEnabledStatus: BraveNews.UserEnabled.NOT_MODIFIED
Expand Down
21 changes: 5 additions & 16 deletions components/brave_today/browser/brave_news_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -184,28 +184,17 @@ void BraveNewsController::FindFeeds(const GURL& possible_feed_or_site_url,
}

void BraveNewsController::GetChannels(GetChannelsCallback callback) {
publishers_controller_.GetLocale(base::BindOnce(
[](ChannelsController* channels_controller, GetChannelsCallback callback,
const std::string& locale) {
channels_controller->GetAllChannels(locale, std::move(callback));
},
base::Unretained(&channels_controller_), std::move(callback)));
channels_controller_.GetAllChannels(std::move(callback));
}

void BraveNewsController::SetChannelSubscribed(
const std::string& locale,
const std::string& channel_id,
bool subscribed,
SetChannelSubscribedCallback callback) {
publishers_controller_.GetLocale(base::BindOnce(
[](ChannelsController* channels_controller, const std::string& channel_id,
bool subscribed, SetChannelSubscribedCallback callback,
const std::string& locale) {
auto result = channels_controller->SetChannelSubscribed(
locale, channel_id, subscribed);
std::move(callback).Run(std::move(result));
},
base::Unretained(&channels_controller_), channel_id, subscribed,
std::move(callback)));
auto result =
channels_controller_.SetChannelSubscribed(locale, channel_id, subscribed);
std::move(callback).Run(std::move(result));
}

void BraveNewsController::SubscribeToNewDirectFeed(
Expand Down
3 changes: 2 additions & 1 deletion components/brave_today/browser/brave_news_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ class BraveNewsController : public KeyedService,
void FindFeeds(const GURL& possible_feed_or_site_url,
FindFeedsCallback callback) override;
void GetChannels(GetChannelsCallback callback) override;
void SetChannelSubscribed(const std::string& channel_id,
void SetChannelSubscribed(const std::string& locale,
const std::string& channel_id,
bool subscribed,
SetChannelSubscribedCallback callback) override;
void SubscribeToNewDirectFeed(
Expand Down
Loading