Refactor the WorkspaceSettingsPage #6033

mountiny · 2021-10-25T13:54:22Z

Details

This PR refactors some bad smells:

It moves API actions from WorkspaceSettingsPage.js to actions/Policy.js
The actions do not return Promise anymore as this is not required in this case
Use Onyx as source of truth

Additionally, I have added a small change in the flow (which we can discuss if we want to keep) which is the avatar being saved in the policy when it is uploaded. Currently, user needs to upload the avatar, wait for it to load and then click Save to actually save the avatar into the policy.

After this refactoring, the avatar flow is independent on the name/currency flow, so the new picture is saved to our database once it is uploaded.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/180546

Tests

This is a refactoring of the Workspace settings editor. We need to make sure uploading a new avatar for workspace or changing name/currency still works as expected.

Go to your workspace's General settings.
Click on the avatar and pick some image to use as avatar and upload it.
Success Growl message should show indicating the upload has been successful.
Exit the right hand side panel, reload the page (potentially even clearing your cache) and make sure the workspace has the new picture as its avatar.
Go back to the workspace settings and change name of the workspace. Click Save.
Exit the setting page, reload the page and make sure the name of the settings is changed.
Lastly, go back to the settings page, click on the Avatar and remove it. Make sure it is removed and the placeholder image is shown (even after reloading).

QA Steps

Same as tests above.

Tested On

Screenshots

Web

Screen.Recording.2021-10-25.at.19.28.09.mov

Mobile Web

Desktop

iOS

Android

… from ONYX

… failed

mountiny

This is ready for a review! Thank you very much for your time reviewing this.

I prefer this flow when the avatar image is saved immediately and users does not have to click the Save button in order to save the image. I think this is better UX as will more likely be times when users adds the avatar and notices it being uploaded and exits the settings without saving just to be confused that the image has actually not been saved.

On the other hand, I cannot imagine situation when users would be confused that the picture has been uploaded before they clicked Save.

However, I am open for discussion and happy to change this if you will think this approach is worse.

mountiny · 2021-10-25T18:58:14Z

src/libs/actions/Policy.js

