Skip to content
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.

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

Refactor updating user role to save local user asynchronously #20244

Merged
merged 3 commits into from
Mar 9, 2023

Conversation

crazytonyli
Copy link
Contributor

Similar to #20239, but this PR only changes the synchronous performAndSave to asynchronous one just in one place, since the diff is a bit messy. See pbArwn-5Qy-p2#how-does-this-apply-to-the-wp-and-jp-ios-apps for detailed reasoning.

Test instructions

Go to My Sites -> People, invite new user and edit their role. Make sure the role is updated correctly.

Regression Notes

  1. Potential unintended areas of impact
    None. As the refactored function is only used in the "People" screen.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    What described in the "Test instructions"

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

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.

@crazytonyli crazytonyli added the Core Data Issues related to Core Data label Mar 3, 2023
@crazytonyli crazytonyli added this to the 22.0 milestone Mar 3, 2023
@crazytonyli crazytonyli requested a review from mokagio March 3, 2023 08:16
@crazytonyli crazytonyli self-assigned this Mar 3, 2023
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 3, 2023

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

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

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 3, 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 pr20244-61e81f2 on your iPhone

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

Comment on lines +5 to +7
enum PeopleServiceError: Error {
case userNotFoundLocally(User)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of making this a nested type?

extension PeopleService {

  enum Error: Swift.Error {

Probably not worth it because then every existing Error usage in this file should be updated to Swift.Error to avoid clashing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's why I didn't put the new Error type under PeopleService.

Comment on lines +174 to +175
case let .failure(error):
failure?(error, user)
Copy link
Contributor

@mokagio mokagio Mar 8, 2023

Choose a reason for hiding this comment

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

Just to confirm I'm reading the diff correctly, this is a new behavior from the previous code and runs with error equal .userNotFoundLocally if managedPersonFromPerson(_:, in:) doesn't find a match.

In the previous version, instead, the app would have called crashed when failure?(error, reloadedPerson) because no value would have been assigned to var reloadedPerson: User! due to the early return.

Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The app wouldn't crash, but neither the success nor the failure block would be called. With this new behaviour, as you noted, the failures will be called if couldn't find the ManagedPerson.

However, I don't think this error would actually happen though, since this function is called immediately after the user changes the ManagerPerson instance's role on UI, it's not likely that the ManagedPerson is deleted in that split second.

@crazytonyli crazytonyli enabled auto-merge March 9, 2023 01:24
@crazytonyli crazytonyli merged commit 795e06a into trunk Mar 9, 2023
@crazytonyli crazytonyli deleted the refactor-people-service-update-user branch March 9, 2023 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Data Issues related to Core Data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants