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

[Simple AA in NewDot] [HIGH] Improve how we handle auto-submission in NewDot #35091

Open
kavimuru opened this issue Jan 24, 2024 · 98 comments
Open
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Jan 24, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: v1.4.33-4
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @tylerkaraszewski
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1706118036323349

Action Performed:

Prerequisites:
use a collect policy
has scheduled submit enabled
has access to the isPolicyExpenseChatEnabled beta.

  1. Request money from the collect workspace
  2. Wait until the report is auto-submitted (i.e end of the day if set to Daily)
  3. Observe the Submit button is still present on the report preview for the submitter, despite it being submitted.
  4. Observe the submitted $x.xx via Scheduled Submit reportAction is not added to the expense report
  5. Sign-in as the approver
  6. Observe the workspace chat is not unread for the approver, the LHN does not contain a GBR green dot and the preview component doesn't contain an approve button.

Expected Result:

  • On auto-submission, the the submit button should be removed.
  • The NewDot reportAction for autoSubmission should be added to the chat from the submitter, the OldDot one from Concierge should not appear in NewDot.
  • On auto-submission of the report, the workspace chat should be unread for the approver, the LHN chat row should have a GBR green dot and an approve button should be in the preview component.

Actual Result:

  • The submit button is visible and clickable
  • The NewDot reportAction is not added to the report, just the OldDot one.
  • The approver isn't notified (chat is not unread, no GBR applied, no approve button on the preview)

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?
All

Screenshots/Videos

Add any screenshot/video evidence

Submit button still visible:
https://github.com/Expensify/App/assets/43996225/f09c69e2-434c-4205-9949-1457cfee28fa

OldDot reportAction appearing not the NewDot one
image

Chat not unread, no GBR on the chat row in the LHN
To be added

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01aa9178aa3b2044ea
  • Upwork Job ID: 1760318902746746880
  • Last Price Increase: 2024-02-21
Issue OwnerCurrent Issue Owner: @Beamanator
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 24, 2024
Copy link

melvin-bot bot commented Jan 24, 2024

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@kavimuru kavimuru added the Needs Reproduction Reproducible steps needed label Jan 24, 2024
@trjExpensify
Copy link
Contributor

@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
@trjExpensify
Copy link
Contributor

trjExpensify commented Jan 29, 2024

Tyler couldn't reproduce this issue. He did run into a different bug with not being able to delete an expense off a processing report, but that's totally different and being figured out over here.

@melvin-bot melvin-bot bot removed the Overdue label Jan 29, 2024
@luacmartins
Copy link
Contributor

I think the issue involves auto-submission. I've noticed that when the report has already been auto-submitted and then I try to submit again. @tylerkaraszewski, do you see any system messages when clicking into the report preview?

@trjExpensify
Copy link
Contributor

Discussion for ref.

@trjExpensify trjExpensify changed the title submit button remains clickable after first submit. [Wave6] Improve how we handle auto-submission in NewDot Jan 30, 2024
@trjExpensify trjExpensify removed the Needs Reproduction Reproducible steps needed label Jan 30, 2024
@trjExpensify
Copy link
Contributor

Spruced this up in-line with the thread discussion. @mountiny are you taking this one?

@mountiny mountiny self-assigned this Jan 30, 2024
@mountiny
Copy link
Contributor

Yes, taking this! Seems like we do send the updates when this comes from harvesting too, will have to dig more

@trjExpensify
Copy link
Contributor

The plot thickens.. Thanks!

@greg-schroeder greg-schroeder changed the title [Wave6] Improve how we handle auto-submission in NewDot [HIGH] Improve how we handle auto-submission in NewDot Jan 31, 2024
@mountiny
Copy link
Contributor

focusing on wave8 critical issue

@mountiny
Copy link
Contributor

mountiny commented Feb 1, 2024

still focusing on ideal nav

@melvin-bot melvin-bot bot added the Overdue label Feb 5, 2024
@mountiny
Copy link
Contributor

mountiny commented Feb 5, 2024

Still focusing on the ideal nav over this one, hopefully I can make progress on this one this week

@melvin-bot melvin-bot bot removed the Overdue label Feb 5, 2024
Copy link

melvin-bot bot commented Feb 7, 2024

@trjExpensify @mountiny this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Feb 7, 2024
@mountiny
Copy link
Contributor

mountiny commented Feb 7, 2024

Still focusing on managing issues in wave8 and fire today

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Feb 7, 2024
Copy link

melvin-bot bot commented Feb 13, 2024

@trjExpensify, @mountiny Eep! 4 days overdue now. Issues have feelings too...

@mountiny
Copy link
Contributor

working at limited capacity - ill

@melvin-bot melvin-bot bot removed the Overdue label Sep 10, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Sep 10, 2024
@Beamanator
Copy link
Contributor

Beamanator commented Sep 10, 2024

PR for "Automatically submitted <amount> via delayed submission" created & ready for review ^ 👍

For auto-reimbursed, I see we use a report action "REIMBURSED" but that isn't used in NewDot yet 🤔 I see there's multiple ways we can create that action (the Reimburse flow in OldDot can lead to multiple variations of messages showing in OldDot).

Anyone know if we're supporting that report action elsewhere or if this is the first place we'll be supporting it? If this is the first place we're supporting it, I wonder if we should also consider how we show that report action in NewDot when manual reimbursement happens in OldDot 🤔