+        .then((avatarURL) => {
+            // Update the policy with the new avatarURL as soon as we get it
+            Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {avatarURL, isAvatarUploading: false});
+            update(policyID, {avatarURL}, true);


I am now updating the policy immediately when we get the s3bucket url so the avatar is saved straight away.

I love this! I think this is a much better UX as well. When I first tested out this UI I thought it was a bug that the avatar wasn't saving on upload.

Perfect, I felt the same!

mountiny · 2021-10-25T19:04:44Z

src/libs/actions/Policy.js

+        .then((response) => {
+            if (response.jsonCode !== 200) {
+                // Show the user error feedback
+                throw new Error();


Since I am now chaining two .then() together, I could not just simply return here as before (that would be supplied to the next then statement), so I have decided to throw new Error, which will be then caught in the .catch().

I was considering this... and I think it's mostly OK, except I don't really like that there are multiple .then()s being chained together. This makes the code harder to follow and more complex than it needs to be. I think this can all be rewritten like this and it's more simple:

API.User_UploadAvatar({file}) .then((response) => { if (response.jsonCode === 200) { // Update the policy with the new avatarURL as soon as we get it Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {avatarURL: response.s3url, isAvatarUploading: false}); update(policyID, {avatarURL: response.s3url}, true); return; } Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {isAvatarUploading: false}); const errorMessage = translateLocal('workspace.editor.avatarUploadFailureMessage'); Growl.error(errorMessage, 5000); });

The only change this makes is that there is no more catch() so there won't be any HTTP errors caught either (but I'm not sure we really want to be catching those here anyway, and most API calls don't catch those in the action layer).

Oh, I really like this approach, why have I not seen it before 😅 Thanks for bringing this up, it is way clearer!

src/pages/workspace/WorkspaceSettingsPage.js

parasharrajat · 2021-10-25T20:50:55Z

Please hold, one of my PR is going to be merged soon #5271 which handles multiple issues. It's on hold since N6.

mountiny · 2021-10-25T20:59:56Z

@parasharrajat Thank you for heads up! I will need to resolve the merge conflicts first then. I guess this can wait for a review :)

parasharrajat · 2021-10-25T21:01:01Z

@mountiny Sorry, it is just merged. 🤣

tgolen

This looks much better, Vit! Thank you.

tgolen · 2021-10-25T21:43:36Z

src/pages/workspace/WorkspaceSettingsPage.js

-        })).catch(() => {
-            Growl.error(this.props.translate('workspace.editor.avatarUploadFailureMessage'));
-        }).finally(() => Policy.updateLocalPolicyValues(this.props.policy.id, {isAvatarUploading: false}));
+        Policy.updateLocalPolicyValues(this.props.policy.id, {isAvatarUploading: true});


It looks like the three instances we have of Policy.updateLocalPolicyValues() are all immediately before calling something that updates the policy. I think that this is confusing and also error-prone. What I would suggest is see about making Policy.updateLocalPolicyValues() a local method that is not exported, and then just reference it internally inside the action (inside of Policy.update() and Policy.uploadAvatar()).

I think that keeping it internal will make the view code more simple (only have to call one method) and less error prone (don't have to worry about forgetting to call one of the methods).

That is also a good point and I agree that it will be clearer if we keep it in the Policy actions. I have refactored it accordingly :)

tgolen · 2021-10-25T21:48:36Z

src/libs/actions/Policy.js

+        .then((response) => {
+            if (response.jsonCode !== 200) {
+                // Show the user error feedback
+                throw new Error();


I was considering this... and I think it's mostly OK, except I don't really like that there are multiple .then()s being chained together. This makes the code harder to follow and more complex than it needs to be. I think this can all be rewritten like this and it's more simple:

API.User_UploadAvatar({file}) .then((response) => { if (response.jsonCode === 200) { // Update the policy with the new avatarURL as soon as we get it Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {avatarURL: response.s3url, isAvatarUploading: false}); update(policyID, {avatarURL: response.s3url}, true); return; } Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {isAvatarUploading: false}); const errorMessage = translateLocal('workspace.editor.avatarUploadFailureMessage'); Growl.error(errorMessage, 5000); });

The only change this makes is that there is no more catch() so there won't be any HTTP errors caught either (but I'm not sure we really want to be catching those here anyway, and most API calls don't catch those in the action layer).

mountiny · 2021-10-26T12:02:58Z

Some pullerbear bug here, sorry Joel for notification, unassigning you.

mountiny

@tgolen Thank you very much for your time reviewing this. I have addressed your comments and made the improvements. This is ready for another look 🙌

mountiny · 2021-10-26T12:09:52Z

src/pages/workspace/WorkspaceSettingsPage.js

-    componentWillUnmount() {
-        Policy.updateLocalPolicyValues(this.props.policy.id, {isAvatarUploading: false});
-    }
-


We won't need this block, because the entire upload process is now component agnostic and the picture or workspace details will upload even if user quits the component.

mountiny · 2021-10-26T12:10:40Z

src/pages/workspace/WorkspaceSettingsPage.js

    }

    submit() {
-        if (this.props.policy.isAvatarUploading || !this.validate()) {
+        if (this.props.policy.isPolicyUpdating || !this.validate()) {


Since I have separated the image upload flow from the other policy details, we need to use isPolicyUpdating here.

mountiny · 2021-10-26T12:11:27Z

src/pages/workspace/WorkspaceSettingsPage.js

+        if (this.props.policy.isAvatarUploading) {
+            return;
+        }
        this.setState({previewAvatarURL: image.uri});


Additionally, since we are now uploading and saving the ne picture immediately after users selects it from Desktop, we need to make sure they won't select new image while the previous one is still being uploaded.

tgolen

Awesome, this looks really great now!! Nice work

OSBotify · 2021-10-26T16:47:38Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

roryabraham · 2021-10-28T20:45:36Z

@mountiny Can you confirm if this is No QA? If so, please prefix the title with [No QA] 🙏 Thanks!

mountiny · 2021-10-28T20:48:41Z

@roryabraham This should be QA'ed for any regressions, I have forgot to add mention to the QA part that it should be same as tests. Fixed it now.

OSBotify · 2021-10-28T21:26:24Z

🚀 Deployed to staging by @tgolen in version: 1.1.10-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2021-10-29T22:13:19Z

🚀 Deployed to production by @roryabraham in version: 1.1.11-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mountiny added 5 commits October 25, 2021 14:30

Refactor the api call

6adcacc

Remove whitespace

3aaefde

Improve fundtion docs

1b6a89c

Remove the avatarURL from state and use onyx one

0d21cc0

Update the remove function to reflect we are now taking the avatarURL…

e69d4c1

… from ONYX

mountiny self-assigned this Oct 25, 2021

mountiny added 8 commits October 25, 2021 18:31

Refactor the updateAvatar API call to update the Policy on success

5e77872

Add explanatory comment to the updateAvatar action

ae3df64

Remove unused promise

73becf0

Improve some comments

5647b80

Use onyx.merge in the policy actions file

f078b76

Move the updateAvatar function after update function to satisfy linter

aa20ef1

Remove the return from updateAvatar funtion and throw error if upload…

5b7e995

… failed

Add growl success message on avatar upload

9e56837

mountiny requested a review from HorusGoul October 25, 2021 18:51

mountiny added 5 commits October 25, 2021 19:53

Use shouldGrowl parameter of update policy function

79c9f9f

Remove unnecessary growl call

4072f29

Use shouldGrowl parameter of when removing the avatar too

24d2dc7

Dont pass specific error message in updateAvatar function

0ef12a9

Remove growl

3d83845

mountiny commented Oct 25, 2021

View reviewed changes

mountiny changed the title ~~[WIP] Refactor the WorkspaceSettingsPage~~ Refactor the WorkspaceSettingsPage Oct 25, 2021

mountiny requested a review from a team October 25, 2021 19:10

mountiny marked this pull request as ready for review October 25, 2021 19:11

MelvinBot requested review from sketchydroide and removed request for a team October 25, 2021 19:11

mountiny changed the title ~~Refactor the WorkspaceSettingsPage~~ [HOLD #5271] Refactor the WorkspaceSettingsPage Oct 25, 2021

tgolen requested changes Oct 25, 2021

View reviewed changes

mountiny added 5 commits October 26, 2021 11:44

Merge main into 'vit-refactoringOfBadSmells'

ae3aea2

Simplofy the updateAvatar funtion

184ed36

Refactor to have updateLocalPolicyValues as internal function

0f0d5dc

Do not allow to submit the changes when upload is already happening

45d259e

Do not export updateLocalPolicyValues from the Policy action

93ce5b0

mountiny requested a review from a team as a code owner October 26, 2021 12:01

MelvinBot requested review from joelbettner and removed request for a team October 26, 2021 12:02

mountiny removed the request for review from joelbettner October 26, 2021 12:03

mountiny added 2 commits October 26, 2021 13:04

Reorder the functions to satisfy linter

f0759ce

Fix linter

a0507a9

mountiny requested a review from tgolen October 26, 2021 12:08

mountiny commented Oct 26, 2021

View reviewed changes

mountiny changed the title ~~[HOLD #5271] Refactor the WorkspaceSettingsPage~~ Refactor the WorkspaceSettingsPage Oct 26, 2021

tgolen approved these changes Oct 26, 2021

View reviewed changes

tgolen merged commit 1870921 into main Oct 26, 2021

tgolen deleted the vit-refactoringOfBadSmells branch October 26, 2021 16:46

OSBotify mentioned this pull request Oct 28, 2021

Deploy Checklist: New Expensify 2021-10-28 #6106

Closed

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the WorkspaceSettingsPage #6033

Refactor the WorkspaceSettingsPage #6033

mountiny commented Oct 25, 2021 •

edited

Loading

mountiny left a comment

mountiny Oct 25, 2021

tgolen Oct 25, 2021

mountiny Oct 26, 2021

mountiny Oct 25, 2021 •

edited

Loading

tgolen Oct 25, 2021

mountiny Oct 26, 2021

parasharrajat commented Oct 25, 2021

mountiny commented Oct 25, 2021

parasharrajat commented Oct 25, 2021 •

edited

Loading

tgolen left a comment

tgolen Oct 25, 2021

mountiny Oct 26, 2021

tgolen Oct 25, 2021

mountiny commented Oct 26, 2021

mountiny left a comment

mountiny Oct 26, 2021

mountiny Oct 26, 2021

mountiny Oct 26, 2021

tgolen left a comment

OSBotify commented Oct 26, 2021

roryabraham commented Oct 28, 2021

mountiny commented Oct 28, 2021

OSBotify commented Oct 28, 2021

OSBotify commented Oct 29, 2021

Refactor the WorkspaceSettingsPage #6033

Refactor the WorkspaceSettingsPage #6033

Conversation

mountiny commented Oct 25, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mountiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mountiny Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Oct 25, 2021

mountiny commented Oct 25, 2021

parasharrajat commented Oct 25, 2021 • edited Loading

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mountiny commented Oct 26, 2021

mountiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

OSBotify commented Oct 26, 2021

roryabraham commented Oct 28, 2021

mountiny commented Oct 28, 2021

OSBotify commented Oct 28, 2021

OSBotify commented Oct 29, 2021

mountiny commented Oct 25, 2021 •

edited

Loading

mountiny Oct 25, 2021 •

edited

Loading

parasharrajat commented Oct 25, 2021 •

edited

Loading