For #9599: Ability to edit top site URLs #9600

pepve · 2021-02-04T00:07:23Z

~~Add functionality to edit both the title and the URL of a top site simultaneously~~
~~Deprecate functionality to edit only the title of a top site~~

Replace the functionality to rename top sites with functionality to update both the title and the URL.

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.

codecov · 2021-02-04T00:17:55Z

Codecov Report

Merging #9600 (f84dc4e) into master (69a800e) will decrease coverage by 7.26%.
The diff coverage is 80.00%.

@@             Coverage Diff              @@
##             master    #9600      +/-   ##
============================================
- Coverage     73.89%   66.63%   -7.27%     
+ Complexity     5576      553    -5023     
============================================
  Files           768      120     -648     
  Lines         28608     3809   -24799     
  Branches       4710      614    -4096     
============================================
- Hits          21140     2538   -18602     
+ Misses         5023      958    -4065     
+ Partials       2445      313    -2132

Impacted Files	Coverage Δ	Complexity Δ
...onents/feature/top/sites/DefaultTopSitesStorage.kt	`70.27% <0.00%> (ø)`	`12.00 <0.00> (ø)`
...la/components/feature/top/sites/TopSitesStorage.kt	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
.../components/feature/top/sites/PinnedSiteStorage.kt	`78.12% <80.00%> (+78.12%)`	`10.00 <2.00> (+10.00)`
...a/components/feature/top/sites/TopSitesUseCases.kt	`82.35% <100.00%> (ø)`	`1.00 <0.00> (ø)`
...onents/feature/customtabs/CustomTabConfigHelper.kt
...a/components/browser/search/SearchEngineManager.kt
...components/browser/state/reducer/TabListReducer.kt
.../feature/downloads/SimpleDownloadDialogFragment.kt
...java/mozilla/components/concept/menu/MenuButton.kt
...engine/manifest/parser/WebAppManifestIconParser.kt
... and 645 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 69a800e...f84dc4e. Read the comment docs.

gabrielluong · 2021-02-04T00:40:14Z

...ature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt

@@ -76,6 +76,16 @@ class DefaultTopSitesStorage(
        }
    }

