-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Plan and implement a system for optimistic report title formula computation #59689
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
Comments
Triggered auto assignment to Contributor-plus team member for initial proposal review - @akinwale ( |
Triggered auto assignment to @bfitzexpensify ( |
@neil-marcellini Hi, I'm Hubert from Callstack - expert contributor group - and I would like to take a look at this issue. |
Ok, you're assigned. Please post an initial plan, either in a GH comment or a google doc if that's easier. |
High-Level StrategyWe need a front-end system that:
Phase 1 - foundation
Phase 2 - integration
Phase 3 - quality assurance
Phase 4 - continous integration
|
@neil-marcellini Let me know if I can describe anything in more clear fashion. 🌴 Please be informed that I will be off on vacation since Wednesday until Monday. I can start this first thing after my return |
Thanks @sosek108, that's a pretty good start to the plan. I will look at classic code to see if there's any existing JS code that parses report formulas. I don't think there is since classic doesn't do optimistic updates, buy maybe. If not, I will copy some of the existing backend implementation to a private google doc and share it with you for reference to design the frontend system. I like your idea to create a dependency mapping. Here is a small example of that which I recently made for a couple formulas.
Keep in mind that report formulas should not update if the report has been approved, which also means that the reimbursement related formulas aren't really supported. We have an issue to decide if we're going to deprecate that or fix it, but we'll leave as is for now. Here is the google doc with the JS code from classic. We only really care about the report fields in this case, expense can be ignored. Would you please go into more detail about the middleware / optimistic update system and how it will work? Maybe middleware is the wrong approach since we only need to handle optimistic updates. Sorry if I mislead you there. Perhaps it should just be a layer that subscribes to various Onyx collections and makes updates as needed? Or maybe adjusting utils to build optimistic values will be enough? Let's take the formula part For the beta, let's start with a hard coded list of emails to which the beta applies. It's slightly different than normal betas but that's the approach we're using on the backend for efficiency, since the value needs to be checked on many requests.
We already have this under workspace rules, which is admittedly a bit hard to find. ![]() Please take all that into consideration and update your plan in the original GH comment. |
Thanks @neil-marcellini for elaborated response. That is very helpful. First of all - I have access to doc and thanks for that!
Our mechanism wheter its middleware / class / hook could have additional method to decide if name should update eg.
Questions about backend. Backend will discard our report name and return this computed on BE side. If report changes due to other enity modification (eg. Workspace name modified) will application get Pusher update for new report name or should we recompute that? I see several ways to tackle that: A: we stay with the middleware idea and we use that only to intercept all updates and we can create holistic solution that could intercept every change based on configuration (we could configure it to watch for workspace name change).
B: we implement small function that should be used on every action we would to run this action to.
C: we could subsribe to Onyx for every key we would like to investigate and change report name based on these changes
I will appreciate every help on how it's done in Expensify as this is my first time to create beta flag for new feature. |
@neil-marcellini Kind reminder that I start vacation and I'll be back on tuesday. I will work full time on this if we agree on details |
Ah shoot I was 90% done writing a reply to you, then restarted my browser and forgot to save it 😢. I'll have to get back to you later. |
Yeah sure. But also I don't think we'll reuse this system for anything else so I don't think it needs to necessarily be configurable. We can define that logic directly in the function.
I'll check. You can always experiment on classic (staging.expensify.com/inbox) with a test account. Yeah so the name isn't updated if the user has manually set a report name and the policy formula is updated. However the name will be updated to use the formula if enforcement is enabled. 2025-04-30_14-02-41.mp4 |
Yeah I wouldn't say "discard" but the BE will compute the title itself and send an update to overwrite the optimistic data. It will also send Pusher updates for those cases. This issue is only about handling the optimistic updates when queueing API write requests to change data the formula references.
I like this approach best because it seems to be the most simple and we have a relatively limited number of object types which the formula can depend on. These include but are not limited to However I would like to suggest another option which has those benefits without the other drawbacks. It's basically option A but runs before the request. Within prepareRequest before we apply the optimistic data, we could run a function on it which would identify any updates to the formula dependencies, then add optimistic updates for the titles. I think we'll be able to make it relatively efficient by doing only one pass over the updates and returning early if no extra updates are needed. It's also a nice way to centralize it. |
@sosek108 Eep! 4 days overdue now. Issues have feelings too... |
I like the idea with |
@neil-marcellini I've updated my initial plan. I've modified Phase 1 and Phase 2 to incorporate our discussed changes. As Phase 1 is quite simple we would merge P1 and P2. |
Ok great thanks. I'm going to hold off on reviewing that for a little bit, because I have decided that we should write a design doc for the backend and frontend changes. That's our standard practice and I was seeing how far I could get without it, but it's reached a level of complexity where I think that's the best path forward. Once I create that I'll share it with you and you can help me write it. |
To be sure. I should hold implementation phase until you return to me with design doc draft, right? |
Yes please |
@sosek108 Huh... This is 4 days overdue. Who can take care of this? |
@sosek108 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
@sosek108 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it! |
Problem
We have been working on implementing automatic report titles for new Expensify to match the feature on classic. Automatic report titles use a formula defined the policy to compute the report name and the report name is continually updated as data changes. The formulas are made up of various different parts such as
{report:type}
,report:startdate
, etc. You can check out how this feature works by going to Expensify Classic (expensify.com), workspace settings, choosing the report tab on the left side, and scrolling down to the automatic report title section. There is also a help site page linked there with more information. We're also updating the feature so that it sends out real time updates versus requiring a page refresh or loading the report.In order to maintain our standards of an optimistic offline first application, we will need to implement report formula computation on the front end. When a new report is created, it should optimistically have the automatic report title set based on the formula in the policy settings. Whenever any data is optimistically updated such as the amount of an expense, for example, then the report title should update if it depends on that data. One example would be a formula that references the report total.
We will gradually be rolling out support for more formula parts in new Expensify, and the entire feature will be under a beta flag while development is still active. Please stay in touch with me (@neil-marcellini) for updates on our backend progress.
Solution
Let's design a system to optimistically update the report title based on the formula, and integrate it into the application such that any API request will be automatically handled. It might make a good middleware for example. Once the system is designed, implement it for a few report formula parts that we already support like report type, start date and total. After that, we can distribute the implementation across a large number of contributors to get the remaining parts done faster.
The system should be under a beta to start, and we'll let you know the name of the beta as soon as it is created.
Issue Owner
Current Issue Owner: @sosek108The text was updated successfully, but these errors were encountered: