Bugfix: Clear in_person_verification_pending_at on fraud deactivation#10222
Bugfix: Clear in_person_verification_pending_at on fraud deactivation#10222
Conversation
Jack found a sneaky bug, where the UX was correct, but the profile would not be able to be reactivated because validation prevents an in_person_verification_pending profile from activation. Now fixed!
| expect(profile.fraud_review_pending?).to eq(true) | ||
| expect(profile.fraud_rejection?).to eq(false) | ||
| expect(profile.fraud_pending_reason).to eq('threatmetrix_review') | ||
| expect(profile.pending_reasons).to eq([:fraud_check_pending]) |
There was a problem hiding this comment.
Interesting aside: this is what was previously returning [:in_person_verification_pending] causing the problem. I set out to assert that this was an empty array, but it is not.
The action-account code handles this, and it's correct behavior, but it wasn't entirely intuitive in the moment.
spec/models/profile_spec.rb
Outdated
| expect(profile.fraud_review_pending?).to eq(false) # to change | ||
| expect(profile.gpo_verification_pending_at).to be_nil | ||
| expect(profile.in_person_verification_pending_at).to be_nil | ||
| expect(profile.in_person_verification_pending_at).not_to be_nil # to change |
There was a problem hiding this comment.
👍 or 👎 on this comment (and the one below on 1038)? I was following the pattern established on 1024, but am of half a mind to think that it could be misread as meaning that this behavior is subject to change in a future sprint, rather than that we test that this value has been changed.
(And is it bad that most of the test is devoted to assertions that don't change and we need comments to highlight what does?) 🤔
There was a problem hiding this comment.
I vote 👍 - I like when things like this are pointed out like this when there is a lot of data. Also, because it is in a test file- I did not interrupt the comment as a change in a future sprint.
There was a problem hiding this comment.
if we include this comment I would include when it should be changed so we have an indicator as to when the comment should be removed.
I lean toward not including the comment on line 1038
There was a problem hiding this comment.
I think we can use RSpec to write specs that actually highlight what should change, and get better error messages too
expect { profile.deactivate_for_fraud_review }.to(
change { profile.reload.in_person_verification_pending_at }.to(nil)
.and(change { profile.reload.fraud_review_pending?).from(false).to_true)),
)
| }.to(nil) | ||
|
|
||
| expect(profile).to_not be_active | ||
| expect(profile.fraud_review_pending?).to eq(true) |
There was a problem hiding this comment.
It appears that this line has always been redundant; it was tested before and after above already.
spec/models/profile_spec.rb
Outdated
| }.from(false).to(true). | ||
| and change { | ||
| profile.in_person_verification_pending_at | ||
| }.to(nil) |
There was a problem hiding this comment.
rubocop -A made an absolute abomination of this formatting. I tried to clean it up by hand, but rubocop wasn't having it.
@zachmargolis You had provided an example that was considerably less ugly. Do you have any thoughts on how to make this look more like that without rubocop rejecting it?
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
4 participants
🎫 Ticket
TK
🛠 Summary of changes
deactivate_for_fraud_reviewwas not clearingin_person_verification_pending_at. This didn't affect anything on the site and thus went unnoticed until now.But, Profile validation prevents activating an account that is
in_person_verification_pending?, which means that anyone flagged for fraud could not have their account reinstated after a manual review. This fixes that.(Someone please notice that my commit has the description perfectly fit the same length on each line. I am very proud of myself for this meaningless accomplishment.)
📜 Testing Plan
I made some small changes to the unit test for this method.
@JackRyan1989 identified this issue as part of his larger work on improved testing, so I stuck to a simple unit test change rather than steal his excellent work that will be pushed up soon.