-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 checkout login permissions #2752
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the register issue and the comment
|
||
if (currentStatus !== self.template && guestUser === true && anonUser === false) { | ||
// when user is guest, they are also anonymous. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe this better? i.e to explain what the check below it does?
Seeing when user is guest, they are also anonymous
doesn't particular help a reader to understand why we do this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@impactmass I have fixed the changes you requested, and updated the comments. Please confirm it works well now
There's an error from the Circle CI tests. can you confirm why it failed? |
@impactmass they're passing now. |
@@ -17,10 +16,9 @@ Template.checkoutLogin.helpers({ | |||
const cart = Cart.findOne(); | |||
if (cart && cart.workflow) { | |||
const currentStatus = cart.workflow.status; | |||
const guestUser = Reaction.hasPermission("guest", Meteor.user()); | |||
const anonUser = Roles.userIsInRole("anonymous", Meteor.user(), Reaction.getShopId()); | |||
const guestUser = Reaction.hasPermission("guest", Meteor.userId(), Reaction.getShopId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need Meteor.userId() and getShopId(), those are supplied as default.
I worry about wether you should get rid of the permission check of "guest" and "anonymous". I suspect this will have unexpected consequences in the order/completed
page, and if you register a user in the Accounts UI-> not being in the checkout flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to remove it because when clicking to continue as guest, the user still has the anonymous permission. So if we are checking to ensure that they are not anonymous, it fails. So that's why I just check if they are guest alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you click as "continue as guest" you ARE still anonymous
. you are only NOT anonymous if you have supplied an email, such as when you add an email at the order/completed stage. (at least that's how I remembered it, you might want to look closely at that and review other parts where this logic is used to verify)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is how it is, but we were preventing anonymous
users from "continuing as guest". which is why I removed the permission for anonymous
. I will retest and comment guest vs anonymous login view behavior as requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retest and comment guest vs anonymous login view behavior. Implement hasPermissions, don't supply params that are defaults.
@aaronjudd I updated the comments. Please take a look. |
See #2328 as well, may be related |
@aaronjudd Is there more to do on this? Should I have someone else pick it up or are we close to merging? |
@rymorgan Note, while we closed this -> one side affect / repercussion -> the inline login in the checkout is supposed to show "REGISTER" as the default view, while the dropdown is supposed to show "SIGN IN" as the default view. |
@aaronjudd Got it. I've made an issue to correct it. #2944 |
This resolves #2657
Steps to test