[No QA] Create new Edit Card Limit page #45397
Conversation
VickyStash
changed the title
|
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Resolving conflicts... |
# Conflicts: # src/ROUTES.ts # src/SCREENS.ts # src/languages/en.ts # src/languages/es.ts # src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx # src/libs/Navigation/linkingConfig/FULL_SCREEN_TO_RHP_MAPPING.ts # src/libs/Navigation/types.ts
|
@DylanDylann kind bump 🙂 |
|
@DylanDylann are you able to get to this one now, or should we switch in @allgandalf while you work on reviewing #45452? |
|
I am reviewing this PR |
src/pages/workspace/expensifyCard/WorkspaceEditCardLimitPage.tsx
Outdated
Show resolved
Hide resolved
|
@DylanDylann kind bump 🙂 |
|
Sure, I am on it |
| inputID={INPUT_IDS.LIMIT} | ||
| ref={inputCallbackRef} | ||
| /> | ||
| <ConfirmModal |
There was a problem hiding this comment.
NAB, It is better to move ConfirmModal outside the form. But the current code also works well
There was a problem hiding this comment.
@DylanDylann It was easier to pass form values to ConfirmModal this way
| const errors = ValidationUtils.getFieldRequiredErrors(values, [INPUT_IDS.LIMIT]); | ||
|
|
||
| // We only want integers to be sent as the limit | ||
| if (!Number(values.limit) || !Number.isInteger(Number(values.limit))) { |
There was a problem hiding this comment.
NAB: I think we shouldn't allow users type decimal (can't type "." character). It would be better
There was a problem hiding this comment.
The same approach is used on the LimitStep page, see the explanation. So let's keep it this way for now
| const updateCardLimit = (newLimit: string) => { | ||
| setIsConfirmModalVisible(false); | ||
|
|
||
| // TODO: add API call when it's supported https://github.com/Expensify/Expensify/issues/407831 |
There was a problem hiding this comment.
The API endpoint UpdateExpensifyCardLimit is live on staging, can we integrate it as part of this PR? :)
There was a problem hiding this comment.
Sure, I will fo it!
There was a problem hiding this comment.
@MariaHCD Is it possible for me to test this endpoint over the realistic card in the E/App?
There was a problem hiding this comment.
Ah, no, not yet. The backend PR to configure a workspace for the Expensify Card is still WIP so we can't test any issuing cards or updating cards at this point.
We can do full E2E tests once more of the backend pieces are implemented.
There was a problem hiding this comment.
@MariaHCD I've made a call for non-existing card and it returned 200 response. Is it okay?
I expected something with 404 😅
There was a problem hiding this comment.
Integrated UpdateExpensifyCardLimit API call: 5e31125
There was a problem hiding this comment.
Ah, thanks for catching that. Looking at our backend logs, we did throw a 404 but the API didn't rethrow the exception. cc: @nkuoch
# Conflicts: # src/libs/API/parameters/index.ts
There was a problem hiding this comment.
Changes look good to me, I wonder if we need to however add the real api in this PR for now if its not possible to create the real card yet, hence the entire flow is not testable yet.
@MariaHCD I think we should just let this go with mocked data and come back to integration once the card creation flow is integrated, what do you think?
I think we can keep the integration code here for now so it saves us a bit of work later. Is there a benefit to having mocked data here instead? |
# Conflicts: # src/ONYXKEYS.ts # src/ROUTES.ts # src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx # src/libs/Navigation/linkingConfig/FULL_SCREEN_TO_RHP_MAPPING.ts # src/libs/Navigation/types.ts
|
@VickyStash Detail page is ready. Please add logic to navigate to the edit card limit page Screen.Recording.2024-07-19.at.15.08.41.mov |
@DylanDylann This PR includes this update, link Are you sure you have checked on the right branch? |
|
@VickyStash Whoops... Let's me check again |
There was a problem hiding this comment.
Ok lets do this https://github.com/Expensify/App/pull/45397/files
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.10-2 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.0.10-3 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.10-7 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.11-5 🚀
|
|
Coming from the server error, I noticed that you can enter to the view to edit the limit even if there is not card setup: https://staging.new.expensify/settings/workspaces/YOUR_POLICY_ID/expensify-card/1/edit/limit if the form is submitted then, it causes a server error. Should we have implemented something that disables the button or closes the modal if the card with cardID=1 doesn't exist? |
|
Or the standard "NotFound" page when you try to access a page that you can't access or doesn't exist? |
|
That sounds like a good option too |
I think it should be relevant not only for this route but for similar as well: I can implement it during BE integration, WDYT? |
|
Makes sense to me! CC: @mountiny |
Details
Creates new Edit Card Limit page.
Fixed Issues
$ #44326
PROPOSAL: N/A
Tests
Pre-step:
Comment out the feature check in the WorkspaceEditCardLimitPage file, so you can access the screen.
Steps:
/expensify-card/1/edit/limit, for example:https://dev.new.expensify.com:8082/settings/workspaces/YOUR_POLICY_ID/expensify-card/1/edit/limit.Make sure the Card limit page looks as expected.
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop