[HOLD for payment 2023-08-17] [$1000] Create Onyx migration for previousReportActionID

[roryabraham]

Part of the Comment Linking project

Main issue: https://github.com/Expensify/App/issues/20282 Doc section: https://docs.google.com/document/d/1v-ZaIRTZL5LIsyPWB0IopBNNgCajf5WC1OA8cVKtd8I/edit#bookmark=id.bry74gx446k1 Project: Comment Linking

Create an Onyx migration called CheckForPreviousReportActionID that:

• Looks at a single reportAction from Onyx
• If that reportAction has the previousReportActionID field, resolve the migration.
• If the reportAction does not have the previousReportActionID field, delete all reportActions from Onyx

This migration should be created and tested, but not yet added to migrateOnyx until we are ready.

Manual Test Steps

Manually test this by locally adding the migration to migrateOnyx but do not commit that change.

Automated Tests

We should update MigrationTest to include tests for the new migration.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01382a606158d56e43
• Upwork Job ID: 1681813151297843200
2023-07-19
• Automatic offers:
• abdulrahuman5196 | Reviewer | 25687360
• neonbhai | Contributor | 25687361

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01382a606158d56e43

[melvin-bot[bot]]

Triggered auto assignment to @joekaufmanexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot[bot]]

Triggered auto assignment to @joekaufmanexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)

[roryabraham]

No UI changes so I unassigned @shawnborton, but didn't mean to unassign @abdulrahuman5196

[melvin-bot[bot]]

📣 @abdulrahuman5196 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[roryabraham]

Sorry for any confusion, @abdulrahuman5196 is the C+ for this issue

[neonbhai]

Can contributors work on this feature? I am interested in working on this!

[roryabraham]

Yep this is open to contributors. Please make a proposal 🙇🏼

[allroundexperts]

• If the reportAction does not have the previousReportActionID field, delete all reportActions from Onyx

@roryabraham do you mean to delete ALL the report actions?

[neonbhai]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Create Onyx migration for previousReportActionID

What is the root cause of that problem?

New Feature

What changes do you think we should make in order to solve the problem?

We are creating an Onyx migration called CheckForPreviousReportActionID which will:

1. Look at a single reportAction from Onyx.
2. If reportAction has the previousReportActionID field, i.e., if(lodashHas(reportAction, ['previousReportActionID']) returns true, we resolve() the migration.
3. If it returns false, we will delete all reportActions from Onyx.

For test cases:

1. Add mock test cases like the ones in this file. For this migration, we would have mock reportActions with some of them having the previousReportActionID field.
2. Carry out the migration and check if reportActions were removed.
3. Do thorough testing for all senarios! (all reportActions have the field, none of the reportActions have the field, some of the reportActions have the field)

Manual Testing Scenario:

• Add the migration locally here, check if it works (not committed to PR!)

What alternative solutions did you explore? (Optional)

xx

[neonbhai]

@allroundexperts looks like it! This is probably because reportActions now always have to have a previousReportActionID, that represents (as the name says) to the reportAction immediately preceding it. If its not present, we delete reportActions as I'm guessing this implies some other task failed to complete. @roryabraham please confirm! I am also curious how this migration is useful for implementing the comment deeplinking feature, Is there any way I as a contributor can get to read the design doc, as it would help better understanding on how this feature is being implemented!

[allroundexperts]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Create a new onyx migration

What is the root cause of that problem?

N/A

What changes do you think we should make in order to solve the problem?

In migrations folder, we need to add a file called CheckForPreviousReportActionID with the following code:

import _ from 'underscore';
import lodashHas from 'lodash/has';
import Onyx from 'react-native-onyx';
import ONYXKEYS from '../../ONYXKEYS';

/**
 * @returns {Promise<Object>}
 */
function getReportActionsFromOnyx() {
    return new Promise((resolve) => {
        const connectionID = Onyx.connect({
            key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
            waitForCollectionCallback: true,
            callback: (allReportActions) => {
                Onyx.disconnect(connectionID);
                return resolve(allReportActions);
            },
        });
    });
}

export default function () {
    return getReportActionsFromOnyx().then(allActions => {
        const newOnyxData = _.reduce(Object.entries(allActions), (acc, [reportID, reportActions]) => {
            if (_.find(reportActions, (action) => !lodashHas(action, 'previousReportActionID'))) {
                return {...acc, [reportID]: {}};
            }
            return acc;
        }, {});

        if (_.keys(newOnyxData).length === 0) {
            return true;
        }

        return Onyx.multiSet(newOnyxData);
    });
}

Here are the test cases:

it('Should remove all report actions given that a previousReportActionId does not exist', () =>
                Onyx.multiSet({
                    [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}1`]: {
                        1: {
                            actorEmail: 'test2@account.com',
                            previousReportActionId: 0,
                        },
                        2: {
                            actorEmail: 'test2@account.com',
                        },
                    },
                })
                    .then(CheckForPreviousReportActionID)
                    .then(() => {
                        const connectionID = Onyx.connect({
                            key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
                            waitForCollectionCallback: true,
                            callback: (allReportActions) => {
                                Onyx.disconnect(connectionID);
                                const expectedReportAction = {};
                                expect(allReportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}1`][1]).toMatchObject(expectedReportAction);
                            },
                        });
            }));

            it('Should not remove any report action given that previousReportActionId exist in every action', () =>
                Onyx.multiSet({
                    [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}1`]: {
                        1: {
                            previousReportActionId: 0,
                        },
                        2: {
                            previousReportActionId: 1,
                        },
                    },
                })
                    .then(CheckForPreviousReportActionID)
                    .then(() => {
                        const connectionID = Onyx.connect({
                            key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
                            waitForCollectionCallback: true,
                            callback: (allReportActions) => {
                                Onyx.disconnect(connectionID);
                                const expectedReportAction = {
                                    1: {
                                        previousReportActionId: 0,
                                    },
                                    2: {
                                        previousReportActionId: 1,
                                    },
                                };
                                expect(allReportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}1`][1]).toMatchObject(expectedReportAction);
                            },
                        });
            }));

