Bug 2024810 - Fixed last saved folder cache updating

[fmasalha]

No description provided.

[github-actions]

View this pull request in Lando to land it once approved.

[fmasalha]

The scope was changed to be a cancel able scope in February which made the following code not run since exitBookmarks would destroy the fragment which cancels the lifecycleScope causing the last saved pref not to get updated.

[fmasalha]

You can add a bookmark star to the toolbar which didnt have the previously added safeguard to the menudialog.

[segunfamisa]

Thanks for this PR. I added a comment about something I think we should fix once-and for all, but also okay with having this as a follow-up

[segunfamisa]

To be honest, I think that we should fix this once and for-all, and make our AddBookmarksUseCase do all of this dance.

This is pretty much duplicated code from the MenuDialogMiddleware, doing the same thing.

firefox/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/components/menu/middleware/MenuDialogMiddleware.kt

Lines 265 to 280 in 662d186

	// get the last saved folder id
	val targetParentFolderId = lastSavedFolderCache.getGuid() ?: BookmarkRoot.Mobile.id
	// get the corresponding bookmark and fallback to mobile root bookmark node
	// this is necessary because it's possible that the last saved folder no longer exists (
	// e.g. if the folder is removed through sync)
	val parentNode = bookmarksStorage.getBookmark(targetParentFolderId).getOrNull()
	?: bookmarksStorage.getBookmark(BookmarkRoot.Mobile.id).getOrNull()
	val parentGuid = parentNode?.guid ?: BookmarkRoot.Mobile.id
	val guidToEdit = addBookmarkUseCase(
	url = url,
	title = selectedTab.content.title,
	parentGuid = parentGuid,
	)

We discussed this in January and we all kinda agreed that we need to unify this functionality. I think this is a good opportunity to do so.

I also see that you clear the last saved folder cache if for some reason, the resolved parentGuid is not matching the last saved parent folder. That's a good improvement

So, instead of duplicating this logic, we could consider the following:

1. Move LastSavedFolderCache somewhere in our bookmarks package or something
2. Add it as a parameter to our AddBookmarksUsecase
3. Move this logic into the use-case.
4. Remove similar duplication that we do in various places
5. Update tests.

I will say that we could land this to do this properly now, but that's what we said the last time 😂

Maybe @MatthewTighe & @boek can weight in here.

[fmasalha]

I agree, I think this is a larger amount of work so I filed a follow up for this here https://bugzilla.mozilla.org/show_bug.cgi?id=2032600

[fmasalha]

testing-tag explanation

The follow up work will streamline this approach to make it testable

https://bugzilla.mozilla.org/show_bug.cgi?id=2032600

[lando-worker]

Pull request closed by commit 5b47c78