[HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] [LHN Mismatch] Move client-only keys from report_ to reportMetadata_

[puneetlath]

We are going to be changing the behavior of OpenApp/ReconnectApp when it does a full reconnect to replace the full reports list that the client has stored instead of merging with it. This means that any client-only data that we have about reports can potentially get wiped when the back-end reconnects.

We already have a reportMetadata_ object in Onyx that is meant to store such data. Let's move any keys that are currently client-only that are stored in the report_ object to the reportMetadata_ object. These are some likely keys that we will need to migrate:

1. avatarFileName: The filename of the avatar.
2. icons: List of icons for report participants.
3. isPolicyExpenseChat: Indicates whether the report is a policy expense chat.
4. lastMessageTimestamp: The timestamp of the last message on the report.
5. lastReadCreated: The time of the last read of the report.
6. openOnAdminRoom: If the admin room should be opened.
7. cachedTotal: Report cached total.
8. lastMessageTranslationKey: Translation key of the last message in the report.
9. isOptimisticReport: Whether the current report is optimistic.
10. displayName: Display name of the report, shown in options and mentions.
11. ownerEmail: E-mail of the report owner.
12. errors: Collection of errors to be shown to the user.
13. isLastMessageDeletedParentAction: Whether the last message was a deleted parent action.
14. iouReportAmount: Total amount of money owed for IOU report.
15. preexistingReportID: The ID of the preexisting report.
16. isHidden: Whether the report is hidden from the options list.
17. isChatRoom: Whether the report is a chat room.
18. participantsList: Collection of participants' personal details.
19. text: Text to be displayed in the options list, which matches reportName by default.
20. privateNotes: Collection of participant private notes, indexed by their accountID.
21. isLoadingPrivateNotes: Whether participants' private notes are currently loading.
22. selected: Whether the report is currently selected in the options list.
23. pendingChatMembers: Pending members of the report.
24. transactionThreadReportID: The ID of the single transaction thread report associated with this report, if one exists.
25. participantAccountIDs: Participant account IDs.
26. visibleChatMemberAccountIDs: Visible chat member account IDs.

To do this we will need to:

1. Confirm whether this is the complete list of keys to migrate or whether there are others
2. Confirm whether these keys all need to exist. Some of them seem like they might potentially be redundant and could be removed altogether. For example, isPolicyExpenseChat and isChatRoom seem like they may be redundant with chatType. The ownerEmail seems like it may be redundant with ownerAccountID. We should check each key and ensure that it's actually needed.
3. For those that are needed, let's move them to reportMetadata_ by:
   a. Updating any code that sets the data to set it in reportMetadata_ instead
   b. Updating any code that gets the data to get it from reportMetadata_ instead
   c. Create migration to migrate any existing data in these keys from report_ to reportMetadata_
4. Let's make it so that people can't accidentally add new client-only keys to the report object. We can do this by adding a unit test that checks that only these keys have been added to the report object. So that it must be explicitly updated if new keys are added after confirming that they are safe to add there

Issue Owner

Current Issue Owner: @muttmuure

[melvin-bot]

Current assignee @puneetlath is eligible for the NewFeature assigner, not assigning anyone new.

[melvin-bot]

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

[melvin-bot]

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

[puneetlath]

Confirm whether this is the complete list of keys to migrate or whether there are others

I'll be taking this part since it requires looking at the back-end also.

[TMisiukiewicz]

Hey, Tomasz from Callstack here, I can help with this issue

[puneetlath]

@TMisiukiewicz I need to confirm the full list that we're working with. To start, can you look into:

• Why do we have isPolicyExpenseChat? It seems redundant with type/chatType
• Why do we have isChatRoom? It seems redundant with type/chatType
• Whether there are others in the list above that seem redundant with existing data

[TMisiukiewicz]

@puneetlath I checked this list and seems like majority of these properties are generated in the runtime and describe the options for LHN, not reports directly. Seems like they are not stored in Onyx at all, might the Report type be outdated? I think it'd be good to actually go through the properties defined in a type and verify if they are still used anywhere in the project.

So, I'd say both properties are replaceable by referring to chatType when it comes to LHN options, but I don't see those stuff being moved from report to reportMetadata since they are not stored in Onyx

[puneetlath]

Oh wow, ok that's great to know.

I think it'd be good to actually go through the properties defined in a type and verify if they are still used anywhere in the project.

This makes sense to me. Want to do this as a first step? Basically, raise a PR to update the type with only the things that are actually used by the client.

Also, if the back-end attempts to merge something into Onyx that isn't in the type, do we log that?

[TMisiukiewicz]

