Skip to content
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 Feeds] Clean up tasks for workspace feeds #47308

Closed
8 tasks done
mountiny opened this issue Aug 13, 2024 · 31 comments
Closed
8 tasks done

[Workspace Feeds] Clean up tasks for workspace feeds #47308

mountiny opened this issue Aug 13, 2024 · 31 comments
Assignees
Labels
NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2

Comments

@mountiny
Copy link
Contributor

mountiny commented Aug 13, 2024

Par to of the workspace feeds project.

  • We're showing all the cards assigned to the workspace in the member's details - I think we should show only the cards that are assigned to this particular user
Screenshot 2024-08-21 at 10 26 05
  • When starting the issue card flow right after choosing a bank account, after we issue the card we're redirected back to the Choose Account page. We should be redirected back to Expensify Card page no matter if we start from the Choose Account page or from Expensify Card -> Issue New Card @koko57 PR

  • On a fresh account (with no bank account) when we enable Expensify Card and the Empty state is shown we're having "Finish setup" button instead of "Issue New Card", because we're checking for reimbursementAccountStatus !== CONST.BANK_ACCOUNT.STATE.OPEN and we don't have reimbursementAccountStatus yet, so it's true and we're shown improper text - we want to have Issue New Card, right?

  • When editing card name and card limit offline the new value isn't greyed out. I think we need to adjust it. The remaining limit also be updated wrong in offline (PR) - @VickyStash

Screen.Recording.2024-08-28.at.17.29.35.mov
  • Integrate the isMonthlySettlementAllowed property that is now returned in the OpenPolicyExpensifyCardsPage as part of the card settings isMonthlySettlementAllowed @VickyStash (PR)

  • Go back to the workspace member details page if the card creation from was initiated from there @VickyStash (PR)

  • Integrate the RequestExpensifyCardLimitIncrease API call @koko57 (PR)

  • Make the card issued report actions visible in the chat @koko57 (App PR), Web PR

Copy link

melvin-bot bot commented Aug 13, 2024

