Closed hungvu193 closed 1 week ago
Hold can be lifted. PR merged
please merge latest main
please merge latest main
Sure things. I'm finishing SelectionScreen
before merging with main
@rojiphil 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]
Will be ready for another review @rushatgabhane
Are there two C+ reviewers required here? If not, please remove me as a reviewer here.
Are there two C+ reviewers required here? If not, please remove me as a reviewer here.
Ah , thanks @rojiphil. @rushatgabhane will handle it
If we can prioritize this PR, that would be awesome because it has the share component that will be using across Advance page.
@hungvu193 The screenshots are missing for all platforms, can you add those please?
I addressed the comments and added videos @mananjadhav
Thanks the code changes look fine, will work on the checklist.
@hungvu193 I see two problems when testing.
This view should only be visible if Sync reimbursed reports is enabled. While the menu item visibility works fine, https://dev.new.expensify.com:8082/settings/workspaces/76A10AB364F95DA0/accounting/xero/advanced/invoice-account-selector
if I hit the URL with the sync disabled, I can still see the page and able to update the setting as well.
I am not sure if it's a navigation issue/related to this PR, but at times I don't see /invoice-account-selector
in the URL. The page loads on click but ends at /xero/advanced
or /import
if I have navigated to import earlier.
@hungvu193 I see two problems when testing.
- This view should only be visible if Sync reimbursed reports is enabled. While the menu item visibility works fine,
https://dev.new.expensify.com:8082/settings/workspaces/76A10AB364F95DA0/accounting/xero/advanced/invoice-account-selector
if I hit the URL with the sync disabled, I can still see the page and able to update the setting as well.- I am not sure if it's a navigation issue/related to this PR, but at times I don't see
/invoice-account-selector
in the URL. The page loads on click but ends at/xero/advanced
or/import
if I have navigated to import earlier.
I can't reproduce the 2
. With the first one, we need to implement a new props for our AccessOrNotFoundWrapperProps
, currently it's only showing NotFoundPage bases on accessVariants
and featureName
.
About 2, even I don't have proper reproduction steps. For 1, do we have any workaround? I implemented it by not passing the policyID in the categories page. But we can let @lakchote @mountiny confirm if we need it in this PR. I'll continue with the checklist in the current state.
For 1, do we have any workaround?
I'm trying to update AccessOrNotFoundWrapper
but I'm facing the hard issue "naming the variable" π
For 1, do we have any workaround?
I'm trying to update
AccessOrNotFoundWrapper
but I'm facing the hard issue "naming the variable" π
But are you like adding a boolean prop, which will show the NotFound if it's true? along with feature names and variants? We'll need it to be generic as the conditions would be different for some of the pages.
For 1, do we have any workaround?
I'm trying to update
AccessOrNotFoundWrapper
but I'm facing the hard issue "naming the variable" πBut are you like adding a boolean prop, which will show the NotFound if it's true? along with feature names and variants? We'll need it to be generic as the conditions would be different for some of the pages.
Yeah, I want to add a new boolean props, ie: shouldBeBlocked
and update this condition:
const shouldShowNotFoundPage = isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors)) || !policy?.id || !isPageAccessible || !isFeatureEnabled;
to
const shouldShowNotFoundPage = isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors)) || !policy?.id || !isPageAccessible || !isFeatureEnabled || shouldBeBlocked;
I think shouldBeBlocked
should be okay as showShowNotFound
is already used in the component and it would lead to confusion.
OK, let me update it then
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components using Avatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. for onClick={this.submit}
the method this.submit
should be bound to this
in the constructor)this
are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this);
if this.submit
is never passed to a component event handler like onClick
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified that Avatar
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 the Test
steps.It should be working now.
https://github.com/Expensify/App/assets/16502320/8cfe7911-57fd-4e3a-a37d-b7350b6bac5d
Tested the block access change. Added the screencast for Web.
We did not find an internal engineer to review this PR, trying to assign a random engineer to #39752 as well as to this PR... Please reach out for help on Slack if no one gets assigned!
@Expensify/design could you please give it a look before merging?
Unassigning other reviewers as reviews have already been done by 2 C+ contributors.
Design-wise I think this is looking good.
:hand: 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 production by https://github.com/marcaaron in version: 1.4.71-6 π
platform | result |
---|---|
π€ android π€ | success β |
π₯ desktop π₯ | success β |
π iOS π | success β |
πΈ web πΈ | success β |
Details
Fixed Issues
$ https://github.com/Expensify/App/issues/39752 PROPOSAL: N/A
Tests
Precondition: User connected to Xero and had bank accounts.
Xero Invoice Collections Account
in Advance page.Offline tests
Same as Tests.
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
https://github.com/Expensify/App/assets/16502320/79070bd8-59d0-4a93-a3fc-e6b9ee0a4d91Android: mWeb Chrome
iOS: Native
https://github.com/Expensify/App/assets/16502320/f66a1f6e-6ecf-488d-b9ee-73c5e995b681iOS: mWeb Safari
https://github.com/Expensify/App/assets/16502320/367f7e03-f0e0-4373-961a-a0988b25d297MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/16502320/373ead5b-c49e-4505-a9af-cbbf84934b10MacOS: Desktop
https://github.com/Expensify/App/assets/16502320/742ecbc3-0d07-4a60-9ae7-9bac68ee1b6b