Repository navigation
[TAN-8197] Make manual sms campaign's consent false by default - #14325
Conversation
…itizenLabDotCo/citizenlab into TAN-8197-improve-sms-campaigns-consent
|
…AN-8197-improve-sms-campaigns-consent
jamesspeake
left a comment
jamesspeake
left a comment
There was a problem hiding this comment.
Looks good to me, I just feel that clicking an additional box to agree to receive SMS OTP codes seems overkill if the text above the box is really clear that this is what is happening. Can't recall ever having to do this on a website personally.
In fact, take a look at Twilio's guidance. We do have to store that they have consented, but it does not need to be a checkbox: https://www.twilio.com/docs/verify/consent-opt-in
| example 'records the opt-out when the user does not opt in' do | ||
| do_request(confirmation: { code: user.new_phone_confirmation.code, sms_manual_campaign_consent: false }) | ||
| assert_status 200 | ||
| consent = EmailCampaigns::Consent.find_by(user: user, campaign_type: sms_manual_type) | ||
| expect(consent.consented).to be false | ||
| end |
There was a problem hiding this comment.
Surely this is a route that should never be hit? If the user does not consent then we cannot send them a code and therefore we should not even be allowing the form to submit. Worth documenting in the test, if this is the case.
There was a problem hiding this comment.
Surely this is a route that should never be hit? If the user does not consent then we cannot send them a code and therefore we should not even be allowing the form to submit. Worth documenting in the test, if this is the case.
This is for the manual sms campaigns. This is for the case where the user had previously consented to receiving the manual sms campaigns and then they are submitting a new phone number without consenting to the manual sms campaigns.
There was a problem hiding this comment.
About the consent for OTP I agree! I changed it up and now there is only a text below the submit button like in the Twilio docs. Also I record the consent for that with an activity as I assume we need to record every time the user consents to receiving a phone. Like twilio says "Treat any recipient who has not opted in as opted out by default. You must store evidence of each consent event and provide it to Twilio on request."
…AN-8197-improve-sms-campaigns-consent
|
@jamesspeake I re-requested your review because I made some significant changes related to the consent, like a adding a new |
jamesspeake
left a comment
jamesspeake
left a comment
There was a problem hiding this comment.
Looks generally good. The comment about code sharing is not a blocker, but take a look and see what you think (I've not fully thought through what's possible)
| def record_sms_confirmation_consent | ||
| consent = EmailCampaigns::Consent.find_or_initialize_by( | ||
| user_id: current_user.id, | ||
| campaign_type: EmailCampaigns::Campaigns::NewPhoneConfirmation.name | ||
| ) | ||
| consent.update!(consented: true) | ||
| EmailCampaigns::SideFxConsentService.new.log_consent_event(consent, current_user) | ||
| end |
There was a problem hiding this comment.
Feel like this has a lot of shared code with the confirmations controller and might be better as a single method in the sideFxService eg record_sms_consent which takes some parameters.
There was a problem hiding this comment.
I agree! At first I moved it to the SideFxService (here) but then it didn't feel right because I think that SideFxServices are only for logging and shouldn't do any DB mutations. So, I have landed on another solution (here) which I think fits better. WDYT ? @jamesspeake
|
Ran the failing e2e test locally and it passed |
3 participants
Changelog
Added
Changed
Technical
Consentablenow supports opt-in campaigns via aconsented_by_default?class method; email campaigns keep the existing opt-out behaviour.For translators