[$1000] Getting errors on other workspaces too if you get an error in a single workspace

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to staging dot on chrome web
2. Create a new workspace
3. Change the name of the workspace and type in multiple emojis until you get an error
4. Click on save
5. Now go to other workspaces general settings and see that each of the workspace also displays the same error . An error in a single workspace is affecting other workspaces too. Also it's affecting when a new workspace is created.

Expected Result:

Other workspaces should not be affected if an error arises in a single workspace

Actual Result:

Other workspaces also gets the same error when one workspaces gets the error on general settings page

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.93-4
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

workspace.mp4

Recording.116.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680274078505429

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e5262531be62da28
• Upwork Job ID: 1642845457274187776
• Last Price Increase: 2023-04-03

[MelvinBot]

Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01e5262531be62da28

[MelvinBot]

Current assignee @laurenreidexpensify is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

[MelvinBot]

Triggered auto assignment to @AndrewGable (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[laurenreidexpensify]

Sorry took me a while longer than normal to review, can confirm all checks out and ready for external

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workspace error is "shared" to all workspaces.

What is the root cause of that problem?

The error comes from server response in this form of json

Focus on the onyxData. We can see that the key of the onyx is workspaceSettingsForm which is also the key of the workspace setting Form component.

App/src/pages/workspace/WorkspaceSettingsPage.js

Lines 89 to 90 in 4ee21e9

	<Form
	formID={ONYXKEYS.FORMS.WORKSPACE_SETTINGS_FORM}

This means, all workspace have the same key, so when we receive an error on one workspace, all the workspace will also show the same error.

What changes do you think we should make in order to solve the problem?

1. First, the response should be fixed from the BE by returning the workspaceSettingsForm appended with the workspace/policy id (i.e., workspaceSettingsForm_123)
2. On the FE, we should update the Form key by appending it with the workspace/policy id (the same thing as above).

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When typing in multiple emojis into the workspace name, we get a back-end invalid length error, and the error propagates to other workspaces.

What is the root cause of that problem?

There're 2 problems here:

1. The emojis-filled workspace name escapes the max length validation in the front-end side and was sent to the back-end, leading to errors. This doesn't happen with no-emoji workspace name. This leads to the user able to save the invalid name as optimistic data and the wrong name shows up all over the app, we should make sure to validate it correctly.
   This is because in the back-end, we're using C's strlen to calculate the length of the string.
   So 😁 is 2 in client side, 4 in server side.
   😶‍🌫️ is 6 in client side, 14 in server side.
   a is 1 both in client and server side.
   We've had this issue before with the comment length here.

2. The error propagates to other workspaces
   This is because the formId is being set to the same string for all workspaces here

   App/src/pages/workspace/WorkspaceSettingsPage.js

   Line 90 in 2123e1e

   formID={ONYXKEYS.FORMS.WORKSPACE_SETTINGS_FORM}

   , so all workspaces share the same error.

What changes do you think we should make in order to solve the problem?

1. To fix this we need to add a client-side validation so that the length is calculated consistently with the back-end, and there'll be no error thrown.

Since the C's strlen is calculating the byte of the string, we can do the same in JS by using the TextEncoder and get the byteLength property of the text

const getStringLength = (text) => {
   const encoder = new TextEncoder();
   const encodedText = encoder.encode(text);
   return encodedText.byteLength;   
}

The validation can be added to this function

App/src/pages/workspace/WorkspaceSettingsPage.js

Line 65 in 6369026

validate(values) {

2. To fix this we need to make the formId unique for each workspace, we can use ${ONYXKEYS.FORMS.WORKSPACE_SETTINGS_FORM}_${this.props.policy.id} as the formId in client side, and the back-end will need to return the form id in that format as well.

What alternative solutions did you explore? (Optional)

• To fix 1 above, another way is to change the BE to calculate the string length in similar way to the FE, but as mentioned in a similar issue, changing the back-end is expensive so we opted for changing the front-end, so I assume we might want the same here. Also there could be other forms that also have the 1 issue which will benefit from the same fix.
• We'll want to fix the 2 for all forms with similar issues as well, currently we're using a fixed string for many such forms that share the same UI.

[hoangzinh]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Getting same errors on other workspaces too if you get an API error in a single workspace

What is the root cause of that problem?

This length error came from API response, and same as other API response, it would be stored in Onyx. When we navigate to another workspace, because it use shared Onyx key and Form component so it shows same errors

What changes do you think we should make in order to solve the problem?

I think when we left form, we can clear all form errors stored in Onyx. We don't store FE validation errors in Onyx so I think in this case, we can clear all form errors (include BE API error) after we left form is make sense to me.

More details, in this case, we can call action FormActions.setErrorFields to clear all errors in the lifecyle componentWillUnmount of component WorkspaceSettingsPage.

If we want to apply this logic for all forms, we can add same logic into the Form component.

---

There is also another bug (I think) according to the video in GH issue. When BE API response error, the name of Workspace is not reverted but keep unsaved name

[sobitneupane]

Thanks for the proposal everyone.

Proposal from @tienifr looks good to me. He is proposing to improve front-end validation along with uniqueness in formID.

🎀👀🎀 C+ reviewed

cc: @AndrewGable

[bernhardoj]

That won't solve the initial issue of this GH, right?. If there is another server error, the "shared error" issue will happen again. I think the different length validation should be handled on a separate issue.

[tienifr]

@bernhardoj thanks for the concern, the "shared error" will be solved with the 2nd part of my proposal, that part is similar to your proposal.

I think the different length validation should be handled on a separate issue.

The different length issue is what leads the user to notice this bug so I think it's part of the issue and can be handled here.

[bernhardoj]

It's related, but has a different root cause. We previously have a case where even we have the same root cause, yet handled on a separate issue #15925. Why this case would be different? There is also another related issue that @hoangzinh mentioned on his proposal. Why we ignore that? We can simply revert the old workspace value in failureData. I'm ignoring the related issues as it is not the scope of this GH.

I would love to hear and accept any decision from @AndrewGable.

[tienifr]

There is also another related issue that @hoangzinh mentioned on his proposal. Why we ignore that? We can simply revert the old workspace value in failureData

@bernhardoj IMO that's expected, not an issue. If we revert the old workspace value the user will have no idea what happened (let's say if the user updates the workspace name when offline, then comes back online).

Anyway that case will not happen if we have proper validation from client-side.

Let's wait for @AndrewGable to take a look.

Thanks!

[laurenreidexpensify]

@sobitneupane @AndrewGable bump on review ^^

[sobitneupane]

#16830 (comment)

cc: @AndrewGable

[AndrewGable]

Sorry for delay, agree with @sobitneupane. Assigned @tienifr.

[tienifr]

As mentioned in my proposal:

To fix this we need to make the formId unique for each workspace, we can use ${ONYXKEYS.FORMS.WORKSPACE_SETTINGS_FORM}_${this.props.policy.id} as the formId in client side, and the back-end will need to return the form id in that format as well.

I believe we need to put this issue on hold until BE change is made. CC: @laurenreidexpensify

[tienifr]

@laurenreidexpensify Just a friendly remind in case you missed this!

[laurenreidexpensify]

thanks @tienifr - @AndrewGable are we good to hold this one?

[laurenreidexpensify]

Note to myself: I have sent offers to C, C+ and bug reporter in Upwork, but still waiting on @AndrewGable confirmation re: hold

[tienifr]

Bump @AndrewGable in case you missed it: #16830 (comment).

In addition, while we're waiting for BE update, I think we should proceed with reviewing the PR: #16958. @sobitneupane What do you think?

[sobitneupane]

@tienifr As per my understanding the change in backend is required to prevent such issues in the future. For this specific case, your PR should be sufficient to solve the issue, right?

[tienifr]

My proposal has two parts:

1. Client-side validation
2. formID update (require BE change as well)

Therefore, my PR would fully work only when BE is updated. Otherwise, only the first one is resolved.

For example, if it were another issue not related to workspace name and causing BE error, user wouldn't get notified; since BE's formID had not been updated to match client's formID as in my PR.

[sobitneupane]

In that case, I will prefer reviewing the PR only after the change in backend. Overall, PR looks good to me.

[MelvinBot]

@AndrewGable, @sobitneupane, @laurenreidexpensify, @tienifr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[laurenreidexpensify]

@AndrewGable do we have an idea of where the change in the backend is being made? I'm a bit confused here if we're still held on a specific issue, or whether we need to write another PR for the change referred to here?

[laurenreidexpensify]

Starting a convo in Slack https://expensify.slack.com/archives/C049HHMV9SM/p1680274078505429

[laurenreidexpensify]

Still discussing backend change in Slack in thread above

[laurenreidexpensify]

Conversation ongoing in Slack

[laurenreidexpensify]

Bumped Slack thread for next steps

[laurenreidexpensify]

Bumped Slack thread again

[laurenreidexpensify]

Bumped Slack thread again

[laurenreidexpensify]

As discussed in Slack today, we can no longer reproduce this so am closing. thank you

[tienifr]

hi @laurenreidexpensify, I think contributors are eligible for compensation here since the solution was valid at the point of assignment and the PR process already started, just that due to some other changes, the solution became out of date.

Another recent similar case is here and here

Thanks!

cc @sobitneupane

[tienifr]

@laurenreidexpensify could you take a look at this?

Thanks!

[laurenreidexpensify]

Folks I will review the comments here tomorrow and give a recommendation. thanks for the flag!

[laurenreidexpensify]

@tienifr I've issued payment in Upwork as you were already hired for the job there, @sobitneupane I had to create a new issue #20242 to complete payment for you, so let's finish up payment on that issue 👍

[tienifr]

@laurenreidexpensify thank you for looking into this!