[REST API] Check user role #8156

hichamboushaba · 2023-01-10T16:00:26Z

Part of: #8110

Description

This PR adds user role fetching logic during the site credentials login.
While working on it, I found out that the Woo detection logic we have only works for users who have a valid user role, which prevents showing the correct error message.
So I updated the logic in wordpress-mobile/WordPress-FluxC-Android#2628, please check it before this PR.

Testing instructions

Login

Create a user with a role other than "admin" and "shop manager".
Open the app, then sign using site credentials with the new user.
Notice that after the login, the app shows an error screen about the user role.

App Startup

Finish login using a user that has a valid role.
Close the app.
Open the website then update the role to a role other than "admin" and "shop manager".
Re-open the app.
Confirm the app shows an error screen.

Images/gif

device-2023-01-10-173546.mp4

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

wpmobilebot · 2023-01-10T16:18:51Z

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

codecov-commenter · 2023-01-10T16:56:03Z

Codecov Report

Base: 42.92% // Head: 42.91% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (186808f) compared to base (ae19f4b).
Patch coverage: 15.62% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk    #8156      +/-   ##
============================================
- Coverage     42.92%   42.91%   -0.02%     
+ Complexity     3484     3483       -1     
============================================
  Files           686      686              
  Lines         37340    37344       +4     
  Branches       4960     4964       +4     
============================================
- Hits          16030    16026       -4     
- Misses        19864    19873       +9     
+ Partials       1446     1445       -1

