-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Workspace Feed - Fix export item issues #52180
Workspace Feed - Fix export item issues #52180
Conversation
@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] |
It isn't related to our PR but it is minor. Could you please fix it on this PR? |
@@ -33,7 +33,7 @@ function WorkspaceCompanyCardAccountSelectCardPage({route}: WorkspaceCompanyCard | |||
const workspaceAccountID = PolicyUtils.getWorkspaceAccountID(policyID); | |||
const [searchText, setSearchText] = useState(''); | |||
|
|||
const [allBankCards] = useOnyx(`${ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST}${workspaceAccountID}_${bank}`); | |||
const [allBankCards] = useOnyx(`${ONYXKEYS.CARD_LIST}`); |
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.
@narefyev91 Could you explain this change? I think using ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST is correct on this page. please correct me If I am wrong
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.
@DylanDylann on src/pages/workspace/companyCards/WorkspaceCompanyCardDetailsPage.tsx - we show information which is coming from here:
That's why any changes using ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST was not shown on WorkspaceCompanyCardDetailsPage - because we are not using that key for showing card details
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.
@narefyev91 Could you please merge main and check again? It has been updated recently
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@narefyev91 For QA tests, please update step 7:
Screen.Recording.2024-11-11.at.11.43.05.mov |
@narefyev91 One minor comment, the rest looks fine |
@DylanDylann updated PR. |
@narefyev91 Please complete the checklist ![]() |
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.
Thanks! Lets now starting thinking how to for each pr add unit tests to catch this regression/ or gatekeep new feature behaviour
@narefyev91 also you have migrated the taxPicker to useOnyx, can you add a test that will cover that component? |
@mountiny I will complete the checklist in the linked issues |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Tax rate component test (based on useOnyx hook): Screen.Recording.2024-11-12.at.14.56.01.mov |
@narefyev91 can you please add the steps to the QA/ test so QA can run through it? |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.61-0 🚀
|
For this one , we are now showing a default selection in the row, but it is not ![]() ![]() |
@joekaufmanexpensify Not sure if i get to be honest. |
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.61-3 🚀
|
data = getNetSuitePayableAccountOptions(policy ?? undefined, config?.payableAcct); | ||
title = companyCard?.nameValuePairs?.netsuite_export_payable_account ?? data.find((exportPayable) => exportPayable.isSelected)?.text; |
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.
This line introduces: #55753
More details: #55753 (comment)
Explanation of Change
Fixing some issues related to Export account in Company Card:
For mocking qbo connection can be used:
Fixed Issues
$ #51891, #51880, #51887
PROPOSAL:
Tests
Tax picker QA test:
Pre-condition:
Tax is enabled.
Distance rates are enabled.
Track tax in DIstance rates is enabled.
Offline tests
No changes
QA Steps
Tax picker QA test:
Pre-condition:
Tax is enabled.
Distance rates are enabled.
Track tax in DIstance rates is enabled.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
Vendor Bill:

MacOS: Desktop
desktop.mov