Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$1000] Desktop - error message should show in Spanish #19651

Closed
6 tasks done
mvtglobally opened this issue May 26, 2023 · 79 comments
Closed
6 tasks done

[$1000] Desktop - error message should show in Spanish #19651

mvtglobally opened this issue May 26, 2023 · 79 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@mvtglobally
Copy link

mvtglobally commented May 26, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Click Profile > Preference
  2. Click Force Offline
  3. Change the language to Spanish
  4. Click on new chat with invalid number, example +15005550111
  5. Send messages
  6. Go back to Preference, change the language to English > toggle off Force Online

Expected Result:

Error should be in English now that the account language is English

Actual Result:

The error messages is shown in Spanish when the language was already changed to English.

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: v1.3.18-2
Reproducible in staging?: Y
Reproducible in production?: N/A the toggle is not present in PROD app
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
Found when executing PR #19324

Bug6068770_19324_Desktop.mp4

Expensify/Expensify Issue URL:
Issue reported by: Applause Internal Team
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01fdc96c68b8a156b6
  • Upwork Job ID: 1662146078155325440
  • Last Price Increase: 2023-07-11
@mvtglobally mvtglobally added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 26, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 26, 2023

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

@dummy-1111
Copy link
Contributor

The root cause of this issue is as follows.
When switched from offline mode to online mode, queued requests are sent. If user changes the locale from English to Spanish and again from Spanish to English, then 2 updatePreferredLocale APIs are called with en and then es.

This works fine, but the push notification arrives sometimes in reverse order and sometimes a bit later by my observation.
image

That's the main reason.

I think the push notification shouldn't be sent to the author(who sent the request). Or at least add user ID info not to update onyx store.

This should be handled in backend side

@sakluger
Copy link
Contributor

Not sure if it's related, but this bug seems similar to this other one: LHN Attachment taking some to to translate into Spanish

@dummy-1111
Copy link
Contributor

Not sure if it's related, but this bug seems similar to this other one: LHN Attachment taking some to to translate into Spanish

I took a lot of tests. Please check
https://github.com/Expensify/App/assets/126839717/c21675da-2a3b-4868-9c55-17f989fc147f

@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label May 26, 2023
@melvin-bot melvin-bot bot changed the title Desktop - Error messages is shown in Spanish when the language was already changed to English [$1000] Desktop - Error messages is shown in Spanish when the language was already changed to English May 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 26, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 26, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 26, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 26, 2023

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

@fedirjh
Copy link
Contributor

fedirjh commented May 26, 2023

I want to bring attention the actual result, which doesn’t looks correct or accurate

The error messages is shown in Spanish when the language was already changed to English.

The error message reflects the selected langue :

In this frame , the language is set Spanish , you can verify the chat header , which display Spanish translation

Screenshot 2023-05-26 at 9 34 48 PM

In this frame , the language is updated to English , you can verify the chat header as well

Screenshot 2023-05-26 at 9 35 04 PM

@StevenKKC
Copy link
Contributor

StevenKKC commented May 26, 2023

Proposal

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

The error messages are shown in Spanish when the user change the language to English and set online mode.

What is the root cause of that problem?

The root cause is, even if the user change the language to English, the app's language will be set to Spanish after change to online mode.

In offline mode, all UpdatePreferredLocale API request does not send immediately to the server, and are pushed to the SequentialQueue.
Once the user change to online mode, the API requests in the queue will be sent to the server.

Assume that the initial language is English.
If user changes the language from English to Spanish, then one UpdatePreferredLocale API is pushed to the queue, and send to the server later. In this case, the app's language is set to Spanish.

If user changes the language from English to Spanish and again from Spanish to English, then 2 UpdatePreferredLocale APIs are pushed to the queue and will be sent with en and then es language value. And the responses from the server are processed in reverse order, so finally the app process the first API's response, and language is set to Spanish.

If user change language to English again at this time, then three APIs are pushed to the queue.
But only 2 APIs will be sent to the server, because after send one API, the same APIs will be removed by PersistedRequests.remove(requestToProcess); in process function.

