TAN-7638 Remove email confirmation FF#13804
Conversation
|
…tizenLabDotCo/citizenlab into TAN-7638-remove-email-confirmation-FF
…d invite what the hell?
…the unconfirmed_user
jamesspeake
left a comment
jamesspeake
left a comment
There was a problem hiding this comment.
Nice, so many improvements. Sorry took me a while to get through. A few small comments, nothing blocking though. Obviously the main blocker is the one customer still using it!
Have you run the e2e tests on this branch btw? Assume they are fine.
| post 'request_code_unauthenticated', to: 'request_codes#request_code_unauthenticated' | ||
| post 'request_code_authenticated', to: 'request_codes#request_code_authenticated' |
There was a problem hiding this comment.
Nit: could this endpoint be request_code_auth_flow or something similar now?
There was a problem hiding this comment.
I have absolutely no idea what this file is for!
|
|
||
| def create? | ||
| active? && | ||
| (record.user_id == user.id) && |
There was a problem hiding this comment.
Why is this being removed? Seems unrelated
There was a problem hiding this comment.
This means that users can now create responses for other users, but this is not possible for polls is it? Surveys yes, but not polls, so not sure why this change is needed.
There was a problem hiding this comment.
I removed this because a test related to email confirmation suddenly started failing, and I was looking at this code to debug the test. And the whole thing just made no sense. If you are the creator, record.user_id is always going to be your id. There is no way to supply a user_id parameter so completely unnecessary check
| describe do | ||
| let(:email) { 'Super.Boulette@hotmail.com' } | ||
|
|
||
| example 'Accept an invite using different capitalization for the email', document: false do | ||
| do_request | ||
| assert_status 200 | ||
| end | ||
| end |
There was a problem hiding this comment.
This is still a valid test isn't it?
| roles { (project_ids || projects&.map(&:id)).uniq.map { |id| { type: 'project_moderator', project_id: id } } } | ||
| after(:build, &:confirm) |
There was a problem hiding this comment.
nit: Might be nice to make the code here consistent with folder and space moderator
| context 'confirmed users with no password' do | ||
| let(:unconfirmed_user) { create(:unconfirmed_user) } |
There was a problem hiding this comment.
This does not make sense. Should you not create a confirmed user here?
| @@ -121,16 +120,6 @@ | |||
| expect(User.exists?(user_id)).to be false | |||
| end | |||
There was a problem hiding this comment.
I know you did not write this test but think the test name is confusing. Maybe 'deletes the previous user if user has never confirmed their email address and they have a password' which I think is the logic in AuthenticationService.prevent_user_account_hijacking
Could also test be rewritten with :unconfirmed_user.
| expect(user.confirmation_required?).to be(false) | ||
| end | ||
|
|
||
| context 'when email is not verified' do |
There was a problem hiding this comment.
| context 'when email is not verified' do | |
| context 'when email is not confirmed' do |
| before { SettingsService.new.activate_feature!('user_confirmation') } | ||
|
|
||
| let(:invitee) { create(:user_with_confirmation, invite_status: 'pending') } | ||
| let(:invitee) { create(:unconfirmed_user, invite_status: 'pending') } |
There was a problem hiding this comment.
| let(:invitee) { create(:unconfirmed_user, invite_status: 'pending') } | |
| let(:invitee) { create(:invited_user) } |
| example_request 'Create JWT token with 1 day expiration' do | ||
| assert_status 201 | ||
| example_request 'create JWT token with default expiration' do | ||
| expect(status).to eq(201) |
There was a problem hiding this comment.
| expect(status).to eq(201) | |
| assert_status 201 |
|
Just noticed there is a failing test too |
3 participants
Changelog
Changed
user_confirmation(email confirmation) feature flag. Email confirmation is now always enabled. This should only affect Viña del MarTechnical
userfactory for specs (the defaults were really weird and confusing, and making some tests pass when they should not)