added checks for confirm terms and email notifications #5664
Conversation
| <div className="text-center pb-4"> | ||
| <Logo /> | ||
| </div> | ||
| <Card className="col-md-4 mx-auto p-4 border-0 card-shadow"> |
There was a problem hiding this comment.
We can increase the width of the box here abit to stop the title from wrapping around
app/serializers/user_serializer.rb
Outdated
|
|
||
| delegate :confirm_terms, to: :object | ||
|
|
||
| delegate :email_notifs, to: :object |
There was a problem hiding this comment.
I don't think these are necessary - the serializer should be able to do this automatically (as it does with the other values (ie name, email)
There was a problem hiding this comment.
I just tested yeah your right its not needed and works fine without. removed!
| Setting.create!(name: 'ConfirmTerms') unless Setting.exists?(name: 'ConfirmTerms') | ||
|
|
||
| return if SiteSetting.exists?(setting: Setting.find_by(name: 'ConfirmTerms')) | ||
|
|
||
| SiteSetting.create!( | ||
| setting: Setting.find_by(name: 'ConfirmTerms'), | ||
| value: false, | ||
| provider: 'greenlight' | ||
| ) |
There was a problem hiding this comment.
You're doing a data migration inside of a schema migration. They should be seperated into 2 separate migrations
app/serializers/user_serializer.rb
Outdated
| @@ -19,7 +19,7 @@ | |||
| class UserSerializer < ApplicationSerializer | |||
| include Avatarable | |||
|
|
|||
| attributes :id, :name, :email, :provider, :language, :avatar, :verified, :created_at | |||
| attributes :id, :name, :email, :provider, :language, :avatar, :verified, :confirm_terms, :email_notifs, :created_at | |||
There was a problem hiding this comment.
Do we need access to email_notifs on the front end?
There was a problem hiding this comment.
Yes the email_notifs are needed on the ConfirmTerms page as along with option to accept terms they also have a toggle to receive email updates and the initial position of that toggle needs the data from the user. (email_notifs is now marketing).
| t.boolean :confirm_terms, default: false | ||
| t.boolean :email_notifs, default: false |
There was a problem hiding this comment.
For simplicities sake, I would just call this terms and marketing
| <input | ||
| className="form-check-input fs-5" | ||
| type="checkbox" | ||
| checked={isCheckedEmails} | ||
| onChange={() => setIsCheckedEmails(!isCheckedEmails)} | ||
| /> |
There was a problem hiding this comment.
The check boxes should be on the left side of the text
app/assets/locales/en.json
Outdated
| @@ -460,6 +465,12 @@ | |||
| "resend_activation_link": "If you haven't received an activation email or if you are having a problem using it, click on the button below to request a new activation email.", | |||
| "resend_btn_lbl": "Resend Verification" | |||
| }, | |||
| "confirm_terms_page": { | |||
| "title": "Confirm terms and conditions", | |||
There was a problem hiding this comment.
I think this should Accept Our Terms and Conditions
app/assets/locales/en.json
Outdated
| "account_unconfirmed": "Your account has not accepted the terms and conditions.", | ||
| "message": "Please accept the terms and conditions for Greenlight.", |
There was a problem hiding this comment.
It might be better to phrase this a bit more gentle. Also, we don't really refer to the app as Greenlight to the end user since they only know it as BBB.
To continue using BigBlueButton, please accept our terms and conditions
| "title": "Confirm terms and conditions", | ||
| "account_unconfirmed": "Your account has not accepted the terms and conditions.", | ||
| "message": "Please accept the terms and conditions for Greenlight.", | ||
| "confirm_btn_lbl": "Confirm" |
There was a problem hiding this comment.
Is there no plain confirm button that we can reuse from somewhere else?
There was a problem hiding this comment.
Couldn't find one so I reused the resend button from the VerifyAccount page for a plain confirm.
|
Fixes #5638
Screenshots of new sign up page additions and page existing users that have not confirmed terms will see if the site setting is enabled