Impacted Files	Coverage Δ
...n/kotlin/com/woocommerce/android/AppInitializer.kt	`0.00% <0.00%> (ø)`
.../main/kotlin/com/woocommerce/android/model/User.kt	`80.00% <0.00%> (ø)`
...n/sitecredentials/LoginSiteCredentialsViewModel.kt	`0.00% <0.00%> (ø)`
...android/ui/common/UserEligibilityErrorViewModel.kt	`89.18% <83.33%> (+13.67%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

wpmobilebot · 2023-01-12T12:47:18Z

Found 1 violations:

The PR caused the following dependency changes:

expand

-+--- org.wordpress:fluxc:2.10.0
-|    +--- org.wordpress:wellsql:1.7.0
-|    |    \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
-|    +--- org.wordpress.fluxc:fluxc-annotations:2.10.0
-|    +--- org.greenrobot:eventbus:3.3.1 (*)
-|    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.3 (*)
-|    +--- com.android.volley:volley:1.1.1 -> 1.2.0
-|    +--- androidx.paging:paging-runtime:2.1.2
-|    |    +--- androidx.paging:paging-common:2.1.2
-|    |    |    +--- androidx.annotation:annotation:1.0.0 -> 1.5.0 (*)
-|    |    |    \--- androidx.arch.core:core-common:2.0.0 -> 2.1.0 (*)
-|    |    +--- androidx.arch.core:core-runtime:2.0.0 -> 2.1.0 (*)
-|    |    +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.5.1 (*)
-|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.5.1 (*)
-|    |    \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.2.1 (*)
-|    +--- com.goterl:lazysodium-android:5.0.2
-|    +--- net.java.dev.jna:jna:5.5.0
-|    +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
-|    +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.10 -> 1.7.20
-|    |    \--- org.jetbrains.kotlin:kotlin-stdlib:1.7.20 (*)
-|    +--- androidx.appcompat:appcompat:1.0.2 -> 1.5.0 (*)
-|    +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.2.1 (*)
-|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.3
-|    |    \--- androidx.annotation:annotation:1.2.0 -> 1.5.0 (*)
-|    +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
-|    |    +--- androidx.annotation:annotation:1.1.0 -> 1.5.0 (*)
-|    |    +--- com.google.crypto.tink:tink-android:1.5.0
-|    |    \--- androidx.collection:collection:1.1.0 -> 1.2.0 (*)
-|    +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0
-|    |    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.3 (*)
-|    |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.7.20 (*)
-|    +--- com.google.code.gson:gson:2.8.5 -> 2.9.0
-|    +--- org.apache.commons:commons-text:1.10.0 (*)
-|    +--- androidx.room:room-runtime:2.4.2 (*)
-|    +--- androidx.room:room-ktx:2.4.2
-|    |    +--- androidx.room:room-common:2.4.2 (*)
-|    |    +--- androidx.room:room-runtime:2.4.2 (*)
-|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 -> 1.7.20 (*)
-|    |    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 -> 1.6.4 (*)
-|    +--- com.google.dagger:dagger:2.42
-|    |    \--- javax.inject:javax.inject:1
-|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.6.4 (*)
-|    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.6.4 (*)
++--- org.wordpress:fluxc:trunk-463c753ae48797fd0543f237f668b322345eb69b
+|    +--- org.wordpress:wellsql:1.7.0
+|    |    \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
+|    +--- org.wordpress.fluxc:fluxc-annotations:trunk-463c753ae48797fd0543f237f668b322345eb69b
+|    +--- org.greenrobot:eventbus:3.3.1 (*)
+|    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.3 (*)
+|    +--- com.android.volley:volley:1.1.1 -> 1.2.0
+|    +--- androidx.paging:paging-runtime:2.1.2
+|    |    +--- androidx.paging:paging-common:2.1.2
+|    |    |    +--- androidx.annotation:annotation:1.0.0 -> 1.5.0 (*)
+|    |    |    \--- androidx.arch.core:core-common:2.0.0 -> 2.1.0 (*)
+|    |    +--- androidx.arch.core:core-runtime:2.0.0 -> 2.1.0 (*)
+|    |    +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.5.1 (*)
+|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.5.1 (*)
+|    |    \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.2.1 (*)
+|    +--- com.goterl:lazysodium-android:5.0.2
+|    +--- net.java.dev.jna:jna:5.5.0
+|    +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
+|    +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.10 -> 1.7.20
+|    |    \--- org.jetbrains.kotlin:kotlin-stdlib:1.7.20 (*)
+|    +--- androidx.appcompat:appcompat:1.0.2 -> 1.5.0 (*)
+|    +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.2.1 (*)
+|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.3
+|    |    \--- androidx.annotation:annotation:1.2.0 -> 1.5.0 (*)
+|    +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
+|    |    +--- androidx.annotation:annotation:1.1.0 -> 1.5.0 (*)
+|    |    +--- com.google.crypto.tink:tink-android:1.5.0
+|    |    \--- androidx.collection:collection:1.1.0 -> 1.2.0 (*)
+|    +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0
+|    |    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.3 (*)
+|    |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.7.20 (*)
+|    +--- com.google.code.gson:gson:2.8.5 -> 2.9.0
+|    +--- org.apache.commons:commons-text:1.10.0 (*)
+|    +--- androidx.room:room-runtime:2.4.2 (*)
+|    +--- androidx.room:room-ktx:2.4.2
+|    |    +--- androidx.room:room-common:2.4.2 (*)
+|    |    +--- androidx.room:room-runtime:2.4.2 (*)
+|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 -> 1.7.20 (*)
+|    |    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 -> 1.6.4 (*)
+|    +--- com.google.dagger:dagger:2.42
+|    |    \--- javax.inject:javax.inject:1
+|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.6.4 (*)
+|    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.6.4 (*)
-\--- org.wordpress.fluxc.plugins:woocommerce:2.10.0
-     +--- org.wordpress:wellsql:1.7.0 (*)
-     +--- org.wordpress.fluxc:fluxc-annotations:2.10.0
-     +--- androidx.room:room-ktx:2.4.2 (*)
-     +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
-     +--- org.wordpress:fluxc:2.10.0 (*)
-     +--- com.google.code.gson:gson:2.8.5 -> 2.9.0
-     +--- com.google.dagger:dagger:2.42 (*)
-     +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.6.4 (*)
-     +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.6.4 (*)
-     \--- androidx.room:room-runtime:2.4.2 (*)
+\--- org.wordpress.fluxc.plugins:woocommerce:trunk-463c753ae48797fd0543f237f668b322345eb69b
+     +--- org.wordpress:wellsql:1.7.0 (*)
+     +--- org.wordpress.fluxc:fluxc-annotations:trunk-463c753ae48797fd0543f237f668b322345eb69b
+     +--- androidx.room:room-ktx:2.4.2 (*)
+     +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
+     +--- org.wordpress:fluxc:trunk-463c753ae48797fd0543f237f668b322345eb69b (*)
+     +--- com.google.code.gson:gson:2.8.5 -> 2.9.0
+     +--- com.google.dagger:dagger:2.42 (*)
+     +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.6.4 (*)
+     +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.6.4 (*)
+     \--- androidx.room:room-runtime:2.4.2 (*)

Please review and act accordingly

peril-woocommerce · 2023-01-12T15:01:48Z

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

JorgeMucientes · 2023-01-12T17:39:04Z

Hey @hichamboushaba. Changes look good! But I noticed an issue when testing this scenario:

App Startup
Finish login using a user that has a valid role.
Close the app.
Open the website then update the role to a role other than "admin" and "shop manager".
Re-open the app.
Confirm the app shows an error screen.

On app startup the app won't show the user role error screen. At least it won't show it the first time. I have to kill it twice and reopen it again to see the error. Check the screen recording:

UserRoleUpdate.mp4

When I reopen the app for the first time after switching user role, I see several expected 403 errors (which should
[3750] NetworkUtility.shouldRetryException: Unexpected response code 403 for https://mucitest.mystagingwebsite.com/wp-json/wc/v3/orders/?per_page=1&offset=0&status=any

At first, I thought it was a problem with the backend taking some time to update the user role, but I could reproduce this consistently, leaving plenty of time for the backend to update the user role (which has been very fast in all my test). Feels like some event from the eventBus is not triggered.

Can you reproduce this? I'm testing on an emulator API 30 (I don't think is API related though)

hichamboushaba · 2023-01-13T10:05:33Z

Thanks @JorgeMucientes for the review.

On app startup the app won't show the user role error screen. At least it won't show it the first time. I have to kill it twice and reopen it again to see the error. Check the screen recording:

This part has been there for WordPress.com login too, the only change I did is remove the token presence condition.
I suspect the cause is that the app is kept in memory even after clearing it from the recent tasks (the system sometimes decide to do so), and since the task won't run until the app is completely restarted, this will prevent it from running.
Can you please make sure the app is cleared from memory (go to app's settings, and click on "Force Close"), then retest?

JorgeMucientes · 2023-01-13T11:19:42Z

Hey @hichamboushaba sorry for generating unnecessary noise in the PR. The bug is unrelated to the code changes. So, I won't block the PR for this. Just wanted to highlight that one of the scenarios described in the PR was not working as expected. The reason is a long-standing bug that I've described in this new issue #8178. Will take a look at it and see if there's a straightforward way to fix it.

hichamboushaba · 2023-01-13T11:40:43Z

Hey @hichamboushaba sorry for generating unnecessary noise in the PR.

Oh, no worries, it's good actually you caught a bug, the bug is clear, and it seems a race condition is what's needed to either have it occurring or not, that's why it didn't happen for me while it happened for you, I have an idea for a quick fix, unless you already started working on a fix yourself.

JorgeMucientes · 2023-01-13T11:45:10Z

Please go ahead with the quick fix @hichamboushaba! I haven't started working on it. Feel free to assign the issue #8178 to yourself 🙏🏼

hichamboushaba added type: enhancement A request for an enhancement. status: do not merge Dependent on another PR, ready for review but not ready for merge. feature: REST API Support connecting to sites using WordPress REST API. labels Jan 10, 2023

hichamboushaba added 5 commits January 10, 2023 17:01

Bump fluxc

6737afa

Add logic to fetch user info during site credentials login

eef978e

Fix logic of choosing the name to display

0c39055

Simplify logic

c15c09d

Refactor logic to fetch sites and user info for site credentials login

eda59a3

hichamboushaba force-pushed the issue/8110-user-role branch from 88bcf31 to eda59a3 Compare January 10, 2023 16:02

hichamboushaba mentioned this pull request Jan 10, 2023

Woo REST API: fix Woo detection for non-eligible users wordpress-mobile/WordPress-FluxC-Android#2628

Merged

hichamboushaba added 2 commits January 10, 2023 17:22

Simplify loading changes logic

8483b2a

Use the common logout logic in UserEligibilityErrorViewModel

5985ed4

Fix unit tests

7dad4d1

hichamboushaba force-pushed the issue/8110-user-role branch from 9c9fd4c to 7dad4d1 Compare January 10, 2023 17:08

hichamboushaba marked this pull request as ready for review January 12, 2023 08:45

hichamboushaba requested review from 0nko, JorgeMucientes and hafizrahman January 12, 2023 08:45

This was referenced Jan 12, 2023

[REST API] Site Credentials login flow #8110

Closed

[REST API] unit tests for site credentials #8171

Merged

Bump fluxc

186808f

hichamboushaba removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Jan 12, 2023

JorgeMucientes self-assigned this Jan 12, 2023

JorgeMucientes approved these changes Jan 13, 2023

View reviewed changes

JorgeMucientes merged commit bad1d31 into trunk Jan 13, 2023

JorgeMucientes deleted the issue/8110-user-role branch January 13, 2023 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[REST API] Check user role #8156

[REST API] Check user role #8156

Uh oh!

hichamboushaba commented Jan 10, 2023 •

edited

Loading

Uh oh!

wpmobilebot commented Jan 10, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented Jan 10, 2023 •

edited

Loading

Uh oh!

wpmobilebot commented Jan 12, 2023

Uh oh!

peril-woocommerce bot commented Jan 12, 2023

Uh oh!

JorgeMucientes commented Jan 12, 2023

Uh oh!

hichamboushaba commented Jan 13, 2023

Uh oh!

JorgeMucientes commented Jan 13, 2023

Uh oh!

hichamboushaba commented Jan 13, 2023

Uh oh!

JorgeMucientes commented Jan 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[REST API] Check user role #8156

[REST API] Check user role #8156

Uh oh!

Conversation

hichamboushaba commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Login

App Startup

Images/gif

Uh oh!

wpmobilebot commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wpmobilebot commented Jan 12, 2023

The PR caused the following dependency changes:

Uh oh!

peril-woocommerce bot commented Jan 12, 2023

Uh oh!

JorgeMucientes commented Jan 12, 2023

Uh oh!

hichamboushaba commented Jan 13, 2023

Uh oh!

JorgeMucientes commented Jan 13, 2023

Uh oh!

hichamboushaba commented Jan 13, 2023

Uh oh!

JorgeMucientes commented Jan 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hichamboushaba commented Jan 10, 2023 •

edited

Loading

wpmobilebot commented Jan 10, 2023 •

edited

Loading

codecov-commenter commented Jan 10, 2023 •

edited

Loading