Add RC flag for storing history #4495

CrisBarreiro · 2024-05-02T10:10:43Z

Task/Issue URL: https://app.asana.com/0/0/1206816228247406/f

Description

Add RC flag for storing history

Steps to test this PR

Feature 1

Create a JSON blob with history-> storeHistory enabled
Visit a site
Use flipper or database inspector to check site has been stored in the database, and shown in suggestions

Feature 2

Create a JSON blob with history-> storeHistory enabled
Open private search settings
Toggle off visited sites
Visit a site
Use flipper or database inspector to check site has not been stored in the database, and no suggestions are shown (even for sites previously visited)
Toggle on visited sites again
Visit a site
Use flipper or database inspector to check site has been stored in the database, and suggestions are shown

Feature 2

Create a JSON blob with history-> storeHistory disabled
Visit a site
Use flipper or database inspector to check site has not been stored in the database, and no suggestions are shown (even for sites previously visited)
Open private search settings
Check visited sites option is not visible

UI changes

CrisBarreiro · 2024-05-02T10:11:00Z

Address ship review issues #4593
Design improvements #4574
Pixel improvements #4559
Add in app message for autocomplete #4545
Add RC flag for storing history #4495 👈
Schedule deletion of items older than 30 days #4451
Clear history entries from memory and database when clicking fire button #4450
Add auto-complete support #4332 : 3 other dependent PRs (#4341 , #4572 , #4599 )
develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @CrisBarreiro and the rest of your teammates on Graphite

cmonfortep

I'm taking a pass through all PRs, leaving some comments on this one. Just minor comments, overall looks good!

cmonfortep · 2024-05-06T11:45:49Z

history/history-api/src/main/java/com/duckduckgo/history/api/HistoryRCWrapper.kt

+
+package com.duckduckgo.history.api
+
+interface HistoryRCWrapper {


Do we need this one in the :api module? Seems only used internally

cmonfortep · 2024-05-06T11:53:47Z

history/history-impl/src/main/java/com/duckduckgo/history/impl/HistoryRepository.kt

@@ -90,6 +95,14 @@ class RealHistoryRepository(
 }
 }