What alternative solutions did you explore? (Optional)

None

[perunt]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Moving forward, we will utilize the previousReportActionID within reportAction for certain functionalities. Thus, it is crucial for us to ensure its presence in the system

What is the root cause of that problem?

Implementing support comment linking feature

What changes do you think we should make in order to solve the problem?

Create a new onyx migration In this migration, we aim to ensure that each reportAction from a Report possesses a previousReportActionID. We check the first reportAction of each Report, and if the previousReportActionID is missing, we clear all the reportActions for that particular Report from Onyx.

In the migrations folder, create a file called PreviousReportActionID with the following code:

/**
 * @returns {Promise<Object>}
 */
function getReportActionsFromOnyx() {
    return new Promise((resolve) => {
        const connectionID = Onyx.connect({
            key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
            waitForCollectionCallback: true,
            callback: (allReportActions) => {
                Onyx.disconnect(connectionID);
                return resolve(allReportActions);
            },
        });
    });
}

/**
 * Migrate Onyx data for reportActions. If the first reportAction of a reportActionsForReport
 * does not contain a 'previousReportActionID', all reportActions for that report are removed from Onyx.
 *
 * @returns {Promise<void>}
 */
export default function () {
    return getReportActionsFromOnyx().then(
        (oldReportActions) => {
            const onyxData = {};

            _.each(oldReportActions, (reportActionsForReport, onyxKey) => {
              if (_.isEmpty(reportActionsForReport)) {
                Log.info(`[Migrate Onyx] Skipped migration for ${onyxKey} because there were no reportActions`);
                return;
              }

              const firstReportActionID = Object.keys(reportActionsForReport)[0];
              const firstReportAction = reportActionsForReport[firstReportActionID];

              if (!firstReportAction || !firstReportAction.previousReportActionID) {
                Log.info(`[Migrate Onyx] Migration: removing all reportActions for ${onyxKey} because previousReportActionID not found in the first reportAction`);
                onyxData[onyxKey] = {};
              }
            });

            return Onyx.multiSet(onyxData);
        },
    );
}

test cases could look next:

    describe('CheckPreviousReportActionID', () => {
        it("Should wipe reportActions if 'previousReportActionID' is missing", () =>
            Onyx.multiSet({
                [`${ONYXKEYS.COLLECTION.REPORT}1`]: {
                    reportActions: {
                        'action1': {
                            reportActionID: 'action1',
                            previousReportActionID: null,
                        },
                        'action2': {
                            reportActionID: 'action2',
                            previousReportActionID: 'action1',
                        },
                    },
                },
                [`${ONYXKEYS.COLLECTION.REPORT}2`]: {
                    reportActions: {
                        'action3': {
                            reportActionID: 'action3',
                            previousReportActionID: 'action2',
                        },
                        'action4': {
                            reportActionID: 'action4',
                            previousReportActionID: 'action3',
                        },
                    },
                },
            })
                .then(CheckPreviousReportActionID)
                .then(() => {
                    const connectionID = Onyx.connect({
                        key: ONYXKEYS.COLLECTION.REPORT,
                        waitForCollectionCallback: true,
                        callback: (allReports) => {
                            Onyx.disconnect(connectionID);
                            expect(_.keys(allReports).length).toBe(2);
                            expect(allReports.report_1.reportActions).toBeNull();
                            expect(allReports.report_2.reportActions).not.toBeNull();
                        },
                    });
                }));
    });
});

[perunt]

