Skip to content

🍒 Cherry pick PR #8011 to staging 🍒#8013

Merged
TomatoToaster merged 3 commits intostagingfrom
OSBotify-cherry-pick-staging-8011
Mar 7, 2022
Merged

🍒 Cherry pick PR #8011 to staging 🍒#8013
TomatoToaster merged 3 commits intostagingfrom
OSBotify-cherry-pick-staging-8011

Conversation

@OSBotify
Copy link
Contributor

@OSBotify OSBotify commented Mar 4, 2022

🍒 Cherry pick #8011 to staging 🍒

OSBotify and others added 2 commits March 4, 2022 17:02
@OSBotify
Merge pull request #8012 from Expensify/version-BUILD-d64f8c52cc87054…
ecdbb43 
…c6c1f7304f18bda05b3b86920

(cherry picked from commit 2837a6a)
@danieldoglas @OSBotify
Merge pull request #8011 from Expensify/amal-fix-reportdetails-icons
8aa5028 
Fix reportDetails icons for chatrooms

(cherry picked from commit d64f8c5)
@OSBotify OSBotify requested a review from a team as a code owner March 4, 2022 17:02
@MelvinBot MelvinBot removed request for a team March 4, 2022 17:02
@OSBotify
Copy link
Contributor Author

OSBotify commented Mar 4, 2022

This pull request has merge conflicts and can not be automatically merged. 😞
Please manually resolve the conflicts, push your changes, and then request another reviewer to review and merge.
Important: There may be conflicts that GitHub is not able to detect, so please carefully review this pull request before approving.

@danieldoglas danieldoglas requested a review from a team March 4, 2022 17:23
@MelvinBot MelvinBot requested review from ctkochan22 and removed request for a team March 4, 2022 17:24
@ctkochan22
Copy link

ctkochan22 commented Mar 7, 2022

The bit of code change looks good, but I'd feel better if there was someone more familiar with the new dot deploy to make sure the version stuff check sout

@roryabraham
Copy link
Contributor

roryabraham commented Mar 7, 2022 •
edited
Loading

Something weird happened here, be careful @danieldoglas. I'm not seeing any conflicts, but typically what happens if an auto-generated PR has conflicts is that we commit the conflicts, including the <<<HEAD comments, etc.... Then the assignee (you, in this case), must manually resolve conflicts and submit them for review.

I'm very suspicious in this case because I don't see any conflicts. I'm not sure what happened here except that this line failed for some reason.

@danieldoglas
Copy link
Contributor

danieldoglas commented Mar 7, 2022

@roryabraham sorry, I think I'm missing something. There was a conflict commited, which I manually solved and commited, so it seems like the right flow, right?

@roryabraham
Copy link
Contributor

roryabraham commented Mar 7, 2022 •
edited
Loading

@roryabraham sorry, I think I'm missing something. There was a conflict commited, which I manually solved and commited, so it seems like the right flow, right?

Oh, completely missed the fact that you manually resolved conflicts. :oh_nothing:

So you did everything right and I've just got a case of the Mondays. ignore me...

@roryabraham
Copy link
Contributor

roryabraham commented Mar 7, 2022

@TomatoToaster for review since these changes were originally yours.

TomatoToaster
TomatoToaster approved these changes Mar 7, 2022
View reviewed changes
Copy link
Contributor

@TomatoToaster TomatoToaster left a comment

Choose a reason for hiding this comment

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

LGTM

@TomatoToaster TomatoToaster merged commit b0637b3 into staging Mar 7, 2022
@TomatoToaster TomatoToaster deleted the OSBotify-cherry-pick-staging-8011 branch March 7, 2022 19:44
@OSBotify
Copy link
Contributor Author

OSBotify commented Mar 7, 2022

🚀 Cherry-picked to staging by @TomatoToaster in version: 1.1.41-5 🚀

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.

@francoisl
Copy link
Contributor

francoisl commented Mar 8, 2022

Looks like something is still wrong with this PR and I don't understand exactly how it happened. The code for OptionsListUtils.getAvatarSources() that's used in #8011 doesn't exist on tag 1.1.41-5 (it was originally added in this PR), and that's causing a crash.

@francoisl francoisl mentioned this pull request Mar 8, 2022
Closed
@danieldoglas
Copy link
Contributor

danieldoglas commented Mar 9, 2022

That's strange. That was not a conflict for me.

@Julesssss
Copy link
Contributor

Julesssss commented Mar 9, 2022

Resolved with a PR which reverted this CP.

Discussed here in Slack if anyone is interested.

@OSBotify
Copy link
Contributor Author

OSBotify commented Mar 11, 2022

🚀 Deployed to production by @francoisl in version: 1.1.41-6 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ctkochan22 ctkochan22 Awaiting requested review from ctkochan22

1 more reviewer

@TomatoToaster TomatoToaster TomatoToaster approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@danieldoglas danieldoglas

Labels

automerge Engineering Hourly KSv2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@OSBotify @ctkochan22 @roryabraham @danieldoglas @francoisl @Julesssss @TomatoToaster