+    override fun editTopSite(topSite: TopSite, title: String, url: String) {


Let's remove renameTopSite since I don't imagine any consumers of it after this. Can we also call it updateTopSite instead of editTopSite?

Done and done!

...ts/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/PinnedSiteStorage.kt

gabrielluong · 2021-02-04T03:43:42Z

I'm planning to look at how the edit bookmarks (use case if there is one?) works tomorrow to investigate how it works - looking for clues on how/if it handles empty URL provided and perhaps malformed URLs. Will also look at other browser to see how much we will need to dive into this. Feel free to also investigate this, I just want to make sure we cover our bases. Thanks again for contributing!

mergify · 2021-02-06T17:06:37Z

This pull request has conflicts when rebasing. Could you fix it @pepve? 🙏

pepve · 2021-02-10T16:22:14Z

I just rebased on the latest master. I also added a commit with a test to improve the (measured) test coverage, please review, I'm not entirely sure of the mockDao function I came up with.

About validation, can we pick that up in a separate issue? From the AC perspective it was already possible to add any random string as a URL. And the new update logic doesn't change that. The new issue will then of course block the Fenix issue I'm doing this for: #17636.

mergify · 2021-02-12T11:23:06Z

This pull request has conflicts when rebasing. Could you fix it @pepve? 🙏

.../src/androidTest/java/mozilla/components/feature/top/sites/OnDevicePinnedSitesStorageTest.kt

gabrielluong · 2021-02-12T15:16:02Z

...ts/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/PinnedSiteStorage.kt

+    @VisibleForTesting
+    internal var currentTimeMillis: () -> Long = { System.currentTimeMillis() }


There's nothing wrong in particular about this, but from looking around our codebase, I would be more comfortable being consistent and calling System.currentTimeMillis() where it is.

I use it to inject a constant time from the tests (in addAllDefaultSites and addPinnedSite) so that I can use verify without much effort. What would be a good alternative?

I think we can just check for anyLong() as an alternative.

I tried that but I think it doesn't work because the long is wrapped in an object. I think this works:
verify(obj).method(eq("foo"), anyLong())
But this doesn't:
verify(obj).method(SomeClass(eq("foo"), anyLong()))
And our case matches the latter. But I don't really have a lot of experience in this area so please correct me if I'm missing something. The real problem I think we're having is that there is no consistent way to mock time used throughout the project. Code that does straight calls to System.currentTimeMillis() is just not very unit-testable. Perhaps we should make an issue about this?

...s/support/migration/src/test/java/mozilla/components/support/migration/FennecMigratorTest.kt

gabrielluong · 2021-02-12T15:58:59Z

docs/changelog.md

@@ -49,6 +49,9 @@ permalink: /changelog/
 * **lib-state**
  * Add `threadNamePrefix` parameter to `Store` to give threads created by the `Store` a specific name.

+* **feature-top-sites**
+  * ⚠️ **This is a breaking change**: Replace `TopSitesUseCases.renameTopSites` (which only updates the title) with `TopSitesUseCases.updateTopSites` (which also updates the url). [#9599](https://github.com/mozilla-mobile/android-components/issues/9599)


Suggested change

* ⚠️ **This is a breaking change**: Replace `TopSitesUseCases.renameTopSites` (which only updates the title) with `TopSitesUseCases.updateTopSites` (which also updates the url). [#9599](https://github.com/mozilla-mobile/android-components/issues/9599)

* ⚠️ **This is a breaking change**: Replace `TopSitesUseCases.renameTopSites` with `TopSitesUseCases.updateTopSites` which will allow for updating the title and url of a top site. [#9599](https://github.com/mozilla-mobile/android-components/issues/9599)

gabrielluong · 2021-02-12T16:05:03Z

...ature/top-sites/src/test/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt

+class PinnedSitesStorageTest {
+
+    @Test
+    fun `add all default sites`() = runBlocking {


Perhaps we can simply use the addAllPinnedSites as the test name and the others.

gabrielluong · 2021-02-12T16:05:38Z

...ature/top-sites/src/test/java/mozilla/components/feature/top/sites/PinnedSitesStorageTest.kt

@@ -0,0 +1,127 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


This is nice!

gabrielluong · 2021-02-16T16:58:50Z

...ts/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/PinnedSiteStorage.kt

            )
-            entity.id = pinnedSiteDao.insertPinnedSite(entity)
+            pinnedSiteDao.insertPinnedSite(entity)


Just noticed this change from your comments in https://github.com/mozilla-mobile/android-components/pull/9600/files#r575826087, can we revert this change for now?

I see we are doing entity.id in a number of places for adding items to the RoomDB - LoginExceptionStorage, TabCollectionStorage. If we think this should be changed, I think we should make a change that also accounts for those 2 other storage. I mostly don't understand the meaning behind the change so happy to be enlighten.

I think this also resolves us from having to return Unit in the other files.

Okay I'm undoing this change. I opened #9708 for it. See also my comment here: https://github.com/mozilla-mobile/android-components/pull/9600/files#diff-3a51f71b988baa050dc7351cdbdb1d06369d75afe7fcb0951091d3d642d6318cR56-R59

gabrielluong · 2021-02-16T17:02:23Z

...ts/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/PinnedSiteStorage.kt

+    @VisibleForTesting
+    internal var currentTimeMillis: () -> Long = { System.currentTimeMillis() }


I think we can just check for anyLong() as an alternative.

mergify · 2021-02-16T18:01:30Z

This pull request has conflicts when rebasing. Could you fix it @pepve? 🙏

…ites test coverage

pepve · 2021-02-23T16:17:17Z

@gabrielluong is there anything else I can do to help progress this?

pepve · 2021-02-24T09:45:11Z

Thank you!

pepve requested review from gabrielluong and grigoryk as code owners February 4, 2021 00:07

gabrielluong self-assigned this Feb 4, 2021

gabrielluong reviewed Feb 4, 2021

View reviewed changes

gabrielluong suggested changes Feb 4, 2021

View reviewed changes

...ts/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/PinnedSiteStorage.kt Outdated Show resolved Hide resolved

pepve force-pushed the edit-top-site branch from 29788db to f0d03db Compare February 4, 2021 01:16

pepve requested a review from gabrielluong February 4, 2021 01:37

pepve force-pushed the edit-top-site branch from f0d03db to 7551ddc Compare February 10, 2021 16:06

pepve force-pushed the edit-top-site branch from 7551ddc to f26a2d4 Compare February 10, 2021 16:35

pepve requested a review from pocmo as a code owner February 10, 2021 16:35

gabrielluong suggested changes Feb 12, 2021

View reviewed changes

pepve force-pushed the edit-top-site branch from f26a2d4 to 2bb9701 Compare February 14, 2021 16:11

pepve requested a review from gabrielluong February 14, 2021 16:13

gabrielluong suggested changes Feb 16, 2021

View reviewed changes

pepve force-pushed the edit-top-site branch from 2bb9701 to 534ee77 Compare February 16, 2021 20:48

pepve added 2 commits February 16, 2021 21:53

For mozilla-mobile#9599: Ability to update top site URLs

9aa6504

For mozilla-mobile#9599: Add PinnedSitesStorageTest to increase top-s…

c350bd9

…ites test coverage

pepve force-pushed the edit-top-site branch from 534ee77 to c350bd9 Compare February 16, 2021 20:54

pepve requested a review from gabrielluong February 16, 2021 20:59

gabrielluong approved these changes Feb 23, 2021

View reviewed changes

gabrielluong added the 🛬 needs landing (squash) PRs that are ready to land (squashed) label Feb 23, 2021

Merge branch 'master' into edit-top-site

f84dc4e

mergify bot merged commit 93ee2f3 into mozilla-mobile:master Feb 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #9599: Ability to edit top site URLs #9600

For #9599: Ability to edit top site URLs #9600

pepve commented Feb 4, 2021 •

edited

Loading

codecov bot commented Feb 4, 2021 •

edited

Loading

gabrielluong Feb 4, 2021

pepve Feb 4, 2021

gabrielluong commented Feb 4, 2021

mergify bot commented Feb 6, 2021

pepve commented Feb 10, 2021

mergify bot commented Feb 12, 2021

gabrielluong Feb 12, 2021

pepve Feb 14, 2021 •

edited

Loading

gabrielluong Feb 16, 2021

pepve Feb 16, 2021

gabrielluong Feb 12, 2021

pepve Feb 14, 2021

gabrielluong Feb 12, 2021

pepve Feb 14, 2021

gabrielluong Feb 12, 2021

pepve Feb 14, 2021

gabrielluong Feb 16, 2021

pepve Feb 16, 2021

gabrielluong Feb 16, 2021

mergify bot commented Feb 16, 2021

pepve commented Feb 23, 2021

pepve commented Feb 24, 2021

		@VisibleForTesting
		internal var currentTimeMillis: () -> Long = { System.currentTimeMillis() }

	* ⚠️ This is a breaking change: Replace `TopSitesUseCases.renameTopSites` (which only updates the title) with `TopSitesUseCases.updateTopSites` (which also updates the url). [#9599](https://github.com/mozilla-mobile/android-components/issues/9599)
	* ⚠️ This is a breaking change: Replace `TopSitesUseCases.renameTopSites` with `TopSitesUseCases.updateTopSites` which will allow for updating the title and url of a top site. [#9599](https://github.com/mozilla-mobile/android-components/issues/9599)

		@@ -0,0 +1,127 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

For #9599: Ability to edit top site URLs #9600

For #9599: Ability to edit top site URLs #9600

Conversation

pepve commented Feb 4, 2021 • edited Loading

Pull Request checklist

After merge

codecov bot commented Feb 4, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong commented Feb 4, 2021

mergify bot commented Feb 6, 2021

pepve commented Feb 10, 2021

mergify bot commented Feb 12, 2021

Choose a reason for hiding this comment

pepve Feb 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Feb 16, 2021

pepve commented Feb 23, 2021

pepve commented Feb 24, 2021

pepve commented Feb 4, 2021 •

edited

Loading

codecov bot commented Feb 4, 2021 •

edited

Loading

pepve Feb 14, 2021 •

edited

Loading