This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #3249: Add RSS feed parser to Brave Today #3317

Merged

kylehickinson merged 17 commits into development from brave-today-rss

Mar 12, 2021

Collaborator

kylehickinson commented Feb 16, 2021 •

edited

Loading

Summary of Changes

This pull request fixes #3249

Submitter Checklist:

Unit Tests are updated to cover new or changed functionality
User-facing strings use NSLocalizableString()

Test Plan:

On Add Source page:

Test being able to search for feeds via a website (example: type in "9to5mac.com", "theverge.com", etc.)
Test being able to add feed directly via URL

Repeat tests with all of the above but using pages that don't exist (blahblahblahdfdsa.com), pages that don't have feeds (google.com), pages that have only 1 feed, pages that have multiple feeds.

~~Test being able to add feeds via OPML by pasting an OPML URL in the search field (examples: https://planet.mozilla.org/opml.xml, http://hosting.opml.org/dave/spec/subscriptionList.opml)~~
~~Test being able to add feeds via OPML by using the "Import OPML" button below (copy above examples into Files.app)~~

~~Repeat tests above with OPML files that don't include RSS feeds (http://hosting.opml.org/dave/spec/states.opml)~~

On Brave Today settings

Verify custom sources are added there
Verify that they can be deleted via swipe gesture

On Brave Today

Verify custom sources appear in the Brave Today page
Verify they can be enabled/disabled via long-press like normal sources
- After toggling, verify their enabled/disabled status is correct in the Brave Today settings

Reviewer Checklist:

Issues include necessary QA labels:
- QA/(Yes|No)
- release-notes/(include|exclude)
- bug / enhancement
Necessary security reviews have taken place.
Adequate unit test coverage exists to prevent regressions.
Adequate test plan exists for QA to validate (if applicable).
Issue is assigned to a milestone (should happen at merge time).

kylehickinson requested review from soner-yuksel, iccub and Brandon-T

February 16, 2021 19:27

kylehickinson added the needs security review label

iccub suggested changes

View reviewed changes

Contributor

iccub left a comment

left some nitpick comments, will run the app later and make another review round

Client/Frontend/Brave Today/Composer/FeedDataSource+RSS.swift Show resolved Hide resolved

Client/Frontend/Brave Today/Composer/FeedDataSource+RSS.swift Outdated Show resolved Hide resolved

Client/Frontend/Brave Today/Composer/FeedDataSource.swift Outdated Show resolved Hide resolved

Client/Frontend/Brave Today/Composer/FeedDataSource.swift Outdated Show resolved Hide resolved

Client/Frontend/Brave Today/Composer/FeedDataSource.swift Outdated Show resolved Hide resolved

Client/Frontend/Settings/Brave Today/BraveTodayAddSourceViewController.swift Show resolved Hide resolved

Data/models/Model.xcdatamodeld/Model11.xcdatamodel/contents Show resolved Hide resolved

Data/models/RSSFeedSource.swift Outdated Show resolved Hide resolved

Data/models/RSSFeedSource.swift Show resolved Hide resolved

Data/models/RSSFeedSource.swift Show resolved Hide resolved

kylehickinson force-pushed the brave-today-rss branch from 80959b5 to 8ad4192 Compare

February 19, 2021 15:41

iccub approved these changes

View reviewed changes

kylehickinson force-pushed the brave-today-rss branch from b358e1e to 487aa47 Compare

February 23, 2021 17:53

jumde reviewed

View reviewed changes

Client/Frontend/Brave Today/Composer/FeedDataSource+RSS.swift Show resolved Hide resolved

jumde reviewed

View reviewed changes

Client/Frontend/Brave Today/Composer/FeedDataSource+RSS.swift Outdated

+                          }
+                          if description.isEmpty, let text = doc?.root?.childNodes(ofTypes: [.Text, .Element]).map({ node in
+                              node.stringValue
+                                  .trimmingCharacters(in: .whitespacesAndNewlines)

Contributor

jumde Feb 24, 2021

text nodes can have html content as well, may be escapeHTML here?

Collaborator Author

kylehickinson Feb 25, 2021

I tested what stringValue returns here and it automatically decodes HTML entities (i.e. & became &), so I don't think we need further calls

jumde reviewed

View reviewed changes

Client/Frontend/Brave Today/Composer/FeedDataSource+RSS.swift Outdated

+                      if let html = feedItem.contentHtml {
+                          let doc = try? HTMLDocument(string: html)
+                          if imageURL == nil, let src = doc?.firstChild(xpath: "//img[@src]")?.attr("src") {
+                              imageURL = URL(string: src, relativeTo: location.url.domainURL)

Contributor

jumde Feb 24, 2021

Filter for only valid schemes http, https:, data: (may be)?

jumde reviewed

View reviewed changes

Client/Frontend/Brave Today/Composer/FeedDataSource+RSS.swift Outdated

+                      var description = ""
+                      var imageURL: URL?
+                      if let thumbnail = feedItem.media?.mediaThumbnails?.first?.attributes?.url {
+                          imageURL = URL(string: thumbnail, relativeTo: location.url.domainURL)

Contributor

jumde Feb 24, 2021

same as: https://github.com/brave/brave-ios/pull/3317/files#r581598920

jumde reviewed

View reviewed changes

Client/Frontend/Brave Today/Composer/FeedDataSource+RSS.swift Outdated

+                      } else if feedItem.content?.attributes?.type == "html", let html = feedItem.content?.value {
+                          // Find one in description?
+                          let doc = try? HTMLDocument(string: html)
+                          if imageURL == nil, let src = doc?.firstChild(xpath: "//img[@src]")?.attr("src") {

Contributor

jumde Feb 24, 2021

same as: https://github.com/brave/brave-ios/pull/3317/files#r581598920

kylehickinson force-pushed the brave-today-rss branch from 487aa47 to dc636a4 Compare

February 24, 2021 22:11

kylehickinson requested a review from jumde

February 25, 2021 21:20

jumde reviewed

View reviewed changes

Client/Frontend/Brave Today/Composer/FeedDataSource+RSS.swift Show resolved Hide resolved

jumde reviewed

View reviewed changes

Client/Frontend/Settings/Brave Today/BraveTodayAddSourceResultsViewController.swift Outdated

		@@ -0,0 +1,137 @@
		// Copyright 2020 The Brave Authors. All rights reserved.

Contributor

jumde Mar 1, 2021

nit: 2021

Collaborator Author

kylehickinson Mar 1, 2021

Ah that's because our custom header is hard-coded, I'll open up a separate PR to update it, but all files created since Jan will have 2020

kylehickinson requested a review from jumde

March 1, 2021 16:11

kylehickinson force-pushed the brave-today-rss branch from 87020e9 to d0800bf Compare

March 1, 2021 19:26

Brandon-T suggested changes

View reviewed changes

Client/Frontend/Brave Today/Composer/FeedDataSource.swift Show resolved Hide resolved

Client/Frontend/Settings/Brave Today/BraveTodayAddSourceViewController.swift Show resolved Hide resolved

jumde approved these changes

View reviewed changes

Contributor

jumde left a comment

lgtm

kylehickinson requested a review from Brandon-T

March 4, 2021 19:22

kylehickinson added this to the 1.25 milestone

kylehickinson added the blocked label

kylehickinson modified the milestones: 1.25, 1.24

kylehickinson removed the blocked label

kylehickinson added 3 commits

March 11, 2021 17:02


          WIP: Add RSS support to Brave Today

e4a3ad5


          Support JSON feeds. Parse relative images URLs correctly

114d35b


          Add support for adding RSS feed from share sheet

d3369ff

kylehickinson added 12 commits

March 11, 2021 17:02


          Hide user sources from the All Sources list

f4091d7


          Handle errors when adding user sources

a148880


          Add OPML parsing

732ab23


          Add missed localization strings

ee34db7


          Only load RSS feeds that are enabled; Use deferred instead of groups

72a1c34


          Allow adding feeds via share sheet if viewing feed URL directly

28271c5


          Check for lowercased/normalized refresh http-equiv

07a9f85


          Add activity indicator when searching for/loading rss feeds

dffb50c


          Fix presentation on iOS 12

a16ec8b


          Split insecure sources and secure sources when adding custom sources

da5ea85


          Validate url schemes when parsing out feeds and image urls

bcb1de3


          Update file header year on new files

401043b

kylehickinson force-pushed the brave-today-rss branch from d0800bf to 401043b Compare

March 11, 2021 22:02


          Add freemium constraints

eb189b4

kylehickinson requested a review from iccub

March 11, 2021 22:49

Collaborator Author

kylehickinson commented Mar 11, 2021

@iccub Sorry some requirements changed post approval, can you please review the most recent commit! Thanks!

kylehickinson commented

View reviewed changes

Client.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

Comment on lines +4 to +12

+                    {
+                      "package": "FeedKit",
+                      "repositoryURL": "https://github.com/nmdias/FeedKit.git",
+                      "state": {
+                        "branch": null,
+                        "revision": "68493a33d862c33c9a9f67ec729b3b7df1b20ade",
+                        "version": "9.1.2"
+                      }
+                    },

Collaborator Author

kylehickinson Mar 11, 2021

I think somewhere along the line of rebase + force push it got removed, nothing changed with the version

Contributor

iccub commented Mar 11, 2021

will re-review tomorrow morning, thanks

iccub reviewed

View reviewed changes

Contributor

iccub left a comment

found small stuff only, will run the app now and leave final review

Client/Frontend/Brave Today/Composer/FeedDataSource.swift Outdated

+                                      case .success(let feed):
+                                          self.sources.append(feed.source)
+                                          self.items.append(contentsOf: feed.items)
+                                      case .failure(_):

Contributor

iccub Mar 12, 2021

we can skip brackets here i believe

Client/Frontend/Brave Today/OPML/OPML.swift Outdated

+                  static func parse(data: Data) -> OPML? {
+                      guard let document = try? XMLDocument(data: data),
+                            let _ = document.firstChild(xpath: "//opml") else {
+                          log.info("Failed to parse XML document")

Contributor

iccub Mar 12, 2021

warning or error log levels would be better here imo

Client/Frontend/Settings/Brave Today/BraveTodayAddSourceResultsViewController.swift Outdated

+                  }
+                  override func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
+                      return section == 0 ? secureLocations.count : insecureLocations.count

Contributor

iccub Mar 12, 2021

return can be skipped

Client/Frontend/Settings/Brave Today/BraveTodayAddSourceViewController.swift Outdated

+                      pageTask = session.dataTask(with: url) { [weak self] (data, response, error) in
+                          guard let self = self else { return }
+                          if let error = error as? URLError, error.code == .cancelled {
+                              return

Contributor

iccub Mar 12, 2021

do we not want to return completion handler here?

Collaborator Author

kylehickinson Mar 12, 2021

Good point, no reason to handle cancelled and return without handler. If we dismiss the screen, the error will not display anyways since self will be nil


          Code style cleanup and address minor feedback

05cd4b3

iccub approved these changes

View reviewed changes

Brandon-T approved these changes

View reviewed changes

kylehickinson merged commit ec963f6 into development

kylehickinson deleted the brave-today-rss branch

March 12, 2021 17:21

srirambv mentioned this pull request

Add Custom RSS to Brave Today #3249

Closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs security review