function process() {
const persistedRequests = PersistedRequests.getAll();
if (_.isEmpty(persistedRequests) || NetworkStore.isOffline()) {
return Promise.resolve();
}
const requestToProcess = persistedRequests[0];
// Set the current request to a promise awaiting its processing so that getCurrentRequest can be used to take some action after the current request has processed.
currentRequest = Request.processWithMiddleware(requestToProcess, true)
.then(() => {
PersistedRequests.remove(requestToProcess);
RequestThrottle.clear();
return process();
})
.catch((error) => {
// On sign out we cancel any in flight requests from the user. Since that user is no longer signed in their requests should not be retried.
// Duplicate records don't need to be retried as they just mean the record already exists on the server
if (error.name === CONST.ERROR.REQUEST_CANCELLED || error.message === CONST.ERROR.DUPLICATE_RECORD) {
PersistedRequests.remove(requestToProcess);
RequestThrottle.clear();
return process();
}
return RequestThrottle.sleep().then(process);
});
return currentRequest;
}

/**
* @param {Object} requestToRemove
*/
function remove(requestToRemove) {
persistedRequests = _.reject(persistedRequests, (persistedRequest) => _.isEqual(persistedRequest, requestToRemove));
Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
}

And the app process the first API's response, and the language is set to es, that is Spanish.

If user change to Spanish again, then 4 APIs are pushed, but the final language is set to es for the same reason.

That is, when user changes the language in offline mode, one or two API will be sent to the server, and the app language is set to the first changed language, that is Spanish.

The same is true if the language is initially set to Spanish.
In this case, after change to online mode, the language will be set to English.

1

That is, when change to online mode, the language is set to the first API's language.

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

We should only send the last UpdatePreferredLocale API to the server once.
For this, in offline mode when the new API is pushed to the queue, we would remove previous API requests and push the last API request by changing the save function as below.

function save(requestsToPersist) {
    HttpUtils.cancelPendingReconnectAppRequest();
    persistedRequests = _.chain(persistedRequests)
        .concat(requestsToPersist)
        .partition((request) => request.command !== CONST.NETWORK.COMMAND.RECONNECT_APP)
        .flatten()
        .value();

    if (NetworkStore.isOffline() && requestsToPersist[0].command === 'UpdatePreferredLocale') {
        persistedRequests = _.reject(persistedRequests, (persistedRequest) => (persistedRequest.command === 'UpdatePreferredLocale' && persistedRequest !== requestsToPersist[0]));
    }

    Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
}

Now we can send the last one API to the server, so the app language is set correctly when changes from offline mode to online mode.

What alternative solutions did you explore? (Optional)

None.

@abdulrahuman5196
Copy link
Contributor

@fedirjh Thank you for pointing out.

@Christinadobrzyn Could you check this #19651 (comment)
I am also not sure of the issue here, the error message is in sync with the app's language.

Are we thinking that,
"app language should directly change to the last choosen language, instead of switching through the previous selections?" If so I think that's a different issue and this issue's information has to be updated,

And the proposals here is also trying to fix the issue of "app language should directly change to the last choosen language, instead of switching through the previous selections"

@abdulrahuman5196
Copy link
Contributor

@s-alves10 Not sure if this #19651 (comment) is a proposal or general rootcause comment. If it is a proposal, kindly update the proposal as per our guidelines. https://github.com/Expensify/App/blob/main/contributingGuides/PROPOSAL_TEMPLATE.md

@c3024
Copy link
Contributor

c3024 commented May 28, 2023

Proposal

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

After forcing offline, changing language to Spanish, sending messages to an invalid number, then again changing language to English, then going online shows errors for the messages in Spanish.

What is the root cause of that problem?

Lines 39 to 71 of App/src/libs/API.js

function write(command, apiCommandParameters = {}, onyxData = {}) {
    ....

    // Write commands can be saved and retried, so push it to the SequentialQueue
    SequentialQueue.push(request);
}

API.js pushes requests to SequentialQueue

These requests are pushed to persistedRequests and they would be processed right away through flush if network is online. If NetworkStore.isOffline()), push returns and requests stay in persistedRequests.

Lines 105 to 121 of App/src/libs/Network/SequentialQueue.js

function push(request) {
    // Add request to Persisted Requests so that it can be retried if it fails
    PersistedRequests.save([request]);

    // If we are offline we don't need to trigger the queue to empty as it will happen when we come back online
    if (NetworkStore.isOffline()) {
        return;
    }
    ...
    flush();
}

Once network is online SequentialQueue all requests in persistedRequests are processed one by one.

Line 32 to 57 of App/src/libs/Network/SequentialQueue.js

function process() {
    const persistedRequests = PersistedRequests.getAll();
    ...
    currentRequest = Request.processWithMiddleware(requestToProcess, true)
        .then(() => {
            PersistedRequests.remove(requestToProcess);
            RequestThrottle.clear();
            return process();
        })
       ...
    return currentRequest;
}

