-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Start using new new RequestCall Onyx-optimized API command instead of Inbox_CallUser #9573
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
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
fe6c2d4
Add use new requestCall Onyx-optimized API command
yuwenmemon b9df3b1
Make sure that we reset the errorMessage when opening the Request Cal…
yuwenmemon 7c4639c
Update copy
yuwenmemon 6e5b8ee
Merge branch 'main' of github.com:Expensify/App into yuwen-requestCall
yuwenmemon 76629f3
Don't neccesistate the presence of onyxData in a response in order to…
yuwenmemon 68ba343
Merge branch 'main' of github.com:Expensify/App into yuwen-requestCall
yuwenmemon b07fec9
Merge branch 'main' of github.com:Expensify/App into yuwen-requestCall
yuwenmemon 1b496ac
Fix tests
yuwenmemon d6fa52d
Spanish
yuwenmemon 6339767
More Spanish
yuwenmemon 880007d
More spanish
yuwenmemon 26b0111
errorMessage -> error
yuwenmemon dc47777
Merge branch 'main' of github.com:Expensify/App into yuwen-requestCall
yuwenmemon f3f6aab
Use FormAlertWithSubmitButton to enforce offline mode
yuwenmemon c2643fe
Merge branch 'main' of github.com:Expensify/App into yuwen-requestCall
yuwenmemon 60b381c
PR Comments - use property in requestCallForm Onyx key rather than gl…
yuwenmemon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,57 +1,81 @@ | ||
| import Onyx from 'react-native-onyx'; | ||
| import ONYXKEYS from '../../ONYXKEYS'; | ||
| import * as API from '../API'; | ||
| import * as DeprecatedAPI from '../deprecatedAPI'; | ||
| import Growl from '../Growl'; | ||
| import * as Localize from '../Localize'; | ||
| import Navigation from '../Navigation/Navigation'; | ||
| import * as User from './User'; | ||
|
|
||
| /** | ||
| * Requests a call from Guides | ||
| * | ||
| * @param {Object} params | ||
| * @param {String} taskID | ||
| * @param {String} policyID | ||
| * @param {String} firstName | ||
| * @param {String} lastName | ||
| * @param {String} phoneNumber | ||
| * @param {String} params.taskID | ||
| * @param {String} params.policyID | ||
| * @param {String} params.firstName | ||
| * @param {String} params.lastName | ||
| * @param {String} params.phoneNumber | ||
| * @param {String} params.phoneNumberExtension | ||
| */ | ||
| function requestInboxCall({ | ||
| function requestCall({ | ||
|
marcaaron marked this conversation as resolved.
|
||
| taskID, policyID, firstName, lastName, phoneNumber, phoneNumberExtension, | ||
| }) { | ||
| Onyx.merge(ONYXKEYS.REQUEST_CALL_FORM, {loading: true}); | ||
| DeprecatedAPI.Inbox_CallUser({ | ||
| policyID, | ||
| firstName, | ||
| lastName, | ||
| phoneNumber, | ||
| phoneNumberExtension, | ||
| taskID, | ||
| }) | ||
| .then((result) => { | ||
| if (result.jsonCode === 200) { | ||
| Growl.success(Localize.translateLocal('requestCallPage.growlMessageOnSave')); | ||
| Navigation.goBack(); | ||
| return; | ||
| } | ||
| const optimisticData = [{ | ||
| onyxMethod: 'merge', | ||
| key: ONYXKEYS.REQUEST_CALL_FORM, | ||
| value: { | ||
| loading: true, | ||
|
marcaaron marked this conversation as resolved.
|
||
| }, | ||
| }]; | ||
|
|
||
| if (result.jsonCode === 666) { | ||
| // The fact that the API is returning this error means the BLOCKED_FROM_CONCIERGE nvp in the user details has changed since the last time we checked, so let's update | ||
| User.getUserDetails(); | ||
| } | ||
| const successData = [ | ||
| { | ||
| onyxMethod: 'merge', | ||
| key: ONYXKEYS.REQUEST_CALL_FORM, | ||
| value: { | ||
| loading: false, | ||
| error: '', | ||
| didRequestCallSucceed: true, | ||
| }, | ||
| }, | ||
| ]; | ||
|
|
||
| // Phone number validation is handled by the API | ||
| Growl.error(result.message, 3000); | ||
| }) | ||
| .finally(() => { | ||
| Onyx.merge(ONYXKEYS.REQUEST_CALL_FORM, {loading: false}); | ||
| }); | ||
| const failureData = [{ | ||
| onyxMethod: 'merge', | ||
| key: ONYXKEYS.REQUEST_CALL_FORM, | ||
| value: { | ||
| loading: false, | ||
| }, | ||
| }]; | ||
|
|
||
| API.write( | ||
| 'RequestCall', | ||
| { | ||
| policyID, | ||
| firstName, | ||
| lastName, | ||
| phoneNumber, | ||
| phoneNumberExtension, | ||
| taskID, | ||
| }, | ||
| {optimisticData, successData, failureData}, | ||
| ); | ||
| } | ||
|
|
||
| function openRequestCallPage() { | ||
| API.read('OpenRequestCallPage'); | ||
| // Reset the error message in case we had one set from a previous failed attempt at requesting a call. | ||
| const optimisticData = [{ | ||
| onyxMethod: 'merge', | ||
| key: ONYXKEYS.REQUEST_CALL_FORM, | ||
| value: { | ||
| error: '', | ||
| }, | ||
| }]; | ||
| API.read('OpenRequestCallPage', {}, {optimisticData}); | ||
| } | ||
|
|
||
| function clearDidRequestCallSucceed() { | ||
| Onyx.merge(ONYXKEYS.REQUEST_CALL_FORM, {didRequestCallSucceed: false}); | ||
| } | ||
|
|
||
| export { | ||
| requestInboxCall, | ||
| openRequestCallPage, | ||
| requestCall, | ||
| clearDidRequestCallSucceed, | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import React from 'react'; | ||
| import {Image, View} from 'react-native'; | ||
| import Text from '../components/Text'; | ||
| import withLocalize, {withLocalizePropTypes} from '../components/withLocalize'; | ||
| import styles from '../styles/styles'; | ||
| import confettiPop from '../../assets/images/confetti-pop.gif'; | ||
| import Button from '../components/Button'; | ||
| import FixedFooter from '../components/FixedFooter'; | ||
| import Navigation from '../libs/Navigation/Navigation'; | ||
|
|
||
| const propTypes = { | ||
| ...withLocalizePropTypes, | ||
| }; | ||
|
|
||
| const RequestCallConfirmationScreen = props => ( | ||
| <> | ||
| <View style={[styles.screenCenteredContainer, styles.alignItemsCenter]}> | ||
| <Image | ||
| source={confettiPop} | ||
| style={styles.confettiIcon} | ||
| /> | ||
| <Text | ||
| style={[ | ||
| styles.textStrong, | ||
| styles.textLarge, | ||
| styles.mb2, | ||
| ]} | ||
| > | ||
| {props.translate('requestCallConfirmationScreen.callRequested')} | ||
| </Text> | ||
| <Text style={styles.textAlignCenter}> | ||
| {props.translate('requestCallConfirmationScreen.allSet')} | ||
| </Text> | ||
| </View> | ||
| <FixedFooter> | ||
| <Button | ||
| success | ||
| text={props.translate('requestCallConfirmationScreen.gotIt')} | ||
| style={styles.mt6} | ||
| pressOnEnter | ||
| onPress={() => Navigation.goBack()} | ||
| /> | ||
| </FixedFooter> | ||
| </> | ||
| ); | ||
|
|
||
| RequestCallConfirmationScreen.propTypes = propTypes; | ||
| RequestCallConfirmationScreen.displayName = 'RequestCallConfirmationScreen'; | ||
|
|
||
| export default withLocalize(RequestCallConfirmationScreen); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @yuwenmemon
I'm not sure if this case matters, but if a requests hits the back end with a command that is not implemented, the backend returns a 404. I would have expected that the
failureDatato be pushed into Onyx, but it isn't becauseresponseDatais undefined so I ended up without feedback in the UI. Could this happen in a real case? like if we try to send a request about a workspace or a chat that doesn't exist anymore?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean because the response for that case does not come via
jsonCode: https://github.com/Expensify/Web-Expensify/blob/30edfabd582f9ac70ec59cef3172a5bf25657efa/api.php#L2761-L2766Maybe we can add a parameter that indicates if this call to the Middleware is coming from a failed retry request or not? Maybe checking the retryCount? Or check the HTTP response code somehow? The original issue was that if a request was a failed retry (i.e. could not connect to the Server), we'd have no
responseDataand thus get an undefined error.However I don't really think this case matters too much right? Why would we want to support error messages for non-existent API commands? We shouldn't be calling non-existent API commands in the production App ever, right? Any "feedback in the UI" would solely be serving developers and that's it, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree that this doesn't matter if it only happens when a command doesn't exist, but I had my doubt if this could happen with other crashes like and exception being thrown. I'll test and let you know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, retested throwing an exception in the web API, we get a
jsonCode = 666,responseDatais not undefined, and thefailureDatais applied correctly. I think this is the case that matters and works fine.. so lets do nothing :)