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] LOW: UpdateDistanceRequest 1:1:1 - UpdateMoneyRequestDescription - Split out into its own command #32314

Closed
Tracked by #28358
tgolen opened this issue Nov 30, 2023 · 12 comments
Assignees
Labels
Distance Wave5-free-submitters Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@tgolen
Copy link
Contributor

tgolen commented Nov 30, 2023

There will be three pieces to this:

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0166ac586517ecf2dd
  • Upwork Job ID: 1730346767459143680
  • Last Price Increase: 2023-11-30
@tgolen tgolen changed the title UpdateDistanceRequest 1:1:1 - UpdateRequestDescription - Split out into its own command [DISTANCE] LOW: UpdateDistanceRequest 1:1:1 - UpdateRequestDescription - Split out into its own command Nov 30, 2023
@tgolen tgolen changed the title [DISTANCE] LOW: UpdateDistanceRequest 1:1:1 - UpdateRequestDescription - Split out into its own command [DISTANCE] LOW: UpdateDistanceRequest 1:1:1 - UpdateMoneyRequestDescription - Split out into its own command Nov 30, 2023
@tgolen tgolen added Engineering Weekly KSv2 Internal Requires API changes or must be handled by Expensify staff Distance Wave5-free-submitters labels Nov 30, 2023
Copy link

melvin-bot bot commented Nov 30, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0166ac586517ecf2dd

Copy link

melvin-bot bot commented Nov 30, 2023

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

@chiragsalian chiragsalian self-assigned this Nov 30, 2023
@chiragsalian
Copy link
Contributor

Handing this over to @NikkiWines. I have an auth PR in draft and you can continue from it. it seems to work fine in my testing. I tested it via a test.php file having,

$params = [
    "authToken" => "<authToken>",
    "transactionID" => "2390656467468356405", // a transaction the user has access to
    "modifiedExpenseReportActionID" => 8299677307715651178, // a random reportActionID, with every test call increment this by 1.
    "comment" => "test one five",
];

Auth::updateMoneyRequestDescription($params); // create a simple auth updateMoneyRequestDescription method to call the auth endpoint.

and confirmed it updates the comment just fine.

sqlite> select comment from transactions where transactionID = 2390656467468356405;
comment
----------------------------------------------
{"comment":"test one five","test":"something"}

i added the test something to confirm other parts of the comment are not lost and just the comment bit is updated.
the report action looks fine on newDot too.
image

So things remaining

  1. Auth tests
  2. Web and newDot code to split it into its own command.

Hit me up on newDot if you have any questions.

@chiragsalian
Copy link
Contributor

oh i just remembered another remaining item and i.e., to maybe return or send a push notification of commentJSON so that the client gets the changes.

@melvin-bot melvin-bot bot added the Overdue label Dec 18, 2023
@NikkiWines
Copy link
Contributor

Planning on tackling this during the week

@melvin-bot melvin-bot bot removed the Overdue label Dec 18, 2023
@NikkiWines
Copy link
Contributor

Auth PR is ready for review, will follow up with Web-E and App PRs tomorrow

@melvin-bot melvin-bot bot added the Overdue label Dec 29, 2023
@NikkiWines
Copy link
Contributor

Auth PRs been updated, Web-E and App PRs are WIPs!

@melvin-bot melvin-bot bot removed the Overdue label Jan 3, 2024
@NikkiWines NikkiWines added Reviewing Has a PR in review Improvement Item broken or needs improvement. labels Jan 5, 2024
@NikkiWines
Copy link
Contributor

NikkiWines commented Jan 8, 2024

@NikkiWines
Copy link
Contributor

Noting here that we'll need to remove this comment before this issue can be fully closed out. @tgolen since all of the UpdateMoneyRequest<X> commands will have this same comment, is there an issue that covers updating these instances so they don't stay as perpetual todos in our code?

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Jan 10, 2024

is there an issue that covers updating these instances so they don't stay as perpetual todos in our code?

Maybe we should create a single issue to deal with all of them together later. I'm just guessing that after moving the logic needed from PHP to cpp for one, fixing the others will be trivial.

I would close all these individual separate GH issues after creating the proposed GH issue.

@tgolen
Copy link
Contributor Author

tgolen commented Jan 10, 2024

Ah, that's a good point. I think that comment should be removed from the code and we should create a GH issue to move those updates to Auth at some point. I don't know when it will be valuable to move them.

I agree with Aldo that once the work is done to move the updates for one command, that will be 99% of the work.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jan 12, 2024
@NikkiWines
Copy link
Contributor

This is all done!

@github-project-automation github-project-automation bot moved this from Release 5: Best in Class to Done in [#whatsnext] Wave 05 - Deprecate Free Jan 23, 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 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Development

No branches or pull requests

5 participants