Skip to content

Show Closed Rooms as "Deleted" in E.Cash#4023

Merged
yuwenmemon merged 11 commits intomainfrom
yuwen-deletedRooms
Jul 21, 2021
Merged

Show Closed Rooms as "Deleted" in E.Cash#4023
yuwenmemon merged 11 commits intomainfrom
yuwen-deletedRooms

Conversation

@yuwenmemon
Copy link
Contributor

@yuwenmemon yuwenmemon commented Jul 13, 2021

@TomatoToaster pleas review

HOLD ON https://github.com/Expensify/Auth/pull/5781

Details

Show Closed (status=2, state=2) Default Chat rooms as "Deleted" in E.Cash

Fixed Issues

For https://github.com/Expensify/Expensify/issues/165433

Tests/QA

  • Create a new Policy on the legacy (oldDot) app, invite some users to it (make sure it's on an expensify domain so you have access to the beta)
  • Open up E.cash, make sure that you can load up the #announce and #admins chat reports for that policy
  • Delete the policy
  • Reload the E.cash app, make sure that the #announce/admins rooms now show up as deleted:

Screen Shot 2021-07-13 at 4 26 52 PM

- Make sure that the icon for the room has been replaced with a trash can icon - Make sure that the room shows up with `(deleted)` in the title - Make sure that the room input is disabled - Make sure that the room detail section does not contain any editable actions about the room (should just show an empty page)

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-07-13 at 4 31 12 PM

Screen Shot 2021-07-13 at 4 30 53 PM

Screen Shot 2021-07-13 at 4 30 55 PM

Screen Shot 2021-07-13 at 4 31 03 PM

Screen Shot 2021-07-13 at 4 31 07 PM

Mobile Web

Desktop

iOS

Android

@yuwenmemon yuwenmemon requested a review from TomatoToaster July 13, 2021 23:30
@yuwenmemon yuwenmemon self-assigned this Jul 13, 2021
@yuwenmemon yuwenmemon requested a review from a team as a code owner July 13, 2021 23:30
@MelvinBot MelvinBot requested review from sketchydroide and removed request for a team July 13, 2021 23:30
@yuwenmemon yuwenmemon changed the title Show Closed Rooms as "Deleted" in E.Cash [WIP] Show Closed Rooms as "Deleted" in E.Cash Jul 13, 2021
@yuwenmemon yuwenmemon changed the title [WIP] Show Closed Rooms as "Deleted" in E.Cash [HOLD - AUTH#5775] Show Closed Rooms as "Deleted" in E.Cash Jul 14, 2021
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.

Just had one suggestion but otherwise everything looks good 👍🏾 .

@yuwenmemon yuwenmemon changed the title [HOLD - AUTH#5775] Show Closed Rooms as "Deleted" in E.Cash Show Closed Rooms as "Deleted" in E.Cash Jul 15, 2021
@yuwenmemon yuwenmemon changed the title Show Closed Rooms as "Deleted" in E.Cash [HOLD AUTH#5781] Show Closed Rooms as "Deleted" in E.Cash Jul 16, 2021
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

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.

Actually wait, there's just one thing I caught.

blockedFromConcierge: 'Communication is barred',
youAppearToBeOffline: 'You appear to be offline.',
fileUploadFailed: 'Upload Failed. File is not supported.',
localTime: ({user, time}) => `It's ${time} for ${user}`,
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 you're not using this right? I think this was changed in main since you made this branch.

It looks like it was removed here: https://github.com/Expensify/Expensify.cash/pull/4020/files

writeSomething: 'Escribe algo...',
blockedFromConcierge: 'Comunicación no permitida',
youAppearToBeOffline: 'Parece que estás desconectado.',
localTime: ({user, time}) => `Son las ${time} para ${user}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, I think this can be deleted.

TomatoToaster
TomatoToaster previously approved these changes Jul 16, 2021
@TomatoToaster
Copy link
Contributor

There's a bug with K2 on ECash repos. The button is green but don't merge it's on HOLD!

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@yuwenmemon
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@yuwenmemon yuwenmemon changed the title [HOLD AUTH#5781] Show Closed Rooms as "Deleted" in E.Cash Show Closed Rooms as "Deleted" in E.Cash Jul 20, 2021
@yuwenmemon
Copy link
Contributor Author

Removing HOLD!

@yuwenmemon
Copy link
Contributor Author

@sketchydroide please feel free to give this a look now!

@sketchydroide
Copy link
Contributor

will probably do it tomorrow as it's a bit late for me :)

@yuwenmemon
Copy link
Contributor Author

Of course buddy :) no worries!

Copy link
Contributor

@sketchydroide sketchydroide left a comment

Choose a reason for hiding this comment

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

LGTM

@sketchydroide
Copy link
Contributor

Code looks good, but I won't be able to test it, as I'm pretty much taking all my time to fill out the relocation excel file and the notes, and I'm OOO the next couple days moving house.

@yuwenmemon yuwenmemon merged commit 2d673cc into main Jul 21, 2021
@yuwenmemon yuwenmemon deleted the yuwen-deletedRooms branch July 21, 2021 16:55
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-5🚀

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

@isagoico
Copy link

'Deleted' is not displayed in room channel after policy has been deleted

Action Performed

Step 1:

  1. Create a policy in OldDot
  2. Add a member to the policy
  3. Navigate to NewDot and search for the #admin room
  4. Send a message in the #Admin room
  5. Navigate to OldDot and delete the policy
  6. Navigate to NewDot and check that the #Admin room for the policy has the deleted status.

Expected Result

#announce/admins rooms should show up as deleted.

Actual Result

The #announce/admins rooms doesn't have the deleted status. The only thing visible is the Trash Can icon.

Notes/Images/Video

image

@isagoico isagoico added the DeployBlockerCash This issue or pull request should block deployment label Jul 26, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@yuwenmemon
Copy link
Contributor Author

Ah whoops! Fix incoming ^

@TomatoToaster
Copy link
Contributor

Forgot to mention, but that is merged, this is ready for a retest!

@isagoico
Copy link

@yuwenmemon @TomatoToaster when testing this we noticed that the LHN does not update after the refresh. It only updates and shows the deleted status when the user clicks ok the conversation
screen_shot_2021-07-27_at_2 24 46_pm

@yuwenmemon
Copy link
Contributor Author

Hmmm.. that might be a symptom of the bug that was just fixed? Can we retest on a fresh account?

@yuwenmemon
Copy link
Contributor Author

Just retested locally, worked fine for me:
Screen Shot 2021-07-27 at 12 06 03 PM

@isagoico
Copy link

Tester just confirmed he tried on a new account. He's testing again just in case.

@isagoico
Copy link

Tester has the same behaviour in windows chrome. Maybe we could open this as a separate issue?

@yuwenmemon
Copy link
Contributor Author

@isagoico yeah, let's do that and remove the blocker since I wouldn't want to hold up the deploy while I look into this (it doesn't seem obvious since I'm not able to reproduce)

@yuwenmemon yuwenmemon removed the DeployBlockerCash This issue or pull request should block deployment label Jul 27, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.80-2🚀

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants