Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #3327: Remove stale sync v1 code, rename old 'Bookmarks' to 'Favorites' #3268

Merged
merged 11 commits into from
Feb 18, 2021

Conversation

iccub
Copy link
Contributor

@iccub iccub commented Feb 5, 2021

Summary of Changes

This pull request fixes #3327

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

3 Basic tests would be nice to have, they don't have to be really thorough

  1. Test that sync is working
  2. Test that migration from pre sync-v2 is working, can be basic test just to verify that old bookmarks got migrated correctly.
  3. Test that non-sync bookmarks and favorites behavior works: add, create, update, delete, reorder

Screenshots:

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).

@iccub iccub requested a review from Brandon-T February 11, 2021 17:13
@iccub iccub marked this pull request as ready for review February 11, 2021 17:13
@iccub iccub requested a review from a team February 11, 2021 17:13
@iccub iccub changed the title Nobug/remove sync v1 No Bug: Remove stale sync v1 code, rename old 'Bookmarks' to 'Favorites' Feb 15, 2021
@iccub iccub added this to the 1.24 milestone Feb 15, 2021
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

So just to clarify:

Bookmark is still the entity in CoreData, but its NSManagedObject subclass is named Favourite, and a bunch of the old Chromium bookmark migration stuff was moved to LegacyBookmark?

@iccub
Copy link
Contributor Author

iccub commented Feb 17, 2021

Yes @kylehickinson

  1. Bookmark renamed to Favorite, no migration required, but we can migrate if we want to, removed unused properties etc
  2. Methods that still rely on old Bookmark code were moved to LegacyBookmark, which is the same class as Favorite, added for readability reasons. At the moment only chromium migration and older 1.12, 1.7 migration/restoration code still uses core-data bookmarks.
  3. Most notable change in Bookmark/Favorite code is going back to using order instead of syncOrder, besides that most of the work is removing unused code or unused properties from code if possible, for fields like isFolder isFavorite parentFolder

@iccub iccub changed the title No Bug: Remove stale sync v1 code, rename old 'Bookmarks' to 'Favorites' Fix #3327: Remove stale sync v1 code, rename old 'Bookmarks' to 'Favorites' Feb 18, 2021
@iccub iccub merged commit 9afb1f3 into development Feb 18, 2021
@iccub iccub deleted the nobug/remove_sync_v1 branch February 18, 2021 21:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove stale sync v1 code.
3 participants