Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/32230: Update recently used tag and category when editing request #32467

Merged
merged 4 commits into from
Dec 13, 2023
Merged

Fix/32230: Update recently used tag and category when editing request #32467

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
504eed2
update recently used tag and categoy when editing request
DylanDylann Dec 5, 2023
5910285
fix lint
DylanDylann Dec 5, 2023
60c7027
using SET method
DylanDylann Dec 9, 2023
53dc13a
update using MERGE method for tag
DylanDylann Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 46 additions & 8 deletions src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,30 @@ function editDistanceMoneyRequest(transactionID, transactionThreadReportID, tran
},
});

// Update recently used categories if the category is changed
if (_.has(transactionChanges, 'category')) {
const optimisticPolicyRecentlyUsedCategories = Policy.buildOptimisticPolicyRecentlyUsedCategories(iouReport.policyID, transactionChanges.category);
if (!_.isEmpty(optimisticPolicyRecentlyUsedCategories)) {
optimisticData.push({
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_CATEGORIES}${iouReport.policyID}`,
value: optimisticPolicyRecentlyUsedCategories,
});
}
}

// Update recently used categories if the tag is changed
if (_.has(transactionChanges, 'tag')) {
const optimisticPolicyRecentlyUsedTags = Policy.buildOptimisticPolicyRecentlyUsedTags(iouReport.policyID, transactionChanges.tag);
if (!_.isEmpty(optimisticPolicyRecentlyUsedTags)) {
optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use the method "SET" here? For POLICY_RECENTLY_USED_CATEGORIES above, it's using method SET, but here it's using method MERGE. I found it's inconsistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoangzinh I agree with you at this point. But currently we use MERGE method for tag in other places

App/src/libs/actions/IOU.js

Line 187 in 18d4b9b

onyxMethod: Onyx.METHOD.MERGE,

App/src/libs/actions/IOU.js

Line 1875 in 18d4b9b

onyxMethod: Onyx.METHOD.MERGE,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like MERGE or SET is same if it's an array https://github.com/Expensify/react-native-onyx#should-i-use-merge-or-set-or-both, nvm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoangzinh Do you mean that we should update to use SET in all places ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, it's stated here

Use set() when you need to completely reset an object or array of data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoangzinh Updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had this exact question!

The only thing though is that I think these all either need to be SET or all need to MERGE per

For this reason, it is usually preferable to use one or the other, but not both.

And even more fun once you look at buildOptimisticPolicyRecentlyUsedCategories returns an array and buildOptimisticPolicyRecentlyUsedTags returns an object

So I think for calls to set the recently used categories we should use SET everywhere as noted

You should avoid arrays as much as possible. They do not work well with merge() because it can't update a single element in an array, it must always set the entire array each time.

And for recently used tags we should use MERGE everywhere since those are objects.

key: `${ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_TAGS}${iouReport.policyID}`,
value: optimisticPolicyRecentlyUsedTags,
});
}
}

// Clear out the error fields and loading states on success
successData.push({
onyxMethod: Onyx.METHOD.MERGE,
Expand Down Expand Up @@ -1809,8 +1833,6 @@ function editRegularMoneyRequest(transactionID, transactionThreadReportID, trans
updatedChatReport.lastMessageHtml = messageText;
}

const optimisticPolicyRecentlyUsedTags = Policy.buildOptimisticPolicyRecentlyUsedTags(iouReport.policyID, transactionChanges.tag);

const isScanning = TransactionUtils.hasReceipt(updatedTransaction) && TransactionUtils.isReceiptBeingScanned(updatedTransaction);

// STEP 4: Compose the optimistic data
Expand Down Expand Up @@ -1870,12 +1892,28 @@ function editRegularMoneyRequest(transactionID, transactionThreadReportID, trans
: []),
];

if (!_.isEmpty(optimisticPolicyRecentlyUsedTags)) {
optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_TAGS}${iouReport.policyID}`,
value: optimisticPolicyRecentlyUsedTags,
});
// Update recently used categories if the category is changed
if (_.has(transactionChanges, 'category')) {
const optimisticPolicyRecentlyUsedCategories = Policy.buildOptimisticPolicyRecentlyUsedCategories(iouReport.policyID, transactionChanges.category);
if (!_.isEmpty(optimisticPolicyRecentlyUsedCategories)) {
optimisticData.push({
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_CATEGORIES}${iouReport.policyID}`,
value: optimisticPolicyRecentlyUsedCategories,
});
}
}

// Update recently used categories if the tag is changed
if (_.has(transactionChanges, 'tag')) {
const optimisticPolicyRecentlyUsedTags = Policy.buildOptimisticPolicyRecentlyUsedTags(iouReport.policyID, transactionChanges.tag);
if (!_.isEmpty(optimisticPolicyRecentlyUsedTags)) {
optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_TAGS}${iouReport.policyID}`,
value: optimisticPolicyRecentlyUsedTags,
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to DRY those LOC @DylanDylann?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoangzinh We can create new function called createOptimisticPolicyRecentlyUsedTagOrCategory
and update like this

if (_.has(transactionChanges, 'category')) {
            optimisticData.push(createOptimisticPolicyRecentlyUsedTagOrCategory());
        }
    }

But I don't think It helps to dry code lots. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, let's leave it as it is. It's not a big problem for me. Let me complete the checklist and wait opinion from Internal engineer.

}

const successData = [
Expand Down
Loading