[HOLD for payment 2024-12-25] [HOLD for payment 2024-12-19] Import & Export Onyx state reliability issues

[TMisiukiewicz]

Exporting and importing Onyx state features are highly valuable in scenarios such as:

• Troubleshooting bugs caused by inconsistencies in the local Onyx state, which can lead to unexpected errors or unexpected UI behavior
• Measuring and improving performance on accounts experiencing significant slowdowns, as it allows developers to reproduce and analyze performance bottlenecks in a controlled environment.

However, there is significant room for improvement in both features to maximize their potential and ensure reliability. The current issues include the following:

Export

• Masking export data currently does not include multiple sensitive fields, such as last message, phone number or address
• Every email address and message is replaced with ***. While this ensures data anonymity, it also makes it difficult to analyze and reproduce bugs related to participants-related features and possibly impacts reliability when measuring performance, as the simplified masking fails to simulate realistic data structures or payload sizes

Import

• Missing items in key UI elements, such as LHN and Search option lists, which results in broken or incomplete user experience
• Imported states often show inconsistnt option titles compared to original state
• Certain expenses are missing which impacts testing expenses workflows
• When splitting expense on 1:1 chat, the original account is displayed as third participant
• Navigating back to the original state from an opened report screen often triggers errors

All these issues collectively affect the reliability of the export and import features, reducing their effectiveness for debugging and performance optimization tasks. Addressing these problems is crucial for ensuring their value in real-world scenarios.

Issue Owner

Current Issue Owner: @jjcoffee

[TMisiukiewicz]

While recording videos with a fix for proper displaying LHN elements I noticed the Import is broken on iOS. I'll focus on fixing this one first before I open any other PR since it blocks testing on the platform entirely

[TMisiukiewicz]

Found a solution for above error. The react-native-fs library is not maintained anymore for more than 2 years now, and it is expecting NSInteger pointer for both position and length parameters of read function. According to the React Native docs, using NSInteger should be avoided.

So we have a couple of options:

1. Patch the library and switch from using NSInteger to NSNumber
2. Replace the library with it's fork, https://github.com/birdofpreyru/react-native-fs, which is actively maintained and supports new architecture
3. Use react-native-blob-util filesystem that we already have in the codebase. It supports reading files as stream, so i think it should work as well.

From my perspective, the fastest way to unblock us with improvements of the Import feature is using a patch. However the last one sounds reasonable as well. Which one should we follow? We can always go with a patch first as it's a quick one (and I already have it working) and rewrite it to readStream in the future

[mountiny]

If we already use react-native-blob-util in the app and it can be used here too without issues and its maintained, then lets use that instead of patching the unmaintained library.

[melvin-bot]

Triggered auto assignment to @Gonals, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

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

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

• fix: Import Onyx state on iOS #53370
• refactor: mask emails with random strings instead of *** #53527

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)

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

[jjcoffee]

@Gonals Could you assign me here and add the Bug label to get a BZ assigned for future payment? Thanks 🙏

[eh2077]

Same here, I helped review this PR #53527

[melvin-bot]

Triggered auto assignment to @RachCHopkins (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot]

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

• fix: mask fields with sensitive data for Export #53559
• fix: display all LHN options in Import state mode #53374

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

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

• @TMisiukiewicz does not require payment (Contractor)
• @jjcoffee requires payment through NewDot Manual Requests

[melvin-bot]

@jjcoffee @RachCHopkins @jjcoffee The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[TMisiukiewicz]

just FYI there might be another PRs for that in the future linked to this issue - since we already have a couple of fixes merged, I'll do another round of testing if there are still any troubles when using it.

[RachCHopkins]

@Gonals does @eh2077 need to be added here per this comment?

@jjcoffee can you complete the checklist please?

And can someone tell me what the price tag is for our contributors?

[mollfpr]

👋 I also help reviewed #53559

[jjcoffee]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: N/A

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion:

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Precondition:

• An account with a large number of chats/reports in LHN

Test:

1. Go to Settings -> Troubleshoot
2. Export Onyx state
3. Log out and log into a brand new account
4. Go to Settings -> Troubleshoot
5. Press Import Onyx state and select the exported file
6. Wait until the file loads
7. Verify the LHN have an equally large number of items comparing to the original account

Do we agree 👍 or 👎

[jjcoffee]

@RachCHopkins Done! I guess the C+s on other PRs should just post regression tests if needed.

The price should be the standard $250 for each PR as far as I can see.

[RachCHopkins]

Payment Summary:

• Contributor: @TMisiukiewicz - no payment, contractor
• Contributor: @eh2077 to be paid $250 via NewDot manual request
• Contributor: @mollfpr to be paid $250 via NewDot manual request
• Contributor: @eVoloshchak to be paid $250 via NewDot manual request
• Contributor+: @jjcoffee to be paid $250 via NewDot manual request

No Upwork job.

@Gonals @jjcoffee, can I get a buddy check on this, please? It's a little confusing.

[Edit: Updated 23 Dec to include contributor per below]

[jjcoffee]

@RachCHopkins There's a 4th PR #53370 that @eVoloshchak reviewed, but otherwise that looks right to me!

[RachCHopkins]

Wow, this was a real party!! Updating Payment Summary.

[RachCHopkins]

Contributors ready for payment via NewDot, no contracts to complete, no Upwork post to close.

Merry Xmas, Happy Holidays and Best Wishes to everyone involved!