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.

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

fix: Regression - Emoji reaction not working - Issue 22235 (Original - 21826) #22270

Merged
merged 30 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
fd300e2
fix: Changes
jeet-dhandha Jun 21, 2023
06f9bbc
Revert "fix: Changes"
jeet-dhandha Jun 21, 2023
4839b98
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jun 23, 2023
9aaae8a
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jun 24, 2023
4ba0164
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jun 27, 2023
86f7d3a
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jun 29, 2023
1a1ef11
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jun 29, 2023
93b92ae
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jun 29, 2023
9ae6e80
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jun 30, 2023
3cd7964
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 3, 2023
c9474fd
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 4, 2023
fd41eb4
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 5, 2023
d126def
fix-regression
jeet-dhandha Jul 5, 2023
ae07063
fix: report action name changes
jeet-dhandha Jul 5, 2023
fcbdd8c
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 5, 2023
5eeeb95
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 6, 2023
cc1bb9c
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 7, 2023
1fa3623
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 9, 2023
39737ca
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 11, 2023
ccb128f
Merge branch 'main' into regression-fix-21826
jeet-dhandha Jul 11, 2023
9634040
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 11, 2023
5162f9a
Merge branch 'main' into regression-fix-21826
jeet-dhandha Jul 11, 2023
461b766
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 13, 2023
5f7fb5c
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 14, 2023
a5cbaad
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 18, 2023
670ecfc
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 19, 2023
30d615f
Merge remote-tracking branch 'upstream/main'
jeet-dhandha Jul 20, 2023
653c0f7
Merge branch 'main' into regression-fix-21826
jeet-dhandha Jul 20, 2023
db668d3
chore: use reportId from reportAction
jeet-dhandha Jul 20, 2023
229a5d3
chore: change jsdoc type
jeet-dhandha Jul 20, 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
7 changes: 4 additions & 3 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -1635,13 +1635,14 @@ function removeEmojiReaction(reportID, originalReportAction, emoji) {
/**
* Calls either addEmojiReaction or removeEmojiReaction depending on if the current user has reacted to the report action.
* @param {String} reportID
* @param {String} reportActionID
* @param {Object} action
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param {Object} action
* @param {Object} reportAction

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's re-use name from original code so replace all action with reportAction

Copy link
Contributor Author

@jeet-dhandha jeet-dhandha Jul 5, 2023

Choose a reason for hiding this comment

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

But this will give conflicts with the const created inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* @param {Object} emoji
* @param {number} paramSkinTone
* @returns {Promise}
*/
function toggleEmojiReaction(reportID, reportActionID, emoji, paramSkinTone = preferredSkinTone) {
const reportAction = ReportActionsUtils.getReportAction(reportID, reportActionID);
function toggleEmojiReaction(reportID, action, emoji, paramSkinTone = preferredSkinTone) {
const originalReportID = ReportUtils.getOriginalReportID(reportID, action);
Copy link
Contributor

Choose a reason for hiding this comment

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

As original reportID is already calculated here, let's remove redundant calculation in addEmojiReaction and removeEmojiReaction

const reportAction = ReportActionsUtils.getReportAction(originalReportID, action.reportActionID);

if (_.isEmpty(reportAction)) {
return;
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ContextMenu/ContextMenuActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export default [
};

const onEmojiSelected = (emoji) => {
Report.toggleEmojiReaction(reportID, reportAction.reportActionID, emoji);
Report.toggleEmojiReaction(reportID, reportAction, emoji);
closeContextMenu();
};

Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ReportActionItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ function ReportActionItem(props) {

const toggleReaction = useCallback(
(emoji) => {
Report.toggleEmojiReaction(props.report.reportID, props.action.reportActionID, emoji);
Report.toggleEmojiReaction(props.report.reportID, props.action, emoji);
},
[props.report, props.action],
);
Expand Down
4 changes: 2 additions & 2 deletions tests/actions/ReportTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ describe('actions/Report', () => {
const resultAction = _.first(_.values(reportActions));

// Add a reaction to the comment
Report.toggleEmojiReaction(REPORT_ID, resultAction.reportActionID, EMOJI);
Report.toggleEmojiReaction(REPORT_ID, resultAction, EMOJI);
return waitForPromisesToResolve();
})
.then(() => {
Expand All @@ -665,7 +665,7 @@ describe('actions/Report', () => {
// Now we toggle the reaction while the skin tone has changed.
// As the emoji doesn't support skin tones, the emoji
// should get removed instead of added again.
Report.toggleEmojiReaction(REPORT_ID, resultAction.reportActionID, EMOJI, 2);
Report.toggleEmojiReaction(REPORT_ID, resultAction, EMOJI, 2);
return waitForPromisesToResolve();
})
.then(() => {
Expand Down
Loading