Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Issue #6331: Add isDefault bool to TopSiteEntity and APIs for flagging default top sites #6715

Merged
merged 2 commits into from
Apr 28, 2020

Conversation

gabrielluong
Copy link
Member

@gabrielluong gabrielluong commented Apr 20, 2020

Fixes #6331.

@Amejia481 Just to give you some context about the changes in this PR:

In Fenix, we have added 3 default top sites (Pocket, Wikipedia, YouTube) along with telemetry.
It is not enough to just have a hardcoded list of these 3 default top sites since we can't differentiate from the user added top sites and top sites migrated from fennec.

We want to add this isDefault boolean to each TopSiteEntity so that in telemetry we can easily just query if the top site is a default top site added by the application. On the front end, we can also sort all the isDefault top sites to the beginning when we are showing the list of top sites.

In this PR, we did the following:

  • Add isDefault bool to TopSiteEntity
  • Migrate the DB to account for this new property
  • Migrate the 3 default top sites to have its isDefault bool set to true

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@gabrielluong gabrielluong requested a review from pocmo April 20, 2020 14:54
@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #6715 into master will decrease coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6715      +/-   ##
============================================
- Coverage     77.57%   77.38%   -0.19%     
+ Complexity     4748     4676      -72     
============================================
  Files           629      605      -24     
  Lines         23219    22877     -342     
  Branches       3418     3402      -16     
============================================
- Hits          18012    17704     -308     
+ Misses         3787     3755      -32     
+ Partials       1420     1418       -2     
Impacted Files Coverage Δ Complexity Δ
...a/components/support/migration/TelemetryHelpers.kt 88.17% <0.00%> (-0.40%) 0.00% <0.00%> (ø%)
...onents/support/sync/telemetry/BaseGleanSyncPing.kt 100.00% <0.00%> (ø) 11.00% <0.00%> (ø%)
...ice/pocket/data/PocketGlobalVideoRecommendation.kt
...ozilla/components/feature/tabs/ext/BrowserState.kt
...illa/components/service/pocket/ext/ConceptFetch.kt
...mponents/service/pocket/PocketListenEndpointRaw.kt
...lla/components/service/pocket/PocketEndpointRaw.kt
...illa/components/service/pocket/PocketListenURLs.kt
...illa/components/service/pocket/PocketJSONParser.kt
...ts/feature/tabs/toolbar/TabCounterToolbarButton.kt
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a30e917...6b483d8. Read the comment docs.

@gabrielluong gabrielluong added the work in progress Not ready to land yet. Work in progress (WIP). label Apr 20, 2020
@gabrielluong gabrielluong removed the request for review from pocmo April 20, 2020 17:03
@gabrielluong
Copy link
Member Author

@ekager When you have a chance, please take a scan of the PR and let me know if you have any feedback. In the migration of the database schema, we retroactively go back and set isDefault for the default top sites that you added. In Fenix, you would only need to add isDefault = true when calling addTopSite. In terms of ordering, I am wondering as an alternative to the consumer APIs you recommended -- we could potentially do getTopSites and sort/filter based on isDefault.

My hope as a next step is to add a position Integer parameter in the future so we can more actively control the ordering of the top sites and allow for further customization of the order of the top sites.

@ekager
Copy link
Contributor

ekager commented Apr 21, 2020

Migration looks great 🙌🏻 thank you for that! This all makes sense to me, but would it also be helpful to add fun to TopSiteDao for getting the default top sites via a DB query instead of me having to scan every top site for isDefault after getting them all?

@gabrielluong
Copy link
Member Author

Migration looks great 🙌🏻 thank you for that! This all makes sense to me, but would it also be helpful to add fun to TopSiteDao for getting the default top sites via a DB query instead of me having to scan every top site for isDefault after getting them all?

@ekager Just wanted to understand your use case a bit more about having a separate method for getDefaultTopSites. I believe the use case you are thinking about is for constructing the proper order of items in the AdapterView where we have the default top sites at the beginning. An alternative to adding a new method would be to do getTopSites().sortBy { !it.isDefault }.

My only worry was that we added this method and it turns out we didn't need it.

In future iterations where we look at adding reordering of top sites, we would add position Integer into the TopSiteEntity and query for getTopSites() should always return sorted by position.

@gabrielluong
Copy link
Member Author

@Amejia481 It will be helpful to read the first comment for more context about this PR and what we're trying to achieve.

@ekager
Copy link
Contributor

ekager commented Apr 27, 2020

My thought for a use case would be sending the default top site telemetry if we needed to send the size, but just reread the telemetry ticket and it sounds like we don't actually need to send how many defaults a user has still. Nevermind!

@gabrielluong gabrielluong removed the work in progress Not ready to land yet. Work in progress (WIP). label Apr 27, 2020
@gabrielluong
Copy link
Member Author

@Amejia481 This should be ready for review!

Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

I found a couple of nits, apart from that it looks pretty good.
I tested it locally and it worked pretty well 👍 , just one thing, please open a pr on Fenix to address the TopSiteItem breaking change

@Amejia481
Copy link
Contributor

bors r=ekager,Amejia481

@Amejia481 Amejia481 added the 🛬 needs landing PRs that are ready to land label Apr 28, 2020
@bors
Copy link

bors bot commented Apr 28, 2020

Build succeeded:

@bors bors bot merged commit b6245d8 into mozilla-mobile:master Apr 28, 2020
@gabrielluong gabrielluong deleted the 6331 branch April 28, 2020 15:06
bors bot pushed a commit that referenced this pull request Apr 28, 2020
6767: Close #6674: Don't crash in AppLinksUsesCases with illegal Intent URI format r=csadilek a=rocketsroger



6794:  GeckoView update (nightly) (20200428-140644) r=psymoon a=MickeyMoz



6795: Issue #6331: Update the generated top sites schemas r=Amejia481 a=gabrielluong

This is a follow up to #6715, where the top sites schema did not regenerate after fixing the review comments.



Co-authored-by: Roger Yang <[email protected]>
Co-authored-by: MickeyMoz <[email protected]>
Co-authored-by: Gabriel Luong <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add isDefault bool to TopSiteEntity and APIs for flagging default top sites
3 participants