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.

Sign up for GitHub

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

Jump to bottom

[CP Staging] Fix: No notification when closing account #28800

Merged
merged 5 commits into from
Oct 4, 2023
Merged

[CP Staging] Fix: No notification when closing account #28800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
36a58b9
fix: 28748
tienifr Oct 4, 2023
f201b44
catch error
tienifr Oct 4, 2023
b7e3dab
Merge branch 'main' of https://github.com/tienifr/App into fix/28748
tienifr Oct 4, 2023
b92d2bd
run prettier
tienifr Oct 4, 2023
8410b96
detailed error message
tienifr Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/libs/Middleware/HandleUnusedOptimisticID.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ const handleUnusedOptimisticID: Middleware = (requestResponse, request, isFromSe
const responseOnyxData = response?.onyxData ?? [];
responseOnyxData.forEach((onyxData) => {
const key = onyxData.key;
if (!key) {
return;
}

if (!key.startsWith(ONYXKEYS.COLLECTION.REPORT)) {
Comment on lines +13 to 17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI optional chaining is allowed in TS:

Suggested change
if (!key) {
return;
}
if (!key.startsWith(ONYXKEYS.COLLECTION.REPORT)) {
if (!key?.startsWith(ONYXKEYS.COLLECTION.REPORT)) {

return;
}
Expand Down
84 changes: 44 additions & 40 deletions src/libs/Middleware/SaveResponseInOnyx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,50 +14,54 @@ const requestsToIgnoreLastUpdateID = ['OpenApp', 'ReconnectApp', 'GetMissingOnyx
* @returns {Promise}
*/
function SaveResponseInOnyx(requestResponse, request) {
return requestResponse.then((response) => {
// Make sure we have response data (i.e. response isn't a promise being passed down to us by a failed retry request and response undefined)
if (!response) {
return;
}
const onyxUpdates = response.onyxData;

// Sometimes we call requests that are successfull but they don't have any response or any success/failure data to set. Let's return early since
// we don't need to store anything here.
if (!onyxUpdates && !request.successData && !request.failureData) {
return Promise.resolve(response);
}

// If there is an OnyxUpdate for using memory only keys, enable them
_.find(onyxUpdates, ({key, value}) => {
if (key !== ONYXKEYS.IS_USING_MEMORY_ONLY_KEYS || !value) {
return false;
return requestResponse
Copy link
Contributor

@abdulrahuman5196 abdulrahuman5196 Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tienifr Why are having this much diff here? I know we are only adding catch to this?

Can we remove the unwanted diff? Any specific reason to have?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's prettier diff. Maybe because I added the catch phrase thus everything was shift down 1 line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already compared line by line and verified that they're the same. You can check it yourself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I also verified. No changes.

.then((response) => {
// Make sure we have response data (i.e. response isn't a promise being passed down to us by a failed retry request and response undefined)
if (!response) {
return;
}
const onyxUpdates = response.onyxData;

MemoryOnlyKeys.enable();
return true;
});
// Sometimes we call requests that are successfull but they don't have any response or any success/failure data to set. Let's return early since
// we don't need to store anything here.
if (!onyxUpdates && !request.successData && !request.failureData) {
return Promise.resolve(response);
}

// If there is an OnyxUpdate for using memory only keys, enable them
_.find(onyxUpdates, ({key, value}) => {
if (key !== ONYXKEYS.IS_USING_MEMORY_ONLY_KEYS || !value) {
return false;
}

MemoryOnlyKeys.enable();
return true;
});

const responseToApply = {
type: CONST.ONYX_UPDATE_TYPES.HTTPS,
lastUpdateID: Number(response.lastUpdateID || 0),
previousUpdateID: Number(response.previousUpdateID || 0),
request,
response,
};

if (_.includes(requestsToIgnoreLastUpdateID, request.command) || !OnyxUpdates.doesClientNeedToBeUpdated(Number(response.previousUpdateID || 0))) {
return OnyxUpdates.apply(responseToApply);
}

// Save the update IDs to Onyx so they can be used to fetch incremental updates if the client gets out of sync from the server
OnyxUpdates.saveUpdateInformation(responseToApply);

const responseToApply = {
type: CONST.ONYX_UPDATE_TYPES.HTTPS,
lastUpdateID: Number(response.lastUpdateID || 0),
previousUpdateID: Number(response.previousUpdateID || 0),
request,
response,
};

if (_.includes(requestsToIgnoreLastUpdateID, request.command) || !OnyxUpdates.doesClientNeedToBeUpdated(Number(response.previousUpdateID || 0))) {
return OnyxUpdates.apply(responseToApply);
}

// Save the update IDs to Onyx so they can be used to fetch incremental updates if the client gets out of sync from the server
OnyxUpdates.saveUpdateInformation(responseToApply);

// Ensure the queue is paused while the client resolves the gap in onyx updates so that updates are guaranteed to happen in a specific order.
return Promise.resolve({
...response,
shouldPauseQueue: true,
// Ensure the queue is paused while the client resolves the gap in onyx updates so that updates are guaranteed to happen in a specific order.
return Promise.resolve({
...response,
shouldPauseQueue: true,
});
})
.catch((err) => {
console.error('Got exception while saving response in Onyx', err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch causes a regression, it suppresses network errors such as 'Failed to fetch', but I believe it shouldn't.

Unit tests defined in APITest.js are not failing due to a TypeError which is always raised in SequentialQueue file here:

First an error is suppressed in SaveResponseInOnyx:

  console.error
    Got exception while saving response in Onyx Error: Failed to fetch
        at /tests/unit/APITest.js:90:40

      52 |         })
      53 |         .catch((err) => {
    > 54 |             console.error('Got exception while saving response in Onyx', err);
         |                     ^
      55 |         });
      56 |
      57 | export default SaveResponseInOnyx;

Later an error is raised as shouldPauseQueue isn't defined on undefined response:

    error1 =  TypeError: Cannot read properties of undefined (reading 'shouldPauseQueue')
        at /src/libs/Network/SequentialQueue.ts:73:26)

      at log (src/libs/Network/SequentialQueue.ts:81:21)

cc @mountiny @tienifr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

});
});
}

export default SaveResponseInOnyx;
Loading