-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Handle blocked copilot and Expensify card flows gracefully #52103
Handle blocked copilot and Expensify card flows gracefully #52103
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@Julesssss draft is up. Quick question - |
One more important note here, For privateProfileOptions fields (legal name,legal address......) following two options came to my mind.
IMO option 2 aligns more with rest of the app. Now again for option 2 there was an important decision about validation.
Thoughts? |
…ocked-copilot-and-Expensify-card-flows-gracefully
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.
We have design approval now, thanks for sharing the questions!
…ocked-copilot-and-Expensify-card-flows-gracefully
I think something like that would work, but I could also see just using the same exact "Not found" page we use elsewhere in terms of the exact copy and text. cc @Expensify/design |
Currently we display either of "Oops, this page cannot be found" or "You don't have access to this chat" for not found page subtitle. |
Got it, so maybe something like: Title: You don't have access to this Or something like that. cc @jamesdeanexpensify for a quick looksie. |
One more input request,
|
Hmm I would think all of the, right? How do the current "not found" pages work? |
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.
LGTM!
Sorry if I'm late here, but I wonder if we should align this modal with the copy suggestion for this page more, for consistency's sake. What do you all think? So then it might be: Page Modal |
Small change not a problem. Change note: But I could notice {accountOwnerEmail} is dropped, Is that intentional? |
It's probably fine not to include {accountOwnerEmail}, but we could also do: Page Modal |
Proceeding with this copy, as latest suggestion/agreement from design team.
|
Hey @c3024, we made a blunder here.
Just now I made those changes, and I need your help for testing. Could you please provide mock data/mock Onyx data for company card feeds so I could test those(I am not yet invited to Slack so I don't have access to any sandbox data). |
I have not found any sandbox data for cards, and AFAIK, no such data is available for cards. Since the Copilot flow is restricted on the initial clicks on most pages, such as issuing pages, we can test these flows without adding any cards. We can include tests for flows that require card data in QA tests. In the unlikely event that QA finds a bug for these flows, they will raise an issue. cc: @Julesssss |
Changes are ready for review again. |
@c3024 could you put this PR on priority please. |
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.
Screenshots/Videos
Android: Native
cardsAndroid.mov
Android: mWeb Chrome
cardsAndroidmWeb.mp4
iOS: Native
cardsiOS.mov
iOS: mWeb Safari
cardsiOSmWeb.MP4
MacOS: Chrome / Safari
cardsChrome.mov
MacOS: Desktop
cardsDesktop.mov
LGTM!
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.
Nice work
We can make some of these test cases new regression tests once the PR is deployed. |
✋ 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/Julesssss in version: 9.0.73-0 🚀
|
This PR is failing for Android because of issue #53812 |
🚀 Deployed to production by https://github.com/luacmartins in version: 9.0.73-8 🚀
|
Explanation of Change
This PR aims to restrict copilots from performing certain actions on behalf of the owner.
As A Part of Solution Primarily Show DelegetNoAccessModal when copilot tries to perform any restricted action
And restrict delegate from accessing any restricted page using deeplink by wrapping those pages with DelegateNoAccessWrapper.
List of Actions Restricted for Copilot on Behalf of Owner,
Fixed Issues
$ #50796
PROPOSAL: NA
Tests
Same As QA.
Offline tests
Same As QA.
QA Steps
Important
All of the following tests should be performed as a copilot.
For Steps To Add Copilot Visit - https://help.expensify.com/articles/expensify-classic/copilots-and-delegates/Assign-or-remove-a-Copilot
Test 1.
Test 2.
Test 3.(For Accounts With Wallet Not Enabled)
Test 4.
Test 5.
Test 6.
Test 7.
Test 8.
staging.newExpensify.com
ornew.expensify.com
fromdev.new.expensify.com:8082
).Please replace a valid workspaceID in following list while testing.
Test 9.
Issue New Card
Button.+ Issue Card
Button.Test 10.
Add Cards
Button.+ Issue Card
Button.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
Test 1-7
Screen.Recording.2024-11-23.at.5.14.42.AM.mov
Test 8
Screen.Recording.2024-11-23.at.5.13.20.AM.mov
Android: mWeb Chrome
Test 1-7
Screen.Recording.2024-11-23.at.6.08.18.AM.mov
Test 8
Screen.Recording.2024-11-23.at.6.04.51.AM.mov
iOS: Native
Test 1-7
Screen.Recording.2024-11-23.at.3.43.54.AM.mov
Test 8
Screen.Recording.2024-11-23.at.4.34.17.AM.mov
iOS: mWeb Safari
Test 1-7
Screen.Recording.2024-11-23.at.3.02.24.AM.mov
Test 8
Screen.Recording.234.mov
MacOS: Chrome / Safari
Test 1-7
Screen.Recording.2024-11-23.at.2.44.44.AM.mov
Test 8
Screen.Recording.123.mov
MacOS: Desktop
Test 1-7
Screen.Recording.2024-11-23.at.4.47.01.AM.mov
Test 8
Screen.Recording.2024-11-23.at.4.44.54.AM.mov