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

Removing draft comments param #6252

Merged
merged 3 commits into from
Nov 5, 2021
Merged

Removing draft comments param #6252

merged 3 commits into from
Nov 5, 2021

Conversation

chiragsalian
Copy link
Contributor

@chiragsalian chiragsalian commented Nov 5, 2021 •
edited
Loading

Details

The draftComments param for getSideBarOptions was removed here. But at that time the usages were still passing draftComments so the method was receiving params in wrong order. The fixes here should correct that.

Fixed Issues

$ #6231

Tests

QA Steps

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

  1. Go to the App. Select #focus in preferences.
  2. Confirm only pinned chats show.

Mobile Web

Desktop

iOS

Android

@chiragsalian chiragsalian requested a review from a team as a code owner November 5, 2021 18:09
@chiragsalian chiragsalian self-assigned this Nov 5, 2021
@MelvinBot MelvinBot requested review from aldo-expensify and removed request for a team November 5, 2021 18:10
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2021

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

thienlnam
thienlnam reviewed Nov 5, 2021
View reviewed changes
Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

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

Changes look good, looks like you just need to update the method in the unit test as well

@thienlnam
Copy link
Contributor

Oh lol oops 🤦 nevermind

@thienlnam
Copy link
Contributor

thienlnam commented Nov 5, 2021 •
edited
Loading

You still need to update this right? Since it passes report_draft_comments

App/tests/unit/OptionsListUtilsTest.js

Line 597 in 5763378

const results = OptionsListUtils.getSidebarOptions(REPORTS, PERSONAL_DETAILS, REPORT_DRAFT_COMMENTS, 0, CONST.PRIORITY_MODE.GSD);

Also here I believe

App/tests/unit/OptionsListUtilsTest.js

Lines 564 to 570 in 5763378

const results = OptionsListUtils.getSidebarOptions(
reportsWithAddedPinnedMessagelessReport,
PERSONAL_DETAILS,
REPORT_DRAFT_COMMENTS,
0,
CONST.PRIORITY_MODE.DEFAULT,
);

aldo-expensify
aldo-expensify requested changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

I think we should remove these lines too:

App/src/pages/home/sidebar/SidebarLinks.js

Lines 277 to 279 in 13bb449

draftComments: {
key: ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT,
},

The prop draftComments is not used anymore.

@chiragsalian
Copy link
Contributor Author

@thienlnam both of those codes you linked are not in main branch 😬 - file

@chiragsalian
Copy link
Contributor Author

@aldo-expensify, yup good catch.

aldo-expensify
aldo-expensify approved these changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

Looks good to me!

thienlnam
thienlnam approved these changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

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

Oops, I guess I should start updating my local branch to main haha

@chiragsalian chiragsalian merged commit 3528390 into main Nov 5, 2021
@chiragsalian chiragsalian deleted the chirag-removing-draftComments-param branch November 5, 2021 19:37
github-actions bot pushed a commit that referenced this pull request Nov 5, 2021
@chiragsalian
Merge pull request #6252 from Expensify/chirag-removing-draftComments…
e03d621 
…-param

Removing draft comments param

(cherry picked from commit 3528390)
This was referenced Nov 5, 2021
Merged
Closed
@OSBotify
Copy link
Contributor

OSBotify commented Nov 5, 2021

🚀 Cherry-picked to staging by @chiragsalian in version: 1.1.14-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify OSBotify mentioned this pull request Nov 15, 2021
Closed
70 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @chiragsalian in version: 1.1.14-5 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.14-4 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @AndrewGable in version: 1.1.15-15 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify OSBotify mentioned this pull request Aug 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aldo-expensify aldo-expensify aldo-expensify approved these changes

@thienlnam thienlnam thienlnam approved these changes

Assignees

@chiragsalian chiragsalian

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chiragsalian @thienlnam @OSBotify @aldo-expensify