[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[bot]]

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

[melvin-bot[bot]]

:warning: 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 :warning:

[melvin-bot[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:

• [x] icons
• [x] lastMessageTimestamp
• [x] lastReadCreated
• [x] isPolicyExpenseChat
• [x] openOnAdminRoom
• [x] displayName
• [x] errors
• [x] isLastMessageDeletedParentAction
• [x] iouReportAmount
• [x] isChatRoom
• [x] participantsList
• [x] text
• [x] selected
• [x] participantAccountIDs
• [x] 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 https://github.com/Expensify/App/pull/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

[TMisiukiewicz]

Opened another PR to remove isPolicyExpenseChat. Also turned the list from the last comment into checklist to track the progress of the cleanup

[TMisiukiewicz]

Opened last PR with unused properties. The two remaining on the list are:

• displayName Found two places where it's being used, but I don't see any of my reports with this property set: https://github.com/Expensify/App/blob/5e69bfc36ddfd82a113606257abb93f2fa78f7db/src/libs/ReportConnection.ts#L31 https://github.com/Expensify/App/blob/5e69bfc36ddfd82a113606257abb93f2fa78f7db/src/components/HTMLEngineProvider/HTMLRenderers/MentionReportRenderer/index.tsx#L37
• errors Found one place where it's being used, but I don't see it in any of my reports: https://github.com/Expensify/App/blob/5e69bfc36ddfd82a113606257abb93f2fa78f7db/src/libs/ReportUtils.ts#L6460

Is it possible that Report had different shape in the past and it's valid only for some old reports? Maybe we should leave these properties?

[puneetlath]

Ah interesting. Let me see if I can find any place where they get set from the back-end.

[puneetlath]

I don't see report.errors or report.displayName as being returned from the back-end anywhere. So I think it should be safe to remove. Do you agree @shubham1206agra?

Also, @TMisiukiewicz do we log if someone tries to set an Onyx field that isn't part of the report Onyx type? And if not, could we?

[shubham1206agra]

@TMisiukiewicz displayName is being used to store the calculated displayName in LHN. Can you confirm this?

[TMisiukiewicz]

@shubham1206agra yeah they are used to calulate the display name for LHN, but they are not stored in Onyx

[TMisiukiewicz]

Also, @TMisiukiewicz do we log if someone tries to set an Onyx field that isn't part of the report Onyx type? And if not, could we?

I think we have only static checks (Typescript). Do you have anything particular in your mind? If yes, how do you think it should work?

[shubham1206agra]

Hey This variable is most probably used as typecheck there. Just fix that

[TMisiukiewicz]

Opened https://github.com/Expensify/App/pull/52395. I removed errors property entirely and moved displayName to OptionData type as it is generated for LHN options on client-side

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/52182

If no regressions arise, payment will be issued on 2024-11-20. :confetti_ball:

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[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.
• [ ] [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[TMisiukiewicz]

As discussed earlier on Slack, I am starting to work on moving the keys that are not returned from OpenApp to reportMetadata. For now, I identified the following keys:

• [x] avatarFileName
• [x] cachedTotal
• [ ] lastMessageTranslationKey - should stay in report
• [ ] isOptimisticReport
• [x] ownerEmail
• [ ] preexistingReportID - should stay in report
• [ ] isHidden - should stay in report
• [x] ~~privateNotes~~
• [x] isLoadingPrivateNotes
• [x] transactionThreadReportID

I'll verify each one of them and send separate PRs

[puneetlath]

@TMisiukiewicz I see in this PR a bunch of these keys getting re-added to the report type: https://github.com/Expensify/App/pull/50745/files

Can we please add a unit test that tests that only the specific keys that we have deemed should be on the report type are on the report type. Basically we should add a test that:

• has a comment explaining that only keys that are sent from the server should be on the report type. Anything else should be on reportMetaData_
• says that if you want to update this test to include a new key, you need confirmation from an internal engineer that this has indeed been added to the report object that is returned in OpenApp
• checks that only the list of keys that we have whitelisted are on the type

That way if someone wants to add a new key to the report type, they need to update this test in order to do so.

[gedu]

Hey @puneetlath I'm taking a look today, @TMisiukiewicz is OOO, he will continue on Monday if I don't finish

Will be good to hold on that PR, I will start finding a way to achieve the described items

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/52247

If no regressions arise, payment will be issued on 2024-11-22. :confetti_ball:

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)