Offline feedback and errors for optimistic room rows by neil-marcellini · Pull Request #11655 · Expensify/App

neil-marcellini · 2022-10-06T23:05:03Z

Details

I have wrapped the OptionRowLHN and the search OptionRow components in OfflineWithFeedback so they they are grayed out (50% opacity) while pending creation offline, and when creation fails.

I added a pendingAction key and an allReportErrors key to the options for use with the OfflineWithFeedback component. For now I set the pendingAction only for adding the workspace room, but it can be updated to be a combination of more fields in the future.

To set allReportErrors I combined all the error sources related to a report into one big object of error messages keyed by microtime. I set the brickRoadIndicator based on whether allReportErrors is empty.

I also added one new prop to OfflineWithFeedback to handle cases where we want a component to be grayed out when needed, but we don't want to show the error messages under it. Before I achieved that by putting display none of the error messages container. It's better to render the messages only when we need them by setting shouldShowErrorMessages to false.

Related Issues

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

Tests

Offline success

If needed, create a workspace by clicking the green plus, new workspace
Go offline
Click the green plus, new room
Fill out the form and hit save
Verify that you are navigated to the new room and the whole page is grayed out
Find the room row in the LHN and verify that it is grayed out
Click the search icon in the header and type in the room name, then verify that it is grayed out (50% opacity)
Go online
Verify that the room row in the LHN is not grayed out
Verify that the room row in the search is not grayed out

Offline error

