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

Update API.md to reflect microtime keyed errors #10145

Merged
merged 2 commits into from
Aug 1, 2022
Merged

Conversation

yuwenmemon
Copy link
Contributor

@iwiznia @bondydaa @neil-marcellini @Gonals @chiragsalian @MariaHCD please review

Update the API.md docs to reflect the fact that we want errors returned in an array keyed by the microtime.

@yuwenmemon yuwenmemon requested a review from a team as a code owner July 27, 2022 23:46
@yuwenmemon yuwenmemon self-assigned this Jul 27, 2022
@melvin-bot melvin-bot bot requested review from PauloGasparSv and removed request for a team July 27, 2022 23:46
@yuwenmemon
Copy link
Contributor Author

I'm actually curious how we want to handle the "other additional properties" case - should those also be keyed under the microtime?

PauloGasparSv
PauloGasparSv previously approved these changes Jul 28, 2022
Copy link
Contributor

@PauloGasparSv PauloGasparSv left a comment

Choose a reason for hiding this comment

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

LGTM
Just couldn't find Onyx::getErrorMicroTime on main.

@yuwenmemon feel free to merge, not merging so people can have time to look and discuss your last comment.

Copy link
Contributor

@MariaHCD MariaHCD left a comment

Choose a reason for hiding this comment

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

I'm actually curious how we want to handle the "other additional properties" case - should those also be keyed under the microtime?

I'm thinking it makes sense to associate the additional properties with the same microtime key. For instance, in UpdatePersonalDetailsForWallet the API might return an additional prop 'errorCode' => 'ssnError' and later we might also return a different errorCode kbaNeeded.

If we didn't have the additional properties keyed under microtime the onyxData would look like:

[
    [
        'onyxMethod' => Onyx::METHOD_MERGE,
        'key' => OnyxKeys::WALLET_ADDITIONAL_DETAILS,
        'value' => [
            'errors' => [
                microtime() => 'We\'re having trouble verifying your SSN. Please enter the full 9 digits of your SSN.',
                microtime() => 'We need to ask you a few more questions to finish validating your identity',
            ],
            'errorCode' => self:: WALLET_ERROR_KBA, // This will indicate to the frontend that the user must be redirected to the AnswerQuestionsForWallet step
            'idNumber' => $idNumber,
            'questions' => $questions,
        ],
    ],
];

The errorCode would be overwritten by the second error and while this wouldn't matter too much for this command since it's pattern C but if it were pattern B, we wouldn't want that prop to be overwritten, correct?

So perhaps it would be better if the onyxData looked like:

[
    [
        'onyxMethod' => Onyx::METHOD_MERGE,
        'key' => OnyxKeys::WALLET_ADDITIONAL_DETAILS,
        'value' => [
            'errors' => [
               microtime() => [
                    'error' => 'We\'re having trouble verifying your SSN. Please enter the full 9 digits of your SSN.',
                    'errorCode' => 'ssnError', // We'll use this code on the frontend to alter the ssn field to accept the full 9 digit ssn
                ]
               microtime() => [
                    'error' => 'We need to ask you a few more questions to finish validating your identity',
                    'errorCode' => self:: WALLET_ERROR_KBA, // This will indicate to the frontend that the user must be redirected to the AnswerQuestionsForWallet step
                    'idNumber' => $idNumber,
                    'questions' => $questions,
                ]
            ],
        ],
    ],
];

Thoughts?

@@ -43,14 +43,16 @@ The data will automatically be sent to the user via Pusher.
#### WRITE Response Errors
When there is an error on a WRITE response (`jsonCode!==200`), the error must come back to the client on the HTTPS response. The error is only relevant to the client that made the request and it wouldn't make sense to send it out to all connected clients.

Error messages should be returned and stored as a String under the `error` property. If absolutely needed, additional error properties can be stored under other, more specific fields that sit at the same level as `error`:
Error messages should be returned and stored as an object under the `errors` property, keyed by a [microtime](https://www.php.net/manual/en/function.microtime.php). If absolutely needed, additional error properties can be stored under other, more specific fields that sit at the same level as `errors`:
Copy link
Contributor

Choose a reason for hiding this comment

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

keyed by a microtime

I think you should mention that it is an int, and update it to use Onyx::getErrorMicroTime() instead. We can wait until that PR is merged.

@PauloGasparSv PauloGasparSv dismissed their stale review July 28, 2022 19:08

Waiting for this thread's discussion https://expensify.slack.com/archives/C02HWMSMZEC/p1658356668529449 for an agreement on how to deal with multiple error messages

iwiznia
iwiznia previously approved these changes Jul 28, 2022
PauloGasparSv
PauloGasparSv previously approved these changes Jul 28, 2022
chiragsalian
chiragsalian previously approved these changes Jul 28, 2022
@yuwenmemon
Copy link
Contributor Author

@neil-marcellini updated for your suggestion!

Copy link
Contributor

@neil-marcellini neil-marcellini left a comment

Choose a reason for hiding this comment

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

Looks good, thanks 😄

@yuwenmemon yuwenmemon merged commit 631994f into main Aug 1, 2022
@yuwenmemon yuwenmemon deleted the yuwenmemon-patch-1 branch August 1, 2022 17:24
@OSBotify
Copy link
Contributor

OSBotify commented Aug 1, 2022

✋ 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
Copy link
Contributor

OSBotify commented Aug 1, 2022

🚀 Deployed to staging by @yuwenmemon in version: 1.1.87-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Aug 4, 2022

🚀 Deployed to production by @yuwenmemon in version: 1.1.87-9 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants