[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: @puneetlath

[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

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

  App/src/libs/ReportConnection.ts

  Line 31 in 5e69bfc

  reportIDToNameMap[report.reportID] = report.reportName ?? report.displayName ?? report.reportID;

  
  

  App/src/components/HTMLEngineProvider/HTMLRenderers/MentionReportRenderer/index.tsx

  Line 37 in 5e69bfc

  mentionDisplayText = removeLeadingLTRAndHash(report?.reportName ?? report?.displayName ?? htmlAttributeReportID);

• errors
  Found one place where it's being used, but I don't see it in any of my reports:
  

  App/src/libs/ReportUtils.ts

  Line 6460 in 5e69bfc

  const reportErrors = report?.errors ?? {};

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 #52395. I removed errors property entirely and moved displayName to OptionData type as it is generated for LHN options on client-side