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

[Attendee Tracking] Create 'UpdateMoneyRequestAttendees' command to replace 'EditMoneyRequest' #49326

Closed
Julesssss opened this issue Sep 17, 2024 · 17 comments
Assignees
Labels
Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@Julesssss
Copy link
Contributor

Julesssss commented Sep 17, 2024

Problem

The client functionality is working on the feature branch. With our backend changes also deployed we can now focus on hooking them up. But questions have been raised about the request method.

We need to create a separate UpdateMoneyRequestAttendees command for the NewDot client. This will then need to be used by the client.

We also need to ensure recent attendee logic is passed payerAccountID from front-end.

Solution

Create the command, hook up front-end

@Julesssss Julesssss added Engineering Weekly KSv2 Internal Requires API changes or must be handled by Expensify staff labels Sep 17, 2024
@Julesssss Julesssss self-assigned this Sep 17, 2024
@Julesssss Julesssss changed the title [Attendee Tracking] Enable nameValuePairs param to be used for distance requests and edits [Attendee Tracking] Create 'UpdateMoneyRequestAttendees' command to replace 'EditMoneyRequest' Sep 18, 2024
@Julesssss
Copy link
Contributor Author

I don't have much progress to share, but I made a start on this issue today.

@Julesssss Julesssss added Daily KSv2 and removed Weekly KSv2 labels Sep 23, 2024
@Julesssss
Copy link
Contributor Author

I need to halt again to deal with the HybridApp release fire again.

I have WIP PRs for the Auth and Web-E changes.

@Julesssss
Copy link
Contributor Author

I'm pretty much finished with the HybridApp fire and helping with a smaller fire for someone OOO. Will be continuing with this soon

@melvin-bot melvin-bot bot added the Overdue label Sep 27, 2024
Copy link

melvin-bot bot commented Sep 30, 2024

@Julesssss Huh... This is 4 days overdue. Who can take care of this?

@Julesssss
Copy link
Contributor Author

PRs in progress

@melvin-bot melvin-bot bot removed the Overdue label Oct 1, 2024
@Julesssss
Copy link
Contributor Author

Making progress on the Web-E and Auth PRs.

For my reference, here is the test JSON I need to mock the client param.

@Julesssss
Copy link
Contributor Author

I'm making good progress in both Web and Auth. Currently I'm able to update attendees from NewDot using the brand new API. Next step is to clean up the custom SQL query and then the wider command that persists attendees to the transaction.

Screenshot 2024-10-03 at 14 13 44

@melvin-bot melvin-bot bot added the Overdue label Oct 7, 2024
@Julesssss
Copy link
Contributor Author

I'm currently building the new command logic. Will be continuing on this tomorrow and for the rest of the week.

@melvin-bot melvin-bot bot removed the Overdue label Oct 8, 2024
@Julesssss
Copy link
Contributor Author

Today I dealt with a an issue where the client wouldn't pass the attendees data. I also had to take a step back and figure out which data structure to use in Auth. Tomorrow I will continue with the following logic:

  • Parse new and old NVP data
  • Get new and old attendees
  • Compare, return if necessary
  • If new attendees differ, merge with old NVP data
  • THEN and only then, can we set to the DB with a query

@Julesssss
Copy link
Contributor Author

I completed the above logic today and passed attendees are persisted to the DM.

Next I need to create the system action. Current task is to figure out why the updated action wasn't detected:
[alrt] No modified expense action found after editing a money request from NewDot. This should not happen. transactionThreadReportID: 7647575411595398, modifiedExpenseReportActionID: 6520754376852005010

I may need to do this: // Create the modified expense report action and add it to the report from EditmoneyRequest

@melvin-bot melvin-bot bot added the Overdue label Oct 14, 2024
@Julesssss
Copy link
Contributor Author

Next I need to create the system action.

I have got the system action mostly working. Also, I realised I need the cast the attendees to an array before passing it back to client, or to the 'update recent attendees NVP' logic. I'll continue with this tomorrow.

@melvin-bot melvin-bot bot removed the Overdue label Oct 14, 2024
@Julesssss
Copy link
Contributor Author

As mentioned here, the project will be held for a bit.

This issue is close to completion though and I'll get it merged first. I have the backend PRs in draft review:

@Julesssss
Copy link
Contributor Author

Auth PR tests have been created and I'm just waiting on a query timing issue.

@Julesssss
Copy link
Contributor Author

Julesssss commented Oct 18, 2024

Auth PR is in review. The new query has been timed and automated tests have been added.

@melvin-bot melvin-bot bot added the Overdue label Oct 21, 2024
@Julesssss Julesssss added the Reviewing Has a PR in review label Oct 21, 2024
@melvin-bot melvin-bot bot removed the Overdue label Oct 21, 2024
@Julesssss
Copy link
Contributor Author

Auth PR merged, Web-E PR just needs an automated test before it can be submitted

@Julesssss
Copy link
Contributor Author

Web-E shows that a couple of Onyx fields are not returned from Onyx. I'm not yet sure whether this can wait until the feature is picked back up, or if I should resolve this now and delay other priorities...

@Julesssss
Copy link
Contributor Author

In order to move forward I am holding the Web-E PR. THe changes aren't too complex and should be easy to bring up to date once the Attendee Tracking feature is being worked on again.

This 'create new command' issue can be closed as the Auth command is merged and has been deployed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering 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

1 participant