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

Modify the UpdateMoneyRequestDistance API endpoint #35503

Closed
blimpich opened this issue Jan 31, 2024 · 15 comments
Closed

Modify the UpdateMoneyRequestDistance API endpoint #35503

blimpich opened this issue Jan 31, 2024 · 15 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Hot Pick Ready for an engineer to pick up and run with Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@blimpich
Copy link
Contributor

blimpich commented Jan 31, 2024

Problem

We need the UpdateMoneyRequestDistance endpoint to give the frontend more information in its response in order to complete #34686.

Solution

Three things need to happen:

  1. We need the endpoint to calculate the difference between existing and new waypoints
  2. We need it to explicitly return waypoints that have been removed as null
  3. All non-null waypoints should be moved to the start of the response

i.e.

{
  w0: A
  w1: B (remove)
  w2: C
  w3: D (remove)
}

Should turn into:

{
  w0: A
  w1: C
  w2: null
  w3: null
}

Please see this comment/discussion for more details.

cc: @paultsimura @tgolen

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d6425ace3c091bfc
  • Upwork Job ID: 1752762514809204736
  • Last Price Increase: 2024-01-31
@blimpich blimpich added Engineering Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Bug Something is broken. Auto assigns a BugZero manager. Hot Pick Ready for an engineer to pick up and run with labels Jan 31, 2024
Copy link

melvin-bot bot commented Jan 31, 2024

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

Copy link

melvin-bot bot commented Jan 31, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01d6425ace3c091bfc

Copy link

melvin-bot bot commented Jan 31, 2024

Triggered auto assignment to Contributor Plus for review of internal employee PR - @aimane-chnaif (Internal)

@isabelastisser
Copy link
Contributor

Waiting for proposals.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Feb 5, 2024
@isabelastisser
Copy link
Contributor

Still waiting for proposals.

@melvin-bot melvin-bot bot removed the Overdue label Feb 8, 2024
@aimane-chnaif
Copy link
Contributor

This is marked as Internal.
@blimpich is this being done internally or open for contributors?

@blimpich
Copy link
Contributor Author

blimpich commented Feb 9, 2024

Yup internal. Need an expensify engineer to do this. Not waiting on proposals.

@melvin-bot melvin-bot bot added the Overdue label Feb 12, 2024
Copy link

melvin-bot bot commented Feb 12, 2024

@isabelastisser, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@tgolen tgolen self-assigned this Feb 12, 2024
@melvin-bot melvin-bot bot removed the Overdue label Feb 12, 2024
@tgolen
Copy link
Contributor

tgolen commented Feb 12, 2024

I'm going to grab this one and start on it.

@tgolen
Copy link
Contributor

tgolen commented Feb 12, 2024

@blimpich I was able to create a PR that covers this case and formats the results as expected. I have also made it handle this situation:

The request is created with these waypoints:

{
  w0: A
  w1: B
  w2: C
  w3: D
  w4: E
}

The request is updated to remove B and D and adds an F. This is the resulting Onyx update.

{
  w0: A
  w1: C
  w2: E
  w3: F
  w4: null
}

I hope that's expected.

@tgolen
Copy link
Contributor

tgolen commented Feb 12, 2024

Daily Update

  • The Web-E PR was created and is in review

@tgolen
Copy link
Contributor

tgolen commented Feb 13, 2024

Daily Update

  • The Web-E PR has had two reviews and is nearly ready to merge.

Copy link

melvin-bot bot commented Feb 14, 2024

@tgolen @isabelastisser @aimane-chnaif 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!

@tgolen
Copy link
Contributor

tgolen commented Feb 14, 2024

Daily Update

  • The Web-E PR has been merged and it is on staging.

@tgolen tgolen added the Reviewing Has a PR in review label Feb 14, 2024
@tgolen
Copy link
Contributor

tgolen commented Feb 15, 2024

Daily Update

  • The Web-E PR has been deployed to production.

@tgolen tgolen closed this as completed Feb 15, 2024
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. Daily KSv2 Engineering Hot Pick Ready for an engineer to pick up and run with Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

4 participants