Skip to content

Conversation

@hichamboushaba
Copy link
Member

Closes: #8178

Description

This PR fixes the above issue by making sure we re-open the MainActivity after the fetch if the user is ineligible.

Testing instructions

  1. Sign in to the app using a user that has a valid role.
  2. Close the app.
  3. Open the website then update the role to a role other than "admin" and "shop manager".
  4. Re-open the app.
  5. Confirm the app shows an error screen.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Comment on lines +239 to +240
private fun restartMainActivity() {
if (ProcessLifecycleOwner.get().lifecycle.currentState.isAtLeast(STARTED)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed the function to make it clear this would restart the activity, and I added the lifecycle's state check here instead of repeating it always, if it's called when the app is backgrounded, we don't want to pop the activity our of nowhere or the app to crash.

Comment on lines -61 to +59
doReturn(expectedUser.isUserEligible()).whenever(appPrefsWrapper).isUserEligible()
doReturn(expectedUser.email).whenever(appPrefsWrapper).getUserEmail()
whenever(userStore.fetchUserRole(any())).thenReturn(WooResult(expectedUser))

fetcher.fetchUserEligibility()
fetcher.fetchUserInfo()

assertThat(appPrefsWrapper.isUserEligible()).isEqualTo(expectedUser.isUserEligible())
assertThat(appPrefsWrapper.getUserEmail()).isEqualTo(expectedUser.email)
assertFalse(appPrefsWrapper.isUserEligible())
verify(appPrefsWrapper).setUserEmail(expectedUser.email)
verify(appPrefsWrapper).setIsUserEligible(expectedUser.isUserEligible())
Copy link
Member Author

Choose a reason for hiding this comment

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

This and the other test below didn't test what they say they test, as all what it was done is setting the mocks and then checking that they return what we set. I updated them to confirm the behavior against what userStore would return.

@hichamboushaba hichamboushaba marked this pull request as ready for review January 13, 2023 12:18
@hichamboushaba hichamboushaba added type: bug A confirmed bug. feature: login Related to any part of the log in or sign in flow, or authentication. labels Jan 13, 2023
Comment on lines +16 to 20
suspend fun fetchUserInfo(): WCUserModel? {
return userStore.fetchUserRole(selectedSite.get()).model?.also {
updateUserInfo(it)
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of having two functions, I opted to have a single suspendable one, and it automatically saves the data to the prefs, this way, the clients won't have to call updateUserInfo themselves.

@wpmobilebot
Copy link
Collaborator

You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code:

@JorgeMucientes JorgeMucientes self-assigned this Jan 13, 2023
@JorgeMucientes
Copy link
Contributor

Thanks for fixing this bug and making the code simpler and safer. Great job @hichamboushaba. Works perfect!

@JorgeMucientes JorgeMucientes merged commit 8cb661e into trunk Jan 13, 2023
@JorgeMucientes JorgeMucientes deleted the issue/8178-fix-user-role-fetch branch January 13, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: login Related to any part of the log in or sign in flow, or authentication. type: bug A confirmed bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

App doesn't display user eligibility error after role change from wc-core

4 participants