If needed, create a workspace by clicking the green plus, new workspace
Go offline
Click the green plus, new room
Fill out the form with a name (#ffroom), workspace, and hit save
Verify that you are navigated to the new room and the whole page is grayed out
Find the room row in the LHN and verify that it is grayed out
Click the search icon in the header and type in the room name, then verify that it is grayed out (50% opacity)
Close the search page
Sign into the same account on another device or incognito window
Create another workspace room on the same workspace with the same name (#ffroom)
Go back to the other device and go back online
Verify that a red dot shows up on the room row in the LHN and it is grayed out
Verify that a red dot shows up on the room row in search and it is grayed out

Verify that no errors appear in the JS console

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Run the tests above.

Verify that no errors appear in the JS console

Screenshots

Web

Offline creation pending

Room creation failed grayed out in LHN and search

Android

Offline pending creation success

android-success.mov

Creation failed

android-fail.mov

neil-marcellini · 2022-10-11T23:50:54Z

I still need to check off 2 boxes and figure out why the jest unit tests are failing, but reviews can begin.

marcochavezf

Looks good to me so far 👍🏽 just a minor comment

neil-marcellini · 2022-10-17T22:05:37Z

Ready for reviews again. I think I have thoroughly addressed all comments.

neil-marcellini · 2022-10-18T23:12:17Z

I merged main to fix conflicts. Note, I would recommend viewing the diff without whitespace for an easier time reviewing because there are a lot of indentation changes.

iwiznia · 2022-10-19T10:21:34Z

    const needsOpacity = (isOfflinePendingAction && !isUpdateOrDeleteError) || isAddError;
    const needsStrikeThrough = props.network.isOffline && props.pendingAction === 'delete';
    const hideChildren = !props.network.isOffline && props.pendingAction === 'delete' && !hasErrors;
+    const showErrorMessages = props.shouldShowErrorMessages && hasErrors && !_.isEmpty(props.errors);


hasErrors already has that condition

Suggested change

const showErrorMessages = props.shouldShowErrorMessages && hasErrors && !_.isEmpty(props.errors);

const showErrorMessages = props.shouldShowErrorMessages && hasErrors;

iwiznia · 2022-10-19T10:23:33Z

        result.isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(report);
        result.shouldShowSubscript = result.isPolicyExpenseChat && !report.isOwnPolicyExpenseChat && !result.isArchivedRoom;
        result.brickRoadIndicator = getBrickRoadIndicatorStatusForReport(report, reportActions);
+        result.pendingAction = report.pendingFields ? report.pendingFields.addWorkspaceRoom : null;


😕 what is addWorkspaceRoom doing here? Isn't this a generic component?

I added a pendingAction key to the options for use with the OfflineWithFeedback component. For now I set the pendingAction only for adding the workspace room, but it can be updated to be a combination of more fields in the future.

That's from my description of the PR. Yes createOption is called several times by getOptions, which is used by getSearchOptions, getNewChatOptions, and getMemberInviteOptions. What's the harm in setting a pendingAction? What would you suggest instead?

My confusion comes from: we only need to show RBR in the LHN, no? Wouldn't this make it show in all those other places too?

We need to show the RBR / 50% opacity in the LHN and the search. It won't apply for a new chat, and I don't think it will apply for the member invite options, but I haven't confirmed that. Thanks for raising this point, because I should look into that. I will test / explain whether it applies for the member invite or not.

createOption is called several times by getOptions, which is used by getSearchOptions, getNewChatOptions, and getMemberInviteOptions.

The pendingAction should be set for rooms when calling getSearchOptions.

getNewChatOptions only returns reports with a single participant. It skips options without a login.

App/src/libs/OptionsListUtils.js

Lines 546 to 549 in 2df16aa

// Check the report to see if it has a single participant and if the participant is already selected

if (reportOption.login && _.some(loginOptionsToExclude, option => option.login === reportOption.login)) {

continue;

}

The login is only set when creating the option if the report doesn't have multiple participants.

App/src/libs/OptionsListUtils.js

Lines 364 to 368 in 2df16aa

if (!hasMultipleParticipants) {

result.login = personalDetail.login;

result.phoneNumber = personalDetail.phoneNumber;

result.payPalMeAddress = personalDetail.payPalMeAddress;

}

hasMultipleParticipants is always true for chatRooms.

App/src/libs/OptionsListUtils.js

Line 317 in 2df16aa

hasMultipleParticipants = personalDetailList.length > 1 || result.isChatRoom || result.isPolicyExpenseChat;

For getMemberInviteOptions we do not pass reports to getOptions so we won't access any rooms.

App/src/libs/OptionsListUtils.js

Lines 742 to 756 in 2df16aa

function getMemberInviteOptions(

personalDetails,

betas = [],

searchValue = '',

excludeLogins = [],

) {

return getOptions([], personalDetails, {

betas,

searchValue: searchValue.trim(),

excludeDefaultRooms: true,

includePersonalDetails: true,

excludeLogins,

sortPersonalDetailsByAlphaAsc: false,

});

}

So in summary we will only have a pendingAction for rooms in the LHN and search, as intended.

neil-marcellini · 2022-10-20T00:13:08Z

Sorry for all the confusion with this PR. Let's get this back on track and moving forward again. Here are my goals.

Gray out optimistic workspace room rows in the LHN and search page while pending creation offline, and when creation fails.
Use OfflineWithFeedback to achieve goal 1 and do so without rendering error messages, instead of hiding them with display none.
Use OfflineWithFeedback to achieve goal 1 without passing all of the possible report errors.
Keep OfflineWithFeedback relatively simple and logical.

Goal 3 is probably a premature optimization as @iwiznia hinted at here. I could do something like this, but that's also strange.

<OfflineWithFeedback
    pendingAction={optionItem.pendingAction}
    errors={!_.isEmpty(optionItem.brickRoadIndicator) ? {hasErrors: true} : null}
    shouldShowErrorMessages={false}
>

I'll go ahead and write a function to get all possible errors on a report, similar to how the errors are accessed in getBrickRoadIndicatorStatusForReport, and then pass all those errors to OfflineWithFeedback. I'll remove the hasErrors prop and the associated changes. The use in OptionRowLHN will look like this.

<OfflineWithFeedback
    pendingAction={optionItem.pendingAction}
    errors={ReportUtils.getAllErrors(props.reportID)}
    shouldShowErrorMessages={false}
>

Does that sound good?

neil-marcellini · 2022-10-20T17:45:06Z

My above plan was approved in Slack here so I'm going to push those changes now.

neil-marcellini · 2022-10-20T19:24:36Z

Ok there's a problem with my plan which is that the OptionRow and OptionRowLHN components do not currently have access to the report and reportActions, which are needed for getAllReportErrors.

I could write getAllReportErrors in OptionsListUtils since it already subscribes to reportActions, and then add an Onyx subscription to ONYXKEYS.COLLECTION.REPORT. However, that violates the convention in that file which is to take in a report parameter.

I could also subscribe to both report and reportActions in ReportUtils, but again that violates convention.

Instead I think I will add a reportErrors key to the option object, and I will set it right above this line.

App/src/libs/OptionsListUtils.js

Line 294 in d4bcc62

    
           result.brickRoadIndicator = getBrickRoadIndicatorStatusForReport(report, reportActions);

neil-marcellini · 2022-10-20T21:21:49Z

I'm having trouble writing the getAllReportErrors function. It should return an object of error messages keyed by microtime. To create that I need to merge together report.errors, all errors objects stored under report.errorFields, and all errors objects stored in the report actions.

I've been tinkering around with underscore.js trying to figure out how to do this.

neil-marcellini · 2022-10-21T19:07:29Z

Ready for another review 🙇

marcochavezf · 2022-10-22T02:44:44Z

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

Screen.Recording.2022-10-21.at.20.02.40.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Screen.Recording.2022-10-21.at.21.03.35.mov

marcochavezf · 2022-10-22T02:51:58Z

The code LGTM, I'm still testing on all platforms.

I found a weird behavior on iOS (which probably is not related to this PR) is that the RBR indicator appears after I open the room chat (disabled my mac's wifi):

Screen.Recording.2022-10-21.at.21.23.51.mov

And on Safari the error never showed up:

Screen.Recording.2022-10-21.at.21.37.43.mov

Edit: For some reason the network status is not changing on the iOS simulator when I disabled the wifi (I wonder if there's an issue with the NetInfo library 🤔 ), but I can reproduce the failure case for the room creation by restarting the app / browser. The only pending platform is Android / Chrome, the app is not loading locally for me (seems something is broken in my emulator).

neil-marcellini · 2022-10-24T19:10:48Z

Thanks for reviewing and starting the checklist @marcochavezf!

Edit: For some reason the network status is not changing on the iOS simulator when I disabled the wifi (I wonder if there's an issue with the NetInfo library 🤔 ),

Yeah offline testing on the iOS simulator doesn't really work. I had this PR up to add a switch to toggle offline mode for convenient testing, but it never actually got merged. You can try using the changes in that PR for testing. I'll put up another PR for it soon.

Please DM me if you have questions about testing. I'll look into why that jest unit test is not passing.

marcochavezf · 2022-10-24T22:57:42Z

Cool, thanks @neil-marcellini, I recreated the Android emulator to test on Mobile Chrome and everything looks good 👍🏽

neil-marcellini · 2022-10-25T00:27:14Z

Sweet. I figured out what was wrong with the tests so it should be good to go now!

iwiznia

NAB

iwiznia · 2022-10-25T16:51:02Z

+    const allReportErrors = {};
+    _.chain(errorSources)
+        .filter(errors => !_.isEmpty(errors))
+        .each(errors => _.extend(allReportErrors, errors));


Is this really doing a reduce? I think so, right? Wouldn't it be better to use the reduce method to make code clearer?

Oh yeah good point. I'm on it.

OSBotify · 2022-10-25T17:47:27Z

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

OSBotify · 2022-10-26T23:20:17Z

🚀 Deployed to staging by @neil-marcellini in version: 1.2.20-0 🚀

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

OSBotify · 2022-10-28T02:24:58Z

🚀 Deployed to production by @chiragsalian in version: 1.2.20-3 🚀

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

neil-marcellini added 2 commits October 6, 2022 15:55

Add pendingAction to LHN option data for rooms

e08e726

Offline feedback for LHN option row

a1d597c

neil-marcellini self-assigned this Oct 6, 2022

neil-marcellini added 5 commits October 6, 2022 17:11

Allow offline feedback with errors and no messages

bbc54a1

Gray out room in LHN that failed to be created

c9c6b83

Room creation pending or failed gray out in search

f80c58f

Don't render errors rather than hiding them

76dccd5

Merge branch 'main' into neil-pending-rooms

a545d80

neil-marcellini marked this pull request as ready for review October 11, 2022 23:50

neil-marcellini requested a review from a team as a code owner October 11, 2022 23:50

melvin-bot Bot requested review from marcochavezf and removed request for a team October 11, 2022 23:51

marcochavezf reviewed Oct 12, 2022

View reviewed changes

Comment thread src/components/OfflineWithFeedback.js Outdated

marcochavezf reviewed Oct 12, 2022

View reviewed changes

neil-marcellini added 2 commits October 17, 2022 14:41

Show error messages only if they exist

8df3f19

Merge branch 'main' into neil-pending-rooms

5e86ae2

neil-marcellini requested review from iwiznia and marcochavezf October 17, 2022 21:59

Merge branch 'main' into neil-pending-rooms

d4bcc62

iwiznia requested changes Oct 19, 2022

View reviewed changes

Remove the hasErrors prop and related changes

97844c3

neil-marcellini added 2 commits October 20, 2022 18:28

Add function to get all errors related to a report

a439314

Add allReportErrors to options objects

856de84

neil-marcellini added 3 commits October 20, 2022 19:29

Remove unused function

2b0b0d1

Merge branch 'main' into neil-pending-rooms

2df16aa

Additional merge conflict fix

7a560ae

neil-marcellini requested a review from iwiznia October 21, 2022 19:07

marcochavezf reviewed Oct 22, 2022

View reviewed changes

Comment thread src/libs/OptionsListUtils.js Outdated

marcochavezf reviewed Oct 22, 2022

View reviewed changes

Comment thread src/libs/OptionsListUtils.js Outdated

Use .each since no value is returned

87cde49

neil-marcellini requested a review from marcochavezf October 24, 2022 19:11

Fix jest by providing network prop

28515f3

Merge branch 'main' into neil-pending-rooms

dc52ba5

marcochavezf previously approved these changes Oct 25, 2022

View reviewed changes

iwiznia previously approved these changes Oct 25, 2022

View reviewed changes

Use reduce to get all report errors

87eca24

neil-marcellini dismissed stale reviews from iwiznia and marcochavezf via 87eca24 October 25, 2022 17:38

neil-marcellini requested review from iwiznia and marcochavezf October 25, 2022 17:38

iwiznia approved these changes Oct 25, 2022

View reviewed changes

neil-marcellini merged commit 9950642 into main Oct 25, 2022

neil-marcellini deleted the neil-pending-rooms branch October 25, 2022 17:43

OSBotify mentioned this pull request Oct 26, 2022

Deploy Checklist: New Expensify 2022-10-26 #12165

Closed

28 tasks

	const showErrorMessages = props.shouldShowErrorMessages && hasErrors && !_.isEmpty(props.errors);
	const showErrorMessages = props.shouldShowErrorMessages && hasErrors;

	// Check the report to see if it has a single participant and if the participant is already selected
	if (reportOption.login && _.some(loginOptionsToExclude, option => option.login === reportOption.login)) {
	continue;
	}

	if (!hasMultipleParticipants) {
	result.login = personalDetail.login;
	result.phoneNumber = personalDetail.phoneNumber;
	result.payPalMeAddress = personalDetail.payPalMeAddress;
	}

	function getMemberInviteOptions(
	personalDetails,
	betas = [],
	searchValue = '',
	excludeLogins = [],
	) {
	return getOptions([], personalDetails, {
	betas,
	searchValue: searchValue.trim(),
	excludeDefaultRooms: true,
	includePersonalDetails: true,
	excludeLogins,
	sortPersonalDetailsByAlphaAsc: false,
	});
	}

Uh oh!

Conversation

neil-marcellini commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Tests

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

neil-marcellini commented Oct 11, 2022

Uh oh!

Uh oh!

marcochavezf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neil-marcellini commented Oct 17, 2022

Uh oh!

neil-marcellini commented Oct 18, 2022

Uh oh!

iwiznia Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

iwiznia Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

neil-marcellini Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwiznia Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

neil-marcellini Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

neil-marcellini Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

neil-marcellini commented Oct 20, 2022

Uh oh!

neil-marcellini commented Oct 20, 2022

Uh oh!

neil-marcellini commented Oct 20, 2022

Uh oh!

neil-marcellini commented Oct 20, 2022

Uh oh!

neil-marcellini commented Oct 21, 2022

Uh oh!

marcochavezf commented Oct 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

marcochavezf commented Oct 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neil-marcellini commented Oct 24, 2022

Uh oh!

marcochavezf commented Oct 24, 2022

Uh oh!

neil-marcellini commented Oct 6, 2022 •

edited

Loading

marcochavezf left a comment •

edited

Loading

neil-marcellini Oct 19, 2022 •

edited

Loading

marcochavezf commented Oct 22, 2022 •

edited

Loading

marcochavezf commented Oct 22, 2022 •

edited

Loading