Language change, and adding comments after forcing offline get added to persistentRequests and they are processed one by one after network is online and it appears slow.

In this case [UpdatePreferenceLocale, AddComment, AddComment, UpdatePreferenceLocale] are processed one after another. So, first language is changed to Spanish, then comments added and the errors would be displayed in Spanish, then language is changed to English and page changes to English.
What is the root cause of the problem

Offline requests are saved in PersistedRequests which are then processed sequentially in process of SequentialQueue and then they are updated with Onyx.update(QueuedOnyxUpdates.getQueuedUpdates()) here

App/src/libs/Network/SequentialQueue.js

Line 86 in f4f89a0
Onyx.update(QueuedOnyxUpdates.getQueuedUpdates()).then(QueuedOnyxUpdates.clear);

These are updated with this
https://github.com/Expensify/react-native-onyx/blob/5a97bf5282e614b9e9357fb8d620815f0399cef5/lib/Onyx.js#L1216

When different updates happen to same element like preferredLocale or for same reportID and reportingActionID, order in which the updates happen cannot be ensured.

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

In Onyx.js instead of Promise.all we should use

_.reduce(promises, (promiseChain, currentPromise) => {
return promiseChain.then(() => currentPromise);
}, Promise.resolve())

What other solutions did you explore?

  1. Do not UpdatePreferredLocale to persistentRequests for retrying when network is offline. When language is changed, the page changes to that language irrespective of the network connectivity. I think keeping it for running after getting online is unnecessary.
    For this, the following change should be made.
    Lines 105 to 107 of App/src/libs/Network/SequentialQueue.js
function push(request) {
    // Don't retry UpdatePreferredLocale requests so don't add to persistedRequests
     if (request.command !== 'UpdatePreferredLocale') {
        PersistedRequests.save(request);
    }
  ...
}

What alternative solutions did you explore? (Optional)

  1. Here is a solution for a similar issue.
  2. Another solution is to only keep the latest UpdatePreferredLocale in peristedRequests. So after coming to online only the last selected preference would be there and it will update first.
    For this, retain the SequentialQueue.js as it is but following change is required in PersistedRequests.js
    Line 21 to 29 of App/src/libs/action/PersistedRequests.js
function save(requestsToPersist) {
    HttpUtils.cancelPendingReconnectAppRequest();
   if (requestsToPersist[0].command === "UpdatePreferredLocale") {
       persistedRequests = _.filter(persistedRequests, persistedRequest => persistedRequest.command !== "UpdatePreferredLocale";
        persistedRequests = _.chain(requestsToPersist)
          .concat(persistedRequests)
         .partition((request) => request.command !== CONST.NETWORK.COMMAND.RECONNECT_APP)
          .flatten()
          .value();
  }
  else {
     persistedRequests = _.chain(persistedRequests)
          .concat(requestsToPersist)
         .partition((request) => request.command !== CONST.NETWORK.COMMAND.RECONNECT_APP)
          .flatten()
          .value();
   }
      
    Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
}

@melvin-bot melvin-bot bot added the Overdue label May 29, 2023
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented May 30, 2023

@abdulrahuman5196 - great question

"Are we thinking that "app language should directly change to the last choosen language, instead of switching through the previous selections?" If so I think that's a different issue and this issue's information has to be updated,

And the proposals here is also trying to fix the issue of "app language should directly change to the last choosen language, instead of switching through the previous selections"

The issue was that the error message was in Spanish after you changed the language to English (and enabled online). So "app language should directly change to the last choosen language, instead of switching through the previous selections" is what I would expect to happen here.

But I'm not able to reproduce this Spanish error anymore - going to reach out to QA to see if they can test again. https://expensify.slack.com/archives/C9YU7BX5M/p1685475025261219

cc @mvtglobally

@melvin-bot melvin-bot bot removed the Overdue label May 30, 2023
@StevenKKC
Copy link
Contributor

@Christinadobrzyn I'm able to reproduce this error. Please change the language an even number of times, and change to online mode.

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Jun 1, 2023

@abdulrahuman5196 - finally I was able to reproduce this again! I updated the steps in the OP on how I was able to do this.

What is expected: when switching preferences from Spanish to English, all messages (including errors) should be in English.

What is happening: when switching preferences from Spanish to English, all messages (including errors) are in Spanish.

2023-05-31_17-59-36 (1)

@cead22
Copy link
Contributor

cead22 commented Jun 26, 2023

I started a discussion about how to move this forward here https://expensify.slack.com/archives/C01GTK53T8Q/p1687808610323569. We're leaning towards closing this issue

@melvin-bot melvin-bot bot added the Overdue label Jun 28, 2023
@Christinadobrzyn
Copy link
Contributor

I'll check on that thread, not sure if we got to a conclusion.

@melvin-bot melvin-bot bot removed the Overdue label Jun 28, 2023
@Christinadobrzyn
Copy link
Contributor

I think we're moving forward with this, right @cead22?

@StevenKKC
Copy link
Contributor

I agree that my proposal is for a special case, not a general one.
Add removePreviousRequests to API.write function's apiCommandParameters parameter and change code as below maybe more acceptable.

    API.write(
        'UpdatePreferredLocale',
        {
            value: locale,
            removePreviousRequests: NetworkStore.isOffline(),
        },
        {optimisticData},
    );
function save(requestsToPersist) {
    HttpUtils.cancelPendingReconnectAppRequest();
    persistedRequests = _.chain(persistedRequests)
        .concat(requestsToPersist)
        .partition((request) => request.command !== CONST.NETWORK.COMMAND.RECONNECT_APP)
        .flatten()
        .value();
    if (requestsToPersist[0].data.removePreviousRequests && requestsToPersist[0].data.removePreviousRequests === true) {
        persistedRequests = _.reject(persistedRequests, (persistedRequest) => (persistedRequest.command === requestsToPersist[0].command && persistedRequest !== requestsToPersist[0]));
    }
    Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests);
}