Edit: Asked in slack: https://expensify.slack.com/archives/C06ML6X0W9L/p1726005551948299

@trjExpensify
Copy link
Contributor

For auto-reimbursed, I see we use a report action "REIMBURSED"

Are you saying on OldDot there's no distinct report action for a report paid via automation? 🤔

@Beamanator
Copy link
Contributor

Are you saying on OldDot there's no distinct report action for a report paid via automation? 🤔

Bingo - same with auto-submitted - we reuse report actions that are for the same "action" but we just store specific data in the report actions indicating if the actions were taken manually or automatically - i can link some code if you're interested to see it 🙏

@Beamanator
Copy link
Contributor

Ok so little status update on the auto-reimbursement side of things:

  1. Auto reimbursing in NewDot currently goes through 3 different flows:
    1. Control policy on advanced approvals
      • For this, we had to update the auth token used for auto-reimbursing here
    2. Policies on instant submit
    3. All others
  2. For control policy on advanced approvals, we should be good to go to start modifying the report action 👍
  3. For Policies on Instant Submit we currently call PayMoneyRequest to do auto-payments
    • This command adds the NewDot-needed report action showing the report payments 👍
  4. For "all other auto-submitting" we currently call ReimburseReport to do the auto-payments
    • Sadly, this command does NOT add the NewDot-needed "IOU -> Pay" report action, instead we only add the Reimbursed report action which we plan NOT to support in NewDot - so I need to figure out how to get the "IOU -> Pay" report action added in that flow.

Another point: How do we want to update the Report Preview here, when the report was paid automatically?

  • AFAIK this is what it currently looks like after a report is reimbursed - are we ok with keeping this or do we want to try to make it clear that the report was paid automatically?:
Screenshot 2024-09-13 at 1 52 38 PM

@trjExpensify
Copy link
Contributor

AFAIK this is what it currently looks like after a report is reimbursed - are we ok with keeping this or do we want to try to make it clear that the report was paid automatically?:

No change. 👍

@Beamanator
Copy link
Contributor

That makes my life easy <3

@Beamanator
Copy link
Contributor

Web-E & Auth PRs for auto-reimbursed copy in review ^

@Beamanator
Copy link
Contributor

Beamanator commented Sep 20, 2024

Web-E & Auth PRs are merged 👍 Two App PRs left:

  1. Update "submitted by harvesting" report action copy #48939 - fixing some lint errors & waiting for C+ to finish testing (hard b/c it needs a report to be harvested)
  2. Update "automatically paid" report action copy #49187 - getting close, but waiting for Web-E PR to get to staging/prod

@Beamanator
Copy link
Contributor

Beamanator commented Sep 25, 2024

Ooh @trjExpensify - @mollfpr had a good question: Should we also update the "Submit & Close" report action to read like this?

Shawn Borton
automatically submitted and closed $15.00 via delayed submission

Right now, I believe we don't even show that action in NewDot 😬 which seems like an oversight

Also, I just realized I haven't touched "auto approval" report actions yet, that will come after the above action updates

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

Ooh @trjExpensify - @mollfpr had a good question: Should we also update the "Submit & Close" report action to read like this?

Hm, how does the OldDot one read? I'm not sure if we need to differentiate really, we don't call it "closed" in NewDot.

@Beamanator
Copy link
Contributor

Beamanator commented Sep 26, 2024

Hm, how does the OldDot one read?

We currently just add the "(automatically closed due to submit-only policy)" part in OldDot signifying it's submit & closed

Screenshot 2024-09-26 at 1 33 42 PM

@trjExpensify
Copy link
Contributor

Yeah, interesting. We don't use "closed" terminology, so it would be strange to randomly chuck that in here as it's a foreign concept to a NewDot user. I vote for just using this one for now:

automatically submitted $50.00 via delayed submission

The nextSteps illustrates that it's finished and there's nothing more to be done.

@JmillsExpensify
Copy link

100% agree with Tom.

@Beamanator
Copy link
Contributor

Giggidy 👍 I 100.5% agree with Tom 👍

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

Beamanator commented Sep 30, 2024

Okkkkkkkk little update:

  1. Submitted via delayed submission should be in staging ✅
  2. Paid via workspace rules is very slow to get reviewed 🟡
  3. Submitted and closed (showing same as "Submitted via delayed submission") has PRs in review 🟡
  4. Approved via workspace rules new & In Review 🟡
  5. Same ^ for Forwarding needs a bit of work
    • Web-E & Auth PRs for auth token updates 🟡
    • Then PRs for updating the copy for Forwarded report action (added to this one) 🟡

@Beamanator
Copy link
Contributor

Beamanator commented Sep 30, 2024

Okkk another question about auto-approval 😬

  • In OldDot, report action looks like: Concierge final approved this report on behalf of zz@gmail.com
  • In NewDot, we'll make this read automatically approved $1.00 via workspace rules (detailed here)
  • In NewDot, the Report Preview component (in a workspace chat) currently looks like:
    • Screenshot 2024-09-29 at 9 41 02 PM
    • Question: Do we want this to remain the same? Or include automatic / auto-approved somehow?

Oh and Question 2: Do we need to "optimistically" handle auto-approval & payment in this issue or can we do that in a new one / later? :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
Status: Release 2: Summer 2024 (Aug)
Development

No branches or pull requests