@allroundexperts @neonbhai, we are removing all reportActions from the current Report if the previousReportActionID is absent. We are implementing this change to support a feature that enables comment linking, allowing users to navigate through a Report using links instead of scrolling. However, to make this feature work smoothly, we need to eliminate any inconsistency in the saved data.

[allroundexperts]

Thanks @perunt!

That's what I guessed and proposed here.

[joekaufmanexpensify]

Proposals pending review.

[joekaufmanexpensify]

@abdulrahuman5196 could you take a look at the proposals here when you have a sec?

[abdulrahuman5196]

Yes will review today.

[joekaufmanexpensify]

Great, ty!

[abdulrahuman5196]

@roryabraham I have a doubt in the requirement?

If the reportAction does not have the previousReportActionID field, delete all reportActions from Onyx

Do we check all the reportAction for a particular report? Or just first reportAction or something else?

And assuming we find a reportAction with the previousReportActionID, we delete all the reportActions of that particular report right? Or just reportActions preceding the current reportAction?

[roryabraham]

Sorry for the delayed responses here...

@roryabraham do you mean to delete ALL the report actions?

Yes, all reportActions for all reports.

This is probably because reportActions now always have to have a previousReportActionID, that represents (as the name says) to the reportAction immediately preceding it. If its not present, we delete reportActions as I'm guessing this implies some other task failed to complete. @roryabraham please confirm!

Yep, that's correct. In this case, the "other task" would be a change in our API to update the Onyx schema.

Do we check all the reportAction for a particular report? Or just first reportAction or something else? ... And assuming we find a reportAction with the previousReportActionID, we delete all the reportActions of that particular report right? Or just reportActions preceding the current reportAction?

We should look at one reportAction from any report. It does not matter which report or reportAction. If that one reportAction has a previousReportActionID field, then great the migration is done. If not, then we will delete all reportActions from all reports in Onyx.

[roryabraham]

@perunt even though you're heavily involved in this project I'm going to give this issue to @neonbhai for this proposal since it seems like they understand the task and know where to make the change. That gives you more time to focus on other tasks 🙂

[melvin-bot[bot]]

📣 @abdulrahuman5196 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Upwork job

[melvin-bot[bot]]

📣 @neonbhai 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[neonbhai]

Thank you for assigning @roryabraham! Will raise PR in 24 hours :)

[neonbhai]

@roryabraham @abdulrahuman5196 The PR is ready for review!

[joekaufmanexpensify]

PR still in review.

[melvin-bot[bot]]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @neonbhai got assigned: 2023-07-22 08:03:16 Z
• when the PR got merged: 2023-08-08 19:09:21 UTC
• days elapsed: 11

On to the next one 🚀

[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 1.3.52-5 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/23457

If no regressions arise, payment will be issued on 2023-08-17. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• [x] External issue reporter
• [x] Contributor that fixed the issue
• [x] Contributor+ that helped on the issue and/or PR

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

• @abdulrahuman5196 requires payment offer (Reviewer)
• @neonbhai requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[joekaufmanexpensify]

Not overdue. Payment due next week!

[abdulrahuman5196]

No BZ checklist, since this is a migration feature request.

[joekaufmanexpensify]

Okay, all set to issue payment here. Based on the guidelines for payment, this one would qualify for 50% speed penalty, since it took more than 9 business days to merge (11 specifically). This means we need to pay:

• @neonbhai $500 for PR.
• @abdulrahuman5196 $500 for C+ review.

LMK if you have any questions on this, and if not, I'll issue payment on Monday!

[neonbhai]

Hi @joekaufmanexpensify just wanted to say that the PR started reviewing about 3 days after raising the PR as I'm presuming @abdulrahuman5196 sir could not give enough time. As this was my first feature contribution, and since the migration is on the backend and the check could be running on every user signin, me and @abdulrahuman5196 tried to perfect and minimize the PR code. I hope the penalty can be forgiven 🙇

[joekaufmanexpensify]

@neonbhai Thanks for the context! The purpose of the speed bonus/penalty is to incentivize the contributor and C+ to collaborate to get the PR merged quickly. It doesn't look like there were any internal delays here, so we'd still need to apply the penalty.

But this doesn't impact your ability to contribute to future issues, or anything like that! LMK if you have any other questions.

[neonbhai]

@joekaufmanexpensify okay, got it!

[joekaufmanexpensify]

@neonbhai $500 sent and contract ended!

[joekaufmanexpensify]

@abdulrahuman5196 I see you were hired twice on this Upwork job, but I'm not sure why. As an fyi, I'm going to pay you $500 on one of the contracts, and then close out the other one with no payment.

LMK if you have any questions on this!

[joekaufmanexpensify]

@abdulrahuman5196 $500 sent and first contract ended!

[joekaufmanexpensify]

@abdulrahuman5196 second contract ended with no payment.

[joekaufmanexpensify]

Upwork job closed.

[joekaufmanexpensify]

Closing as this is all set. Thanks everyone!