Triggered auto assignment to @stephanieelliott (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 13, 2024
@mountiny mountiny moved this to Release 2: Summer 2024 (Aug) in [#whatsnext] #wave-collect Aug 13, 2024
@koko57
Copy link
Contributor

koko57 commented Aug 20, 2024

I'm starting working on this issue

@koko57
Copy link
Contributor

koko57 commented Aug 21, 2024

@mountiny I'm not sure why I missed it, maybe the PR for that was still open back then when I requested creating this ticket, but this feature of skipping the first step is working now 🙈 😅.
We can close this issue or change the title, because I found a few other problems:

  1. We're showing all the cards assigned to the workspace in the member's details - I think we should show only the cards that are assigned to this particular user
Screenshot 2024-08-21 at 10 26 05
  1. When starting the issue card flow right after choosing a bank account, after we issue the card we're redirected back to the Choose Account page. We should be redirected back to Expensify Card page no matter if we start from the Choose Account page or from Expensify Card -> Issue New Card

  2. On a fresh account (with no bank account) when we enable Expensify Card and the Empty state is shown we're having "Finish setup" button instead of "Issue New Card", because we're checking for reimbursementAccountStatus !== CONST.BANK_ACCOUNT.STATE.OPEN and we don't have reimbursementAccountStatus yet, so it's true and we're shown improper text - we want to have Issue New Card, right?

@mountiny so I'm wainitng for your confirmation here 🙂

@melvin-bot melvin-bot bot added the Overdue label Aug 21, 2024
@koko57
Copy link
Contributor

koko57 commented Aug 21, 2024

@mountiny Vicky and me also have found an issue with Card List - after issuing card for a workspace that doesn't have any card yet the card list does not appear. I'm getting an error that an object cannot be merged with an array in Onyx. In Onyx we have an empty array instead of an object in the cards__Expensify Card. We get this empty array as a response from OpenPolicyExpensifyCardsPage when we call it the first time. It is fixed after I log out/log in.

Screenshot 2024-08-21 at 16 40 07 Screenshot 2024-08-21 at 16 40 13

cc @MariaHCD

@mountiny
Copy link
Contributor Author

@koko57 Yeah lets handle the first issues in here, I will update the OP

For the second issue, I know why its happening Auth sends empty object but PHP changes it to associative array

@melvin-bot melvin-bot bot removed the Overdue label Aug 21, 2024
@mountiny mountiny changed the title [Workspace Feeds] Skip the assignee step in the card creation flow from member page [Workspace Feeds] Clean up tasks for workspace feeds Aug 21, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Aug 22, 2024
@koko57
Copy link
Contributor

koko57 commented Aug 22, 2024

@mountiny PR opened #47844

cc @allgandalf @DylanDylann - will you be able to review it?

@koko57
Copy link
Contributor

koko57 commented Aug 22, 2024

@mountiny I've found another problem and I wonder if we should fix it on the FE or the BE - after logging out and logging in again if you go straight to the Workspace -> Members and go into any member details the cards are not displayed. We get them only when going to Expensify Card page as OpenPolicyExpensifyCardsPage response.

Screenshot 2024-08-22 at 11 16 19

So either we should call OpenPolicyExpensifyCardsPage when opening the member's details or members' page or the cards should be returned in OpenWorkspaceMembersPage (or in another call on login).

@mountiny
Copy link
Contributor Author

Very fair, I think we can add it when you go to the OpenWorkspaceMembersPage, I will handle that too

@mountiny
Copy link
Contributor Author

one web pr is up to make sure the cards are returned as empty object

@mountiny
Copy link
Contributor Author

Added one more clean up:

  1. When editing card name and card limit offline the new value isn't greyed out. I think we need to adjust it. The remaining limit also be updated wrong in offline
Screen.Recording.2024-08-28.at.17.29.35.mov

cc @VickyStash

@koko57
Copy link
Contributor

koko57 commented Aug 28, 2024

@mountiny the first one and the third one - the 2nd was actually broken by fixing #46305 (we need another solution, because this one is contradictory to what we want)

Just to make sure: after we choose account and we start the flow we want to go back to the Choose Account Page or back to Expensify Card Page?

@mountiny
Copy link
Contributor Author

@koko57 I am not sure if it is contradictory; I think it's different going back and forth.

If you choose the settlement bank account and start the card creation flow, then if you go back you just go back to pages based on browser history.

If you complete the flow and card is created you should go to the Expensify cards page in this case (if you create the card from the workspace member details page you go back to that page)

@koko57
Copy link
Contributor

koko57 commented Aug 28, 2024

yes, but I mean contradictory to the second point:

When starting the issue card flow right after choosing a bank account, after we issue the card we're redirected back to the Choose Account page. We should be redirected back to Expensify Card page no matter if we start from the Choose Account page or from Expensify Card -> Issue New Card

either we go back to the Choose Account or we're going back to Expensify Card page and as the bank account is chosen we don't show this page again

@mountiny
Copy link
Contributor Author

I see what you mean. It's confusing because the Choose bank account step is essentially the first step in the create card flow in that case, so we do not go back to it once the card is created. That is because its a one-time action in this case as you mentioned

Sorry if I should have caught this earlier and confused you

@koko57
Copy link
Contributor

koko57 commented Aug 29, 2024

One more thing to take care of:

we need to uncomment all the code related to RequestExpensifyCardLimitIncrease command

@VickyStash
Copy link
Contributor

When editing card name and card limit offline the new value isn't greyed out. I think we need to adjust it. The remaining limit also be updated wrong in offline

@DylanDylann @mountiny The PR for this issue is ready for the review

@VickyStash
Copy link
Contributor

The PR for these two:

  • Integrate the isMonthlySettlementAllowed property that is now returned in the OpenPolicyExpensifyCardsPage as part of the card settings isMonthlySettlementAllowed
  • Go back to the workspace member details page if the card creation from was initiated from there

is ready for the review

@koko57
Copy link
Contributor

koko57 commented Aug 30, 2024

@mountiny while working on uncommenting the RequestExpensifyCardLimitIncrease I've encountered another problem - we don't get the bankAccountList after we login - so bankAccountList for Expensify Card page will be not available when we go straight to the Expensify Card page after logging in. Maybe it also could be returned from OpenPolicyExpensifyCardPage?

@mountiny
Copy link
Contributor Author

Yeah we can return that? What is the bug in steps we encounter now without the bank account list? So we can test the fix out later @koko57

@koko57
Copy link
Contributor

koko57 commented Aug 30, 2024

when you go to Expensify Card Page and you have at least one card issued in the remaining limit modal you will see the button that should be displayed only when the settlement bank account was NOT set up with plaid. As the bank account list is undefined the condition will also be false and the button will be displayed. After going to Wallet and find back to Expensify Card Page we have bankAccountList and the button in the remaining limit modal won't be displayed (if ofc the account was set up with plaid)
Screenshot 2024-08-30 at 15 45 49

@koko57
Copy link
Contributor

koko57 commented Aug 30, 2024

@mountiny for the CARDISSUE and other action types not being shown - because the message array is empty isLegacyDeletedComment in isDeletedAction is true and because of that shouldReportActionBeVisible returns false and that message is then filtered out.
Screenshot 2024-08-30 at 16 19 37

so I guess we want to have something in the message array or message.html returned from the API

@koko57
Copy link
Contributor

koko57 commented Aug 30, 2024

and I've found One more thing... - ReconciliationAccountSettingsPage needs UpdateCardSettlementAccount being triggered. I will include this in my PR
Screenshot 2024-08-30 at 16 32 25

for this one:

When starting the issue card flow right after choosing a bank account, after we issue the card we're redirected back to the Choose Account page. We should be redirected back to Expensify Card page no matter if we start from the Choose Account page or from Expensify Card -> Issue New Card

I will use Vicky's solution with backTo param from this PR #48323

@mountiny
Copy link
Contributor Author

mountiny commented Sep 2, 2024

Thanks for the findings, I will try to include the backend fixes for this

@koko57
Copy link
Contributor

koko57 commented Sep 2, 2024

When starting the issue card flow right after choosing a bank account, after we issue the card we're redirected back to the Choose Account page. We should be redirected back to Expensify Card page no matter if we start from the Choose Account page or from Expensify Card -> Issue New Card

@mountiny I see that Vicy has already included this change in her PR,

I will include the fix for #48376 in my PR with Reconciliation setting uncommented

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 2, 2024
@koko57
Copy link
Contributor

koko57 commented Sep 2, 2024

PR opened #48408

@mountiny
Copy link
Contributor Author

mountiny commented Sep 3, 2024

Web PR to show the card related messages merged.

@trjExpensify
Copy link
Contributor

Anything left on this one, or can we close it out?

@mountiny
Copy link
Contributor Author

mountiny commented Sep 9, 2024

I think we can close

@mountiny mountiny closed this as completed Sep 9, 2024
@github-project-automation github-project-automation bot moved this from Release 2: Summer 2024 (Aug) to Done in [#whatsnext] #wave-collect Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2
Projects
Status: Done
Development

No branches or pull requests

6 participants