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

[DISTANCE] [Tracking] LOW: Refactor UpdateDistanceRequest in Auth/PHP/App to follow 1:1:1 pattern and split it out per field #28358

Closed
8 of 9 tasks
tgolen opened this issue Sep 28, 2023 · 14 comments
Assignees
Labels
Distance Wave5-free-submitters Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item.

Comments

@tgolen
Copy link
Contributor

tgolen commented Sep 28, 2023

Problem

The API command UpdateDistanceRequest in PHP was created to take the entire transaction object and validate/save it. This lead to the UI re-using the same command for multiple actions: updating amount, updating description, updating category, etc.. This is bad because when editing the description, the API command expects the entire object to be passed and be valid. If there are invalid waypoints, for example, you cannot update the description. This leads to lots of front-end hacks to get around issues related to errors.

Solution

Split the command out into field specific commands:

cc @neil-marcellini @Gonals

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010ea3e1f42f2fd6dd
  • Upwork Job ID: 1707222752004390912
  • Last Price Increase: 2023-09-28
@tgolen tgolen added Engineering Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item. labels Sep 28, 2023
@tgolen tgolen self-assigned this Sep 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

Job added to Upwork: https://www.upwork.com/jobs/~010ea3e1f42f2fd6dd

@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

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

@tgolen
Copy link
Contributor Author

tgolen commented Sep 28, 2023

@allroundexperts There won't be anything for you to do on this for a while since I will be working on the server APIs. Once those are done, I'll have to implement it in the UI, which I will then need help on and I'll get it auto-assigned at that time.

@kevinksullivan for the same reason, I won't need you assigned to this for now either. Can you please remove the upwork job?

@tgolen tgolen removed their assignment Sep 28, 2023
@kevinksullivan
Copy link
Contributor

Closed the job 👍

@tgolen
Copy link
Contributor Author

tgolen commented Oct 19, 2023

I'm going to split these into individual issues as I work on them. Each field is going to need an Auth PR, a Web-E PR, and an App PR. First issue is #29993 for date.

@tgolen
Copy link
Contributor Author

tgolen commented Oct 20, 2023

After discussing this with @cead22 in Slack we decided to narrow the scope of this refactoring to PHP only and to leave the Auth command as it is. That should work. It will mean that we need to call Auth to get the full transaction first, then modify the single field that the frontend is updating, and pass the entire transaction to EditMoneyRequest.

@tgolen tgolen changed the title Refactor EditDistanceRequest in auth to follow 1:1:1 pattern and split it out per field Refactor UpdateDistanceRequest in PHP to follow 1:1:1 pattern and split it out per field Oct 20, 2023
@tgolen
Copy link
Contributor Author

tgolen commented Oct 27, 2023

OK, nevermind on that last update. Additional comments in that slack thread with @flodnv and @iwiznia made it a little more clear that we should probably do this all the way down in Auth, so I'm going to stick with that original plan. I should be able to get back working on this on Monday.

@tgolen tgolen changed the title Refactor UpdateDistanceRequest in PHP to follow 1:1:1 pattern and split it out per field Refactor UpdateDistanceRequest in Auth/PHP/App to follow 1:1:1 pattern and split it out per field Nov 1, 2023
@melvin-bot melvin-bot bot added the Overdue label Nov 6, 2023
@tgolen
Copy link
Contributor Author

tgolen commented Nov 6, 2023

This is still moving along at a weekly pace. I'm actually going to move this to monthly since it's the tracking issue and doesn't need updated as frequently.

@melvin-bot melvin-bot bot removed the Overdue label Nov 6, 2023
@tgolen tgolen added Monthly KSv2 and removed Weekly KSv2 labels Nov 6, 2023
@tgolen tgolen changed the title Refactor UpdateDistanceRequest in Auth/PHP/App to follow 1:1:1 pattern and split it out per field [Tracking] Refactor UpdateDistanceRequest in Auth/PHP/App to follow 1:1:1 pattern and split it out per field Nov 6, 2023
@dylanexpensify dylanexpensify changed the title [Tracking] Refactor UpdateDistanceRequest in Auth/PHP/App to follow 1:1:1 pattern and split it out per field [DISTANCE] [Tracking] LOW: Refactor UpdateDistanceRequest in Auth/PHP/App to follow 1:1:1 pattern and split it out per field Nov 7, 2023
@dylanexpensify dylanexpensify added the Distance Wave5-free-submitters label Nov 7, 2023
@tgolen
Copy link
Contributor Author

tgolen commented Nov 30, 2023

Split out all the tasks into their own issues today. The first task has 1 PR merged (should be merged tomorrow), 1 in review, and 1 in draft still.

@tgolen
Copy link
Contributor Author

tgolen commented Jan 2, 2024

This is coming along pretty nicely!

@tgolen
Copy link
Contributor Author

tgolen commented Feb 5, 2024

This is on track to finish soon. Most issues have been closed and just waiting on one last one.

@trjExpensify
Copy link
Contributor

👋 Is this done? Moving it to #wave-collect polish in the meantime.

@tgolen
Copy link
Contributor Author

tgolen commented Mar 8, 2024

Looks like this is all done! 🎉

@tgolen tgolen closed this as completed Mar 8, 2024
@github-project-automation github-project-automation bot moved this from Polish to Done in [#whatsnext] #wave-collect Mar 8, 2024
@melvin-bot melvin-bot bot removed Overdue labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Distance Wave5-free-submitters Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item.
Projects
No open projects
Development

No branches or pull requests

5 participants