This makes sense to me. Want to do this as a first step? Basically, raise a PR to update the type with only the things that are actually used by the client.

yeah sure, I'll start exploring it tomorrow 👍

[TMisiukiewicz]

So i verified all the properties and looks like there is 15 of them that should be possible to remove:

• icons
• lastMessageTimestamp
• lastReadCreated
• isPolicyExpenseChat
• openOnAdminRoom
• displayName
• errors
• isLastMessageDeletedParentAction
• iouReportAmount
• isChatRoom
• participantsList
• text
• selected
• participantAccountIDs
• visibleChatMemberAccountIDs

I’ll remove each unused property along with its references, one at a time, committing after each change to ensure stability.

[puneetlath]

Nice! Sounds good. Perhaps we should break it up into a few different PRs so that it's easier to revert if one of them causes a problem, without needing to revert it all.

[TMisiukiewicz]

Sure, i'll start with a PR with those that are not used anywhere as it's the easiest one

[TMisiukiewicz]

Opened first PR #52182, now I'll move to work on the properties that have some references in the codebase but do not seem to be a part of Report

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @muttmuure you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[melvin-bot]

Payment Summary

Upwork Job

• Contributor: @TMisiukiewicz is from an agency-contributor and not due payment
• ROLE: @shubham1206agra paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@muttmuure)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[puneetlath]

@TMisiukiewicz I have a few more I'd like to look into:

• pendingChatMembers - what is this used for? Sounds like probably something optimistic that could be moved to reportMetaData
• reportActionID - is this used anywhere? It doesn't seem to make sense since a report doesn't have a reportActionID. ReportActions have reportActionIDs.

And then can you remind me what the deal is with lastMessageTranslationKey? Is that something that is ever sent by the back-end? I couldn't find any reference to it. And if it's a front-end only thing is it then only used optimistically? Or when is it used?

[puneetlath]

And then in addition to those, I'd love to find a way to have some kind of front-end validation, that we are only setting fields that have been defined in the Report type. And to log if we aren't. Like if Onyx had a "validator" feature that you could pass it when initializing Onyx. And that would tell Onyx that if you see anything merged into X key it has to match Y shape. And to log if it doesn't. Do you think something like that would make sense and be possible?

[TMisiukiewicz]

pendingChatMembers - what is this used for? Sounds like probably something optimistic that could be moved to reportMetaData

I think this one is used also when you invite members to the room or policy, so i think OpenApp should return this one

reportActionID - is this used anywhere? It doesn't seem to make sense since a report doesn't have a reportActionID. ReportActions have reportActionIDs.

yeah seems like this one can be removed, I'll test it and prepare a PR if everything goes well

And then can you remind me what the deal is with lastMessageTranslationKey? Is that something that is ever sent by the back-end? I couldn't find any reference to it. And if it's a front-end only thing is it then only used optimistically? Or when is it used?

Looks like it's being sent only for optimistic data. I'll do some tests and open a PR if works properly

And then in addition to those, I'd love to find a way to have some kind of front-end validation, that we are only setting fields that have been defined in the Report type. And to log if we aren't. Like if Onyx had a "validator" feature that you could pass it when initializing Onyx. And that would tell Onyx that if you see anything merged into X key it has to match Y shape. And to log if it doesn't. Do you think something like that would make sense and be possible?

Let me think about it some more and discuss it with our team internally, I'll get back to you with that one 👍

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

[TMisiukiewicz]

Opened a PR #53817 to remove reportActionID. Regarding lastMessageTranslationKey, it is possible to move it ro reportMetadata, however this key might be used to generate alternateText for options in the LHN/search. This means we would have to connect to the reportMetadata collection to get this single information each time we are generating an option. For large accounts with thousands of reports, this might have negative impact on performance. I'm not sure the exact performance overhead here, but I'd say it's safer to leave it as it is now.

Regarding validating Onyx schema, this sounds like a pretty complex thing. Validation would be done in the runtime so there would be an additional cost for the performance. Are you thinking about specifically about validating correctness of the keys, or validating keys together with their type? If you are interested, we could possibly try to build some PoC to see how it affects the performance

[puneetlath]

Opened a PR #53817 to remove reportActionID

Thanks!

I think this one is used also when you invite members to the room or policy, so i think OpenApp should return this one

It's not returned by OpenApp. I don't see pendingChatMembers anywhere in the back-end code. So I think it's a front-end only thing.

Regarding lastMessageTranslationKey, it is possible to move it ro reportMetadata, however this key might be used to generate alternateText for options in the LHN/search. This means we would have to connect to the reportMetadata collection to get this single information each time we are generating an option. For large accounts with thousands of reports, this might have negative impact on performance. I'm not sure the exact performance overhead here, but I'd say it's safer to leave it as it is now.

