feat: [UEPR-187] [UEPR-192] [UEPR-194] ToU modal display logic #9248
Conversation
…vate/lock-file-maintenance fix(deps): lock file maintenance
…vate/scratch-l10n-5.x fix(deps): update dependency scratch-l10n to v5.0.120
…vate/scratch-l10n-5.x fix(deps): update dependency scratch-l10n to v5.0.121
…vate/babel-monorepo chore(deps): update babel monorepo to v7.26.8
…vate/scratch-l10n-5.x fix(deps): update dependency scratch-l10n to v5.0.122
…vate/scratch-l10n-5.x fix(deps): update dependency scratch-l10n to v5.0.123
…vate/scratch-storage-4.x fix(deps): update dependency scratch-storage to ^4.0.49
…vate/formatjs-monorepo chore(deps): update formatjs monorepo
…vate/scratch-l10n-5.x fix(deps): update dependency scratch-l10n to v5.0.124
…vate/lock-file-maintenance fix(deps): lock file maintenance
…vate/postcss-8.x chore(deps): update dependency postcss to v8.5.2
…vate/scratch-l10n-5.x fix(deps): update dependency scratch-l10n to v5.0.127
…vate/lock-file-maintenance fix(deps): lock file maintenance
…vate/scratch-l10n-5.x fix(deps): update dependency scratch-l10n to v5.0.128
…vate/scratch-storage-4.x fix(deps): update dependency scratch-storage to ^4.0.50
| className="tou-banner-link" | ||
| // eslint-disable-next-line react/jsx-no-bind | ||
| onClick={() => { | ||
| setIsModalVisible(true); |
There was a problem hiding this comment.
Why not wrap in useCallback instead of ignoring warning?
src/components/page/www/page.jsx
Outdated
| @@ -17,6 +18,7 @@ const Page = ({ | |||
| showDonorRecognition | |||
| }) => ( | |||
| <ErrorBoundary componentName="Page"> | |||
| <TermsOfServiceModal /> | |||
There was a problem hiding this comment.
TermsOfUseModal to keep the naming consistent?
| @@ -0,0 +1,44 @@ | |||
| import React, {useState} from 'react'; | |||
| import {injectIntl, FormattedMessage} from 'react-intl'; | |||
| import TosModalReminderUnder16 from './variations/tou-reminder-under-16.jsx'; | |||
There was a problem hiding this comment.
What do you think about using the whole name TermsOfUse ? Additionally, I think we can omit the under-16 since that's a detail regarding how it's used and since we don't have a reminder modal in the other case, we don't need the distinction. So, finally, the name of the file can become terms-of-use-reminder
| @@ -0,0 +1,44 @@ | |||
| import React, {useState} from 'react'; | |||
| import {injectIntl, FormattedMessage} from 'react-intl'; | |||
| import TosModalReminderUnder16 from './variations/tou-reminder-under-16.jsx'; | |||
There was a problem hiding this comment.
Regarding the file hierarchy - there is a folder under components - modal, where other modals are located. If you want to group terms of use modals together, I'd recommend a hierarchy such as
components/modal/terms-of-use/index.jsx
components/modal/terms-of-use/over-16/index.jsx
components/modal/terms-of-use/under-16/index.jsx
components/modal/terms-of-use/under-16/reminder.jsx
components/modal/terms-of-use/under-16/account-locked.jsx
That way we can even keep the under-16 / over-16 (since in the case of the initial modal it is a valid distinction) in the path name.
| @@ -60,7 +60,20 @@ module.exports.requestSessionWithRetry = (resolve, reject, retriesLeft, totalDel | |||
| nextTimeout | |||
| ); | |||
| } | |||
| return resolve(body); | |||
| console.log('session body:', body); | |||
There was a problem hiding this comment.
reminder not to forget that when merging
src/lib/session.js
Outdated
| ...body, | ||
| flags: { | ||
| ...body.flags, | ||
| hasAgreedToLatestTermsOfService: false, |
There was a problem hiding this comment.
Can you update the logic to use the fields described here - https://scratchfoundation.atlassian.net/wiki/spaces/SET/pages/935100436/Terms+of+Service+confirmation#Backend-changes%3A
Namely, the exposed fields in session will be
has_accepted_terms_of_use → calculated based on TermsOfUseConsent.accepted_date && TERMS_OF_USE_UPDATED_DATE > TermsOfUseConsent.accepted_date
is_in_terms_of_use_grace_period -> calculated based on TermsOfUseConsent.grace_period_expiry_date && TermsOfUseConsent.grace_period_expiry_date >= now().toDate()
is_after_terms_of_use_grace_period → calculated based on !has_accepted_terms_of_use && TermsOfUseConsent.grace_period_expiry_date < now().toDate()`
I know that the fields described in the task description were different, but the initial idea had to evolve since then. Can you also update the task description? 🙇
| @@ -356,8 +358,16 @@ class SplashPresentation extends React.Component { // eslint-disable-line react/ | |||
| 'teacherbanner.faqButton': formatMessage({id: 'teacherbanner.faqButton'}) | |||
| }; | |||
|
|
|||
|
|
|||
| const shouldShowToUBanner = (this.props.userUsesParentEmail && | |||
| !this.props.hasAgreedToLatestTermsOfService && | |||
There was a problem hiding this comment.
This should then depend on this.props.userUsesParentEmail && !this.props.hasAgreedToTermsOfUse && this.props.isInTermsOfUseGracePeriod.
Also, please verify with Anton once more that we don't want to show the banner after the grace period.
| onClose={() => { | ||
| setIsModalVisible(false); | ||
| }} | ||
| isOpen={isModalVisible} |
There was a problem hiding this comment.
Isn't it redundant to have isModalVisible both as a condition for visualising the modal, and in the isOpen prop here? Or is this necessary for some case?
|
I don't see a reason for a pop-up though? The last update to ToU banner had no pop-up, just saying it has been updated and a link to the ToU. |
|
|
||
| }; | ||
|
|
||
| // eslint-disable-next-line arrow-body-style |
There was a problem hiding this comment.
We don't have to disable the rule but omit the return. And do it like this:
const TermsOfUseBanner = () => ( <div className="tou-banner"> <FormattedMessage id="tos.banner.parentAgreement" values={{ a: TermsOfUseLink }} /> </div> );
Same for the other similar components
| }; | ||
|
|
||
| // eslint-disable-next-line arrow-body-style | ||
| const Step1 = ({onNextStep}) => { |
There was a problem hiding this comment.
Can we change the name of the component to something more explanatory? Same for src/components/tou-modal/variations/tou-reminder-under-16.jsx and src/components/tou-modal/variations/tou-under-16.jsx etc
src/views/preview/preview.jsx
Outdated
| <ProjectView.View />, | ||
| <div> | ||
| <TermsOfServiceModal /> | ||
| <ProjectView.View /> |
There was a problem hiding this comment.
Project page returns either PreviewPresentation wrapped in Page or IntlGUIWithProjectHandler. Probably we can add TermsOfServiceModal only to IntlGUIWithProjectHandler
| }) => { | ||
|
|
||
| const [isModalVisible, setIsModalVisible] = useState(true); | ||
|
|
There was a problem hiding this comment.
Remove extra empty line
| usesParentEmail, | ||
| }) => { | ||
|
|
There was a problem hiding this comment.
Nitpick: remove empty line
|
|
||
|
|
||
| const page = window.location.pathname.split('/')[1]; | ||
| if (noShowModalPages.includes(page)) { |
There was a problem hiding this comment.
Let's move this logic in the page component. It makes more sense for the parent of a component to know when a child component should be rendered, than the component itself.
| </p> | ||
| <input className="tou-input" /> | ||
| <div className="tou-modal-button-container"> | ||
| <button |
There was a problem hiding this comment.
There is src/components/forms/button.jsx we can use it
| {currentStep === 1 ? ( | ||
| <Step1 | ||
| onNextStep={handleNextStep} | ||
| email={email} | ||
| /> | ||
| ) : ( | ||
| <TosEmailSentStep /> | ||
| )} |
There was a problem hiding this comment.
I am wondering and this is not really a must but do you think it would be better to have arrays with the steps and to display the component on the index corresponding to the currentStep?
| } | ||
|
|
||
| .tou-modal-link { | ||
| color: $ui-mint-green !important; |
There was a problem hiding this comment.
Can we rewrite this to avoid the !important or is it necessary?
There was a problem hiding this comment.
without it the style is not properly applied
…s and changes in logic
| // eslint-disable-next-line arrow-body-style | ||
| const TosModalReminderUnder16 = ({onClose, isOpen, email}) => { | ||
| const [currentStep, setCurrentStep] = useState(1); | ||
| const handleNextStep = () => { |
|
|
||
|
|
||
| // eslint-disable-next-line arrow-body-style | ||
| const TosModalReminderUnder16 = ({onClose, isOpen, email}) => { |
There was a problem hiding this comment.
I agree that it's more convenient like that, but let's write components adhering to the style guide of the project, so we don't need to ignore eslint
| /> | ||
| </p> | ||
| <p> | ||
| <FormattedMessage id="tos.over16.acceptConfirmation" /> |
There was a problem hiding this comment.
tos -> termsOfUse
generally it's better to avoid abbreviations where possible, as that tends to confuse readers not familiar with that part of the code
| defaultValue={email} | ||
| /> | ||
| <div className="tou-modal-button-container"> | ||
| <button |
There was a problem hiding this comment.
Missed replacing the button component here.
src/views/preview/project-view.jsx
Outdated
| onActivateDeck={this.props.onActivateDeck} | ||
| /> | ||
| <> | ||
| <TermsOfServiceModal /> |
There was a problem hiding this comment.
I imagined that we would put TermsOfServiceModal inside IntlGUIWithProjectHandler as the component already acts as a wrapper to gui.
| import React, {useCallback, useState} from 'react'; | ||
| import {injectIntl, FormattedMessage} from 'react-intl'; | ||
| import TermsOfUseReminderModal from './under16/terms-of-use-reminder.jsx'; | ||
|
|
There was a problem hiding this comment.
IMO, following this file hierarchy, there is no need to include terms-of-use in the naming of the file themselves as that is already qualified by the route. It is still good to include it in the naming of the component function
|
There seems to be some linting issues according to the workflow |
|
|
||
| require('./terms-of-use-modal.scss'); | ||
|
|
||
| export const noShowTermsOfUseModalPages = ['terms_of_use', 'privacy_policy', 'dmca', 'cookies']; |
There was a problem hiding this comment.
Let's move this const to the page component, where it is used
The ST would probably end up in a lot of legal traps if they silently update the ToU. Just one typo fix would be fine for a silent update, but this is changing user rights, which requires a notification. |
There was a problem hiding this comment.
Typo: "the Scratch's" should just be "Scratch's"
@Bogomil-Stoyanov
Ticket:
UEPR-192
UEPR-194
UEPR-187
Adds the logic for displaying the new ToU modals + banner
Banner:
Reminder modal (opens from banner):
ToU Modal under 16:
ToU Modal over 16:
Locked Account modal:
