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

Prevent users blocked from concierge from sending messages #2932

Merged
merged 13 commits into from
Jun 23, 2021

Conversation

stitesExpensify
Copy link
Contributor

Details

This takes the blocking functionality currently on web and moves it to e.cash. If the user is blocked from concierge and tries to chat with them, all buttons and the compose box should be disabled.

Fixed Issues

https://github.com/Expensify/Expensify/issues/158313

Tests/QA

  1. Log into e.cash with a banned account (bstites+permabanned@expensifail.com, Password1)
  2. Try to chat with concierge
  3. Make sure your inputs are all disabled and you can't send a message
    2021-05-13_14-37-07

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@stitesExpensify stitesExpensify requested a review from a team as a code owner May 14, 2021 17:44
@stitesExpensify stitesExpensify self-assigned this May 14, 2021
@MelvinBot MelvinBot requested review from francoisl and removed request for a team May 14, 2021 17:45
placeholder="Write something..."
placeholder={
isBlockedFromConcierge
? 'Communication is barred'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add these to a translation file?

@@ -168,6 +174,13 @@ function validateLogin(accountID, validateCode) {
});
}

function isBlockedFromConcierge(expiresAt) {
const now = moment().format('YYYY-MM-DD');
const expires = moment(expiresAt).format('YYYY-MM-DD');
Copy link
Contributor

@francoisl francoisl May 14, 2021

Choose a reason for hiding this comment

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

Just an observation but it's kinda odd we only block based on the day, and not the time. Looking at the code in Web-E where we block users, if we block them 2 days or a week but only use the day, effectively they'll be blocked less than that.

@stitesExpensify
Copy link
Contributor Author

Oops, sorry I forgot that e.cash automatically assigns a reviewer. I was planning on making a few big changes here 😬

@stitesExpensify stitesExpensify changed the title Prevent users blocked from concierge from sending messages [HOLD https://github.com/Expensify/Web-Expensify/pull/30964] Prevent users blocked from concierge from sending messages May 14, 2021
@stitesExpensify stitesExpensify changed the title [HOLD https://github.com/Expensify/Web-Expensify/pull/30964] Prevent users blocked from concierge from sending messages Prevent users blocked from concierge from sending messages Jun 21, 2021
@stitesExpensify
Copy link
Contributor Author

Off hold!

Copy link
Contributor

@francoisl francoisl left a comment

Choose a reason for hiding this comment

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

I was trying to double check the format of the NVP private_blockedFromConcierge but the permabanned account you mention in the tests doesn't exist in 1Password, can you check it's in the correct vault please?

* @param {String} expiresAt
* @returns {boolean}
*/
function isBlockedFromConcierge(expiresAt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method pretty much just compares a date with now, could it be generalized and moved to DateUtils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that I'm just using the super simple one liner you gave below, i don't think we need to put it in dateUtils

Comment on lines 193 to 198
const now = moment()
.format('YYYY-MM-DD');
const expires = moment(expiresAt)
.format('YYYY-MM-DD');

return now < expires;
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this can be simplified with moment's comparison methods, i.e. the whole method can be rewritten to something like

!_.isNull(expiresAt) && moment().isBefore(moment(expiresAt));

or to only use the date and not the time,

... moment().isBefore(moment(expiresAt), 'day');

@francoisl
Copy link
Contributor

There's a small conflict now FYI :)

@stitesExpensify
Copy link
Contributor Author

RIP. Thanks i'll take a look

@stitesExpensify
Copy link
Contributor Author

I was trying to double check the format of the NVP private_blockedFromConcierge but the permabanned account you mention in the tests doesn't exist in 1Password, can you check it's in the correct vault please?

I can see how that might have been confusing haha. The password is literally Password1, it is not in OnePassword 😄

@francoisl
Copy link
Contributor

The password is literally Password1, it is not in OnePassword 😄

🤦 Haha yikes ok, I see now. Thanks!

@stitesExpensify
Copy link
Contributor Author

Updated!

@francoisl francoisl merged commit 9cbfba7 into main Jun 23, 2021
@francoisl francoisl deleted the stites-preventBlockedUserMessages branch June 23, 2021 23:01
@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.

@roryabraham
Copy link
Contributor

I'm seeing this error on main:

Warning: Failed prop type: Invalid prop `blockedFromConcierge` of type `string` supplied to `ReportActionCompose`, expected `object`.
    in ReportActionCompose (at withOnyx.js:154)

@stitesExpensify
Copy link
Contributor Author

Hm well that's not great. Looking into it

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.73-4🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.74-0🚀

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants