Skip to content

Fix database migration issue with different database models #20214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Fix database migration issue with different database models #20214

wants to merge 3 commits into from

Conversation

@wargcm
Copy link
Contributor

@wargcm wargcm commented Feb 24, 2023 •
edited
Loading

Partially fixes #20207

Description

Note
This does not fix the migration issue with 21.8 WordPress -> 21.7 Jetpack. It only fixes 21.7 WordPress -> 21.8 Jetpack. To fix both scenarios, these changes would need to be applied to the 21.7 branch.

Testing

To test:

WP -> JP Migration fix

  • Checkout the release/21.7.1 branch
  • Install WordPress and login
  • Begin the migration with any of the Jetpack powered banner entry points
  • Checkout this branch issue/20207-fix-model-jp-migration
  • Install Jetpack and launch
  • Verify your data was migrated successfully

App upgrade

  • Checkout the release/21.7.1 branch
  • Install WordPress and login
  • Install Jetpack and login
  • Checkout this branch issue/20207-fix-model-jp-migration
  • Install WordPress and launch
  • Verify you are still logged in with your data
  • Install Jetpack and launch
  • Verify you are still logged in with your data

Regression Notes

  1. Potential unintended areas of impact
    Potentially with the existing manual model migration functions

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manually testing the app upgrade scenario and comparing code to older working code

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@wargcm wargcm added this to the 21.8 ❄️ milestone Feb 24, 2023
@wargcm wargcm requested a review from dvdchr February 24, 2023 21:19
@wargcm wargcm self-assigned this Feb 24, 2023
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Feb 24, 2023

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 24, 2023

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr20214-6c833d6 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@dvdchr dvdchr requested a review from crazytonyli February 27, 2023 06:52
crazytonyli
crazytonyli reviewed Feb 27, 2023
View reviewed changes
WordPress/Classes/Utility/ContextManager.swift
}

guard let metadata = try? NSPersistentStoreCoordinator.metadataForPersistentStore(ofType: NSSQLiteStoreType, at: storeURL),
!objectModel.isConfiguration(withName: nil, compatibleWithStoreMetadata: metadata)
Copy link
Contributor

@crazytonyli crazytonyli Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🤦‍♂️ I mis-translated this line in #19770. Thanks for fixing this up. But I'm surprised the issue wasn't caught by the migration unit tests 🤔

Copy link
Contributor

@dvdchr dvdchr Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, thank you for the quick response @crazytonyli ! I was just about to ping you to confirm if this change was intended.

@mokagio mokagio mentioned this pull request Feb 27, 2023
Merged
4 tasks
@mokagio
Copy link
Contributor

mokagio commented Feb 27, 2023

Closing in favor of #20217.

This fix will reach 21.8 once the 21.7.2 hotfix is submitted to the App Store and it's finalized branched merged into release/21.8. For an example, see #20206.

@mokagio mokagio closed this Feb 27, 2023
@hypest
Copy link
Contributor

hypest commented Mar 6, 2023

Closing in favor of #20217.

👋 @mokagio , can you elaborate on the need for a separate PR? Thanks!

@mokagio
Copy link
Contributor

mokagio commented Mar 7, 2023

👋👋 @hypest

This PR was targeting the release/21.8 branch which would have resulted in the fix being part of 21.8. But we wanted to ship the fix ASAP via a dedicated hotfix. To do so, the PR should have targeted release/21.7.2. In this case, updating the base branch was not an option because issue/20207-fix-model-jp-migration was created started from a commit in release/21.8 and doing so would have brought into release/21.7.2 all the commits from release/21.8. Cherry picking only the relevant commits in a new branch created from release/21.7.2 (#20217) was the cleanest option (that I could think of at the time) and one we've been using in other occasions, such as #20206.

@mokagio mokagio deleted the issue/20207-fix-model-jp-migration branch March 7, 2023 01:19
@hypest
Copy link
Contributor

hypest commented Mar 7, 2023

Thanks for the information @mokagio , makes sense to me too 👍

I wonder if we can also try to keep the attribution as close as (practically) possible to what happened. For example, I think I'd wish to see this fix and PR created by Chris to be merged and see its way to production. I know of course that the commits are keeping their attribution when cherry-picked, but I wonder if we can opt for PR-level attribution too.

I cannot be fully sure about the practicality of the following: to rebase the base branch onto whichever is the desired one, and force push. Can that keep the work in the single PR? No strong feelings.

@mokagio
Copy link
Contributor

mokagio commented Mar 7, 2023

You raise an interesting point on attribution 🤔

I cannot be fully sure about the practicality of the following: to rebase the base branch onto whichever is the desired one, and force push. Can that keep the work in the single PR? No strong feelings.

I can see this working, yes. The only downside I can think of is that a rebase rewrites history so anyone who wishes to interact with the branch afterwards would have to reset. But... I don't think it's a big deal in the case of a hotfix, because the PR would be most likely merged shortly after.

@mokagio
Copy link
Contributor

mokagio commented Mar 8, 2023

@hypest luck would have it (if we want to call it luck) that I had a chance to try the approach you suggested, successfully, just now.

I reset #20275, originally targeting 21.9, on top of 21.8.1 and cherry-picked the original changes. Now attribution is preserved at the commit and PR level. 👍

@mokagio mokagio mentioned this pull request Mar 8, 2023
Merged
4 tasks
@hypest
Copy link
Contributor

hypest commented Mar 8, 2023

I reset #20275, originally targeting 21.9, on top of 21.8.1 and cherry-picked the original changes. Now attribution is preserved at the commit and PR level. 👍

Nice, thanks for giving it a try @mokagio ! As I mentioned earlier, no strong feelings from my side so, if we see signs of adverse effects let's switch to the previous practice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crazytonyli crazytonyli crazytonyli left review comments

@dvdchr dvdchr Awaiting requested review from dvdchr

Assignees

@wargcm wargcm

Labels

Jetpack Content Migration Flow [Type] Bug

Projects

None yet

Milestone

21.8

Development

Successfully merging this pull request may close these issues.

7 participants

@wargcm @wpmobilebot @mokagio @hypest @crazytonyli @dvdchr