For the general solution, there are not enough cases to consider, and I think the above solution is sufficient for some preference settings in offline mode.
It would be better if we discuss general solution when we have more cases in the future.

@melvin-bot melvin-bot bot added the Overdue label Jul 3, 2023
@abdulrahuman5196
Copy link
Contributor

Not overdue.

I think we're moving forward with this, right @cead22?

I doubt that. Could you kindly provide us what is the path forward we are looking on this issue? @cead22

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 3, 2023
@cead22
Copy link
Contributor

cead22 commented Jul 5, 2023

In this thread we identified the following two situations, and agreed that the first one isn't a bug, and that we'll use this issue to fix the second one, does that make sense?

  1. If you switch languages multiple times while offline, when you come online, the language will switch multiple times as the requests queued while you were offline are dequeued and sent to the server
  2. We have an error message that is always shown in English

@melvin-bot melvin-bot bot removed the Overdue label Jul 5, 2023
@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Jul 6, 2023

Yes. @cead22 Fine by me. I think the message is received from backend.

@cead22
Copy link
Contributor

cead22 commented Jul 6, 2023

Correct me if I'm wrong but now that we're switching gears to solve only problem number 2 above, I don't think we have any proposals for that specifically right now.

If that's the case, @StevenKKC @c3024 @s-alves10 would you like to submit a proposal for this? Thanks

@Christinadobrzyn Christinadobrzyn added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 6, 2023
@melvin-bot melvin-bot bot added the Overdue label Jul 10, 2023
@cead22
Copy link
Contributor

cead22 commented Jul 10, 2023

Still waiting for proposals

@melvin-bot melvin-bot bot removed the Overdue label Jul 10, 2023
@abdulrahuman5196
Copy link
Contributor

@cead22 / @Christinadobrzyn The bug is still marked internal? Could we change it to external and update the bug title and description as per the new requirement from here - #19651 (comment)

@Christinadobrzyn Christinadobrzyn added External Added to denote the issue can be worked on by a contributor and removed Internal Requires API changes or must be handled by Expensify staff labels Jul 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 10, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 10, 2023

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

@Christinadobrzyn Christinadobrzyn changed the title [$1000] Desktop - Error messages is shown in Spanish when the language was already changed to English [$1000] Desktop - error message should show in Spanish Jul 10, 2023
@Christinadobrzyn
Copy link
Contributor

@abdulrahuman5196 should I raise the bounty to get more proposals?

@pecanoro
Copy link
Contributor

That error is a back-end error, it cannot be fixed as we don't support handling translations for back-end errors yet. I would say that we can close this issue as we have done with many other instances when this was brought up.

@melvin-bot
Copy link

melvin-bot bot commented Jul 11, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@Christinadobrzyn
Copy link
Contributor

@cead22 @abdulrahuman5196 should we close based on #19651 (comment)?

@cead22
Copy link
Contributor

cead22 commented Jul 14, 2023

Agree with Rocío, let's close

@cead22 cead22 closed this as completed Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests