Schedule deletion of items older than 30 days #4451

CrisBarreiro · 2024-04-23T11:44:16Z

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

Description

See title

Steps to test this PR

Checkout this branch, go to HistoryDeletionWorker, and modify scheduleWorker adding setInitialDelay(5, TimeUnit.Minutes). Clear app data afterwards

Feature 1

Checkout this branch, go to HistoryDeletionWorker, and modify scheduleWorker adding setInitialDelay(5, TimeUnit.Minutes)
Go to RealNavigationHistory and modify historyRepository.clearEntriesOlderThan(currentTimeProvider.localDateTimeNow().minusDays(30)), changing minusDays(30) to minusMinutes (2)
Add a log so you know deletion is happening
Do a fresh install
Open app
Browse normally for a few minutes
Check that after ~5 minutes the log is shown and items older than 2 minutes are removed. Note that, if a site has several visits and some of them are more recent than the threshold for removal, only old visits will be removed, but both the entry entity and the recent visits will remain

UI changes

N/A

CrisBarreiro · 2024-04-23T11:44:28Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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 : 1 other dependent PR (#4341 )
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

thanks @CrisBarreiro , leaving you some comments here as well.

cmonfortep · 2024-05-06T14:52:29Z

...src/test/java/com/duckduckgo/history/impl/scheduleddeletion/RealHistoryDeletionWorkerTest.kt

+
+    @Test
+    fun whenDoWorkThenCallCleanOldEntriesAndReturnSuccess() {
+        appCoroutineScope.launch {


add dispatcher.io

Genuine question, does it matter for a test?

Missed it was a test 😅. doesn't matter then

cmonfortep · 2024-05-06T14:54:07Z

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

@@ -36,6 +36,8 @@ interface HistoryRepository {
    )

    suspend fun clearHistory()
+
+    suspend fun clearEntriesOlderThan(minusDays: LocalDateTime)


should we rename minusDays to something like time or timestamp?

cmonfortep · 2024-05-06T14:57:51Z

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

+    suspend fun deleteEntriesOlderThan(dateTime: LocalDateTime) {
+        val timestamp = DatabaseDateFormatter.timestamp(dateTime)
+
+        deleteOldVisitsByTimestamp(timestamp)


items deleted here are part of items that will be deleted in getEntriesWithNoVisitsOrOlderThanTimestamp?

Hm, I think you have a point and the second part of getEntriesWithNoVisitsOrOlderThanTimestamp is redundant. The idea is to first delete the visits with a timestamp older than the parameter, and then delete the entries that have no remaining visits

I've simplified all these logic, now it's only 2 delete clauses

cmonfortep · 2024-05-06T14:59:13Z

history/history-impl/src/test/java/com/duckduckgo/history/impl/store/HistoryDaoTest.kt

+    }
+
+    @Test
+    fun whenDeleteOldItemsWithNoOldEnoughItemsThenTheyAreDeleted() {


name should be ...withOld.. instead of ...WithNoOld...?

cmonfortep · 2024-05-06T15:06:28Z

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

 import com.duckduckgo.di.scopes.AppScope
 import com.duckduckgo.history.api.HistoryEntry
 import com.duckduckgo.history.api.NavigationHistory
 import com.squareup.anvil.annotations.ContributesBinding
 import io.reactivex.Single
 import javax.inject.Inject

-@ContributesBinding(AppScope::class)
+interface InternalNavigationHistory : NavigationHistory {


why we need this one here if you have HistoryRepository that seems to be your internal Navigation Repository?

RealNavigationHistory is what contains the business logic, whereas NavigationRepository is a bridge between RealNavigationHistory (logic) and the data sources. This internal interface is added so we don't expose externally the ability to schedule data deletion. I don't think it would make sense for the repository to define the time interval for data deletion

CrisBarreiro mentioned this pull request Apr 23, 2024

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

Open

6 tasks

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from 172b303 to c391a17 Compare April 23, 2024 13:07

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch 2 times, most recently from e113df0 to 689cfdd Compare April 23, 2024 13:35

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from c391a17 to 5ad8558 Compare April 24, 2024 11:47

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from 689cfdd to 851d3bd Compare April 24, 2024 11:47

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from 5ad8558 to 17efc3b Compare April 25, 2024 11:35

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from 851d3bd to f4c160e Compare April 25, 2024 11:35

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from 17efc3b to 34610cf Compare April 25, 2024 11:36

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from f4c160e to e86e3ed Compare April 25, 2024 11:36

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from 34610cf to 1179474 Compare April 25, 2024 11:48

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from e86e3ed to 23c2fee Compare April 25, 2024 11:48

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from 1179474 to 137874e Compare April 25, 2024 14:35

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from 23c2fee to 00576a1 Compare April 25, 2024 14:35

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from 137874e to 28eed47 Compare April 25, 2024 15:01

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch 2 times, most recently from 2ecd8e8 to daa8627 Compare April 25, 2024 15:16

CrisBarreiro marked this pull request as ready for review April 25, 2024 15:16

CrisBarreiro added the STACKED PR - DO NOT MERGE label Apr 25, 2024

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from 28eed47 to f43ecc1 Compare April 29, 2024 12:36

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from daa8627 to 9a51eac Compare April 29, 2024 12:36

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from f43ecc1 to 5418fd5 Compare April 29, 2024 14:30

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from 9a51eac to c2e3c41 Compare April 29, 2024 14:30

CrisBarreiro requested a review from cmonfortep April 30, 2024 13:37

CrisBarreiro assigned cmonfortep Apr 30, 2024

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from 5418fd5 to 5a21ae9 Compare April 30, 2024 16:56

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from c2e3c41 to 2a8e5bc Compare April 30, 2024 16:56

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from 5a21ae9 to 67be27a Compare May 2, 2024 09:16

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/delete-old-items branch from 2a8e5bc to 4e9c30b Compare May 2, 2024 09:16

CrisBarreiro mentioned this pull request May 2, 2024

Add RC flag for storing history #4495

Open

16 tasks

cmonfortep reviewed May 6, 2024

View reviewed changes

CrisBarreiro mentioned this pull request May 14, 2024

Add in app message for autocomplete #4545

Draft

27 tasks

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from 67be27a to 8a2b4ad Compare May 15, 2024 14:09

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/clear-on-fire-button branch from 8a2b4ad to e8d4f44 Compare May 15, 2024 15:52

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

CrisBarreiro added 2 commits May 16, 2024 12:14

Schedule deletion of items older than 30 days

cfa48c2

Address PR comments

a7e5ea0

CrisBarreiro force-pushed the feature/cbarreiro/autocomplete/clear-on-fire-button branch from e8d4f44 to 6831b27 Compare May 16, 2024 10:16

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schedule deletion of items older than 30 days #4451

Schedule deletion of items older than 30 days #4451

CrisBarreiro commented Apr 23, 2024 •

edited

CrisBarreiro commented Apr 23, 2024 •

edited

cmonfortep left a comment

cmonfortep May 6, 2024

CrisBarreiro May 6, 2024

cmonfortep May 6, 2024

cmonfortep May 6, 2024

cmonfortep May 6, 2024

CrisBarreiro May 6, 2024

CrisBarreiro May 6, 2024

cmonfortep May 6, 2024

cmonfortep May 6, 2024

CrisBarreiro May 6, 2024

Schedule deletion of items older than 30 days #4451

Are you sure you want to change the base?

Schedule deletion of items older than 30 days #4451

Conversation

CrisBarreiro commented Apr 23, 2024 • edited

Description

Steps to test this PR

UI changes

CrisBarreiro commented Apr 23, 2024 • edited

cmonfortep left a 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

CrisBarreiro commented Apr 23, 2024 •

edited

CrisBarreiro commented Apr 23, 2024 •

edited