+ override fun isHistoryUserEnabled(default: Boolean): Boolean {


do we need default here? could that detail be moved into SharedPreferencesHistoryDataStore impl instead? I assume you need that value to change based on RC flag, if so maybe we can just have it as dependency there.

Yes, I need the value to change based on the RC flag (If the local value is null, then default to the RC flag value). The reason I added it there is because I'm considering handling that case as part of the business logic. Let me know what you think

history/history-impl/src/main/java/com/duckduckgo/history/impl/RealNavigationHistory.kt

cmonfortep · 2024-05-06T12:11:38Z

history/history-impl/src/main/java/com/duckduckgo/history/impl/RealNavigationHistory.kt

@@ -57,4 +62,16 @@ class RealNavigationHistory @Inject constructor(
 override suspend fun clearOldEntries() {
 historyRepository.clearEntriesOlderThan(currentTimeProvider.localDateTimeNow().minusDays(30))
 }
+
+ override fun isHistoryUserEnabled(): Boolean {


I was wondering if all these methods that are related to toggle should be part of HistoryFeature (aka. RealHistoryRCWrapper). So we have repository for storing and dealing with urls, and HistoryFeature for feature state (including user preference)

I don't think so, any feature dealing with history would need access to these methods, so the cleanest the API we give them, the better, IMO. Also, I think that would be a bit confusing, since, as I mentioned on my previous comment, we usually reserve the *Feature suffix for RC-related stuff. Also note that this method is related to user settings, not RC

history/history-impl/src/main/java/com/duckduckgo/history/impl/store/HistoryDataStore.kt

app/src/main/java/com/duckduckgo/app/privatesearch/PrivateSearchActivity.kt

app/src/main/java/com/duckduckgo/app/privatesearch/PrivateSearchViewModel.kt

cmonfortep · 2024-05-06T12:30:33Z

history/history-api/src/main/java/com/duckduckgo/history/api/NavigationHistory.kt

+ /**
+ * Returns whether the history RC flag is enabled.
+ */
+ fun isHistoryRCFlagEnabled(): Boolean


What about naming it as isHistoryUserVisible? just nit to name it based on what we expect rather than what we internally check.

I'd rather change it to isHistoryFeatureAvailable, userVisible sounds too close to the UI layer for an API method

cmonfortep

LGTM

Task/Issue URL: https://app.asana.com/0/0/1206816228247403/f ### Description Display an in-app message (above auto-complete results. See https://app.asana.com/0/0/1206816228247403/f for rules ### Steps to test this PR Update RC and set `storeHistory` to enabled. Alternatively, update `shouldStoreHistory` in `RealHistoryWrapper` so it returns true _Banner is not shown for new users_ - [ ] Do a clean install - [ ] Perform a search or visit a site - [ ] Repeat the same query again - [ ] Check history-based results are shown but no in-app message appears - [ ] Complete the onboarding - [ ] Repeat the same query again - [ ] Check history-based results are shown but no in-app message appears _Banner is shown for existing users_ - [ ] Update from a previous version of the app with completed onboarding, or update `isExistingUser` in `AutoComplete`so it returns true and do a clean install - [ ] Perform a search or visit a site - [ ] Repeat the same query again - [ ] Check history-based results are shown but and an in-app message appears _Banner isn't shown after being dismissed_ - [ ] Update `isExistingUser` in `AutoComplete`so it returns true and do a clean install - [ ] Perform a search or visit a site - [ ] Repeat the same query again - [ ] Check history-based results are shown but and an in-app message appears - [ ] Close the banner - [ ] Repeat the same query again - [ ] Check history-based results are shown but no in-app message appears _Banner is not shown when there are no history based-results results_ - [ ] Update `isExistingUser` in `AutoComplete`so it returns true and do a clean install - [ ] Visit wikipedia.org and add it to favorites - [ ] Use fire button - [ ] Type _wikipedia_ - [ ] Check no history-based results nor the banner are shown _Banner isn't shown more than 3 times_ Update `isExistingUser` in `AutoComplete`so it returns true and do a clean install - [ ] Perform a search or visit a site - [ ] Repeat the same query again - [ ] Check history-based results are shown but and an in-app message appears - [ ] Repeat previous 3 steps 4 times. Check the fourth time, the banner isn't shown anymore ### UI changes ![banner](https://github.com/duckduckgo/Android/assets/6297834/11f748d9-5131-42a8-a5bc-03330a585180)

CrisBarreiro mentioned this pull request May 2, 2024

Clear history entries from memory and database when clicking fire button #4450

Merged

6 tasks

CrisBarreiro mentioned this pull request May 2, 2024

Schedule deletion of items older than 30 days #4451

Merged

6 tasks

CrisBarreiro marked this pull request as ready for review May 2, 2024 12:40

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch 2 times, most recently from 132f86f to 28322ab Compare May 3, 2024 15:01

CrisBarreiro changed the base branch from feature/cbarreiro/autocomplete/clear-on-fire-button to feature/cbarreiro/autocomplete/delete-old-items May 3, 2024 15:03

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch 2 times, most recently from a311445 to 1c7f509 Compare May 6, 2024 10:15

cmonfortep reviewed May 6, 2024

View reviewed changes

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch from 1f369f9 to bdc61b8 Compare May 6, 2024 16:44

CrisBarreiro mentioned this pull request May 14, 2024

Add in app message for autocomplete #4545

Merged

27 tasks

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from 9cbb477 to c0a8b79 Compare May 15, 2024 14:09

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch from bdc61b8 to a59c675 Compare May 15, 2024 14:09

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from c0a8b79 to 8d3eaca Compare May 15, 2024 15:52

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch from 0f2046a to 5d17569 Compare May 15, 2024 15:52

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from 8d3eaca to a7e5ea0 Compare May 16, 2024 10:16

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch from 5d17569 to 997ab03 Compare May 16, 2024 10:16

This was referenced May 20, 2024

Pixel improvements #4559

Merged

Allow bookmarks to be shown in top hits #4572

Merged

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from a7e5ea0 to 381d765 Compare May 22, 2024 08:45

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch from 997ab03 to 2b54b39 Compare May 22, 2024 08:46

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from 381d765 to 6bf6500 Compare May 22, 2024 09:12

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch from 2b54b39 to 1e05eca Compare May 22, 2024 09:12

CrisBarreiro mentioned this pull request May 22, 2024

Design improvements #4574

Merged

3 tasks

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from 6bf6500 to 27d4ca2 Compare May 22, 2024 14:22

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch from 1e05eca to f63eb01 Compare May 22, 2024 14:22

CrisBarreiro assigned cmonfortep May 22, 2024

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from 27d4ca2 to da291cf Compare May 27, 2024 08:10

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch from f63eb01 to c0349ac Compare May 27, 2024 08:10

CrisBarreiro added 5 commits May 27, 2024 11:59

Add RC flag for storing history

c9265ac

Add setting for history in autocomplete

772e64a

Hide setting if RC flag is disabled

19b0ae8

Changes based on PR comments

051624f

Changes based on PR comments

0490131

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from da291cf to c7e0aa5 Compare May 27, 2024 10:00

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/rc-flag branch from c0349ac to 0490131 Compare May 27, 2024 10:00

CrisBarreiro mentioned this pull request May 28, 2024

Address ship review issues #4593

Merged

cmonfortep approved these changes May 29, 2024

View reviewed changes

CrisBarreiro merged commit fa6922a into feature/cbarreiro/autocomplete/delete-old-items May 29, 2024
4 checks passed

CrisBarreiro deleted the feature/cbarreiro/autocomplete/rc-flag branch May 29, 2024 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RC flag for storing history #4495

Add RC flag for storing history #4495

CrisBarreiro commented May 2, 2024 •

edited

Loading

CrisBarreiro commented May 2, 2024 •

edited by cmonfortep

Loading

cmonfortep left a comment

cmonfortep May 6, 2024

cmonfortep May 6, 2024

CrisBarreiro May 6, 2024

cmonfortep May 6, 2024

CrisBarreiro May 6, 2024

cmonfortep May 6, 2024

CrisBarreiro May 6, 2024 •

edited

Loading

cmonfortep left a comment


		package com.duckduckgo.history.api

		interface HistoryRCWrapper {

Add RC flag for storing history #4495

Add RC flag for storing history #4495

Conversation

CrisBarreiro commented May 2, 2024 • edited Loading

Description

Steps to test this PR

UI changes

CrisBarreiro commented May 2, 2024 • edited by cmonfortep Loading

cmonfortep left a comment

Choose a reason for hiding this comment

cmonfortep May 6, 2024

Choose a reason for hiding this comment

cmonfortep May 6, 2024

Choose a reason for hiding this comment

CrisBarreiro May 6, 2024

Choose a reason for hiding this comment

cmonfortep May 6, 2024

Choose a reason for hiding this comment

CrisBarreiro May 6, 2024

Choose a reason for hiding this comment

cmonfortep May 6, 2024

Choose a reason for hiding this comment

CrisBarreiro May 6, 2024 • edited Loading

Choose a reason for hiding this comment

cmonfortep left a comment

Choose a reason for hiding this comment

CrisBarreiro commented May 2, 2024 •

edited

Loading

CrisBarreiro commented May 2, 2024 •

edited by cmonfortep

Loading

CrisBarreiro May 6, 2024 •

edited

Loading