Would you mind giving me a real-world example of how it's used? I'd love to understand what its used for better to see if we can come up with something for it.

Regarding validating Onyx schema, this sounds like a pretty complex thing. Validation would be done in the runtime so there would be an additional cost for the performance. Are you thinking about specifically about validating correctness of the keys, or validating keys together with their type? If you are interested, we could possibly try to build some PoC to see how it affects the performance

I was thinking of both. Validating the keys and their type. I could see that there would be a potential performance impact. Maybe this is better done with integration testing or something. How hard do you think it'd be to create a POC?

[TMisiukiewicz]

It's not returned by OpenApp. I don't see pendingChatMembers anywhere in the back-end code. So I think it's a front-end only thing.

I moved it to reportMetadata, however while working on it, I noticed it's another key that is used by createOption so it will make an additional call to Onyx for each report. I'll try to figure out if there is a good solution for that and what is the performance impact of such change - it's the same thing for moving lastMessageTranslationKey

Would you mind giving me a real-world example of how it's used? I'd love to understand what its used for better to see if we can come up with something for it.

We have en and es languages files. This property is keeping the translation key to be used from those files. I can see this is set e.g. here

App/src/libs/actions/Report.ts

Lines 525 to 538 in 665fa04

	const lastAction = attachmentAction ?? reportCommentAction;
	const currentTime = DateUtils.getDBTimeWithSkew();
	const lastComment = ReportActionsUtils.getReportActionMessage(lastAction);
	const lastCommentText = ReportUtils.formatReportLastMessageText(lastComment?.text ?? '');
	const optimisticReport: Partial<Report> = {
	lastVisibleActionCreated: lastAction?.created,
	lastMessageTranslationKey: lastComment?.translationKey ?? '',
	lastMessageText: lastCommentText,
	lastMessageHtml: lastCommentText,
	lastActorAccountID: currentUserAccountID,
	lastReadTime: currentTime,
	};

and addAction is used when adding attachment or comment. So looks like when we send an attachment, it gets the last report action, then gets its message and puts the translation key for optimistic data. I guess this might be used to display common.attachment translation in the LHN optimistically, however I haven't been able to reproduce it's usage. When I add an attachment, first it displays file name in the LHN, and then changes for [Attachment]. Maybe it's worth digging deeper and verifying if we need this property at all.

I was thinking of both. Validating the keys and their type. I could see that there would be a potential performance impact. Maybe this is better done with integration testing or something. How hard do you think it'd be to create a POC?

Hmm, creating a simple PoC for validating keys and their types shouldn’t be too hard. For a start, we could manually define a single object schema and check if passed object matches it by validating both the key existence and value types. This approach would be basic and give a quick sense of feasibility.

Ideally, though, an automated solution that generates schemas from our existing TypeScript types would be much better for scalability. One tool worth exploring is ts-to-zod, which transforms TypeScript types into Zod schemas. Zod is widely used for schema validation and could simplify the process significantly.

We already have a DebugUtils.ts file in our project, that includes some validations for specific items. However, this seems to require manual maintenance for each validator, which could be tedious over time.

If you are interested I can work on a simple PoC to validate the concept, and if the idea proves useful and practical, we can explore how to streamline schema generation and formalize it into a proposal

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.74-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [No QA] chore: remove reportActionID from report type #53817

If no regressions arise, payment will be issued on 2024-12-19. 🎊

For reference, here are some details about the assignees on this issue:

• @TMisiukiewicz does not require payment (Contractor)
• @shubham1206agra requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@shubham1206agra] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[TMisiukiewicz]

@puneetlath just a quick heads-up, I tried building a simple, basic schema validator, just to validate simplest possible keys during set and merge.

I'll try to set this up with collections to be able to verify the performance overhead

[puneetlath]

I moved it to reportMetadata, however while working on it, I noticed it's another key that is used by createOption so it will make an additional call to Onyx for each report. I'll try to figure out if there is a good solution for that and what is the performance impact of such change

Sounds good.

I guess this might be used to display common.attachment translation in the LHN optimistically, however I haven't been able to reproduce it's usage. When I add an attachment, first it displays file name in the LHN, and then changes for [Attachment]. Maybe it's worth digging deeper and verifying if we need this property at all.

Good to know! It would be awesome if it turns out we can remove lastMessageTranslationKey altogether.

@puneetlath just a quick heads-up, I tried building a simple, basic schema validator, just to validate simplest possible keys during set and merge.
I'll try to set this up with collections to be able to verify the performance overhead

Very cool! Interested to see what you find!