New Terms of Service Dialog #9975

MrFlashAccount · 2024-05-16T16:42:17Z

Pull Request Description

Tl;dr

Closes: enso-org/cloud-v2#1228
This PR adds a new DIalog that requires user to submit terms and conditions

Demo Presentation

Note during the demo the changes on the website were not merged yet, so when I clicked "view terms and conditions" link, I was redirected to the / page. Once the changes on the website are merged, this link will lead to the "Terms and Conditions" page

CleanShot.2024-05-17.at.11.55.32.mp4

This Change:

What this change does in the larger context. Specific details to highlight for review:

Adds a new dialog that blocks the interactions with the dashboard until the user accepts terms and conditions
each 15 mins checks for changes in TOS policy(by fetching eula.json from the website with hash based on EULA content) and if the hash is different from the stored one, show the TOS dialog
Improves loading behavior (now 2 spinners are merged into the one)

Test Plan:

We expect to test the changes after merging https://github.com/enso-org/website/pull/7

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

MrFlashAccount · 2024-05-17T08:30:35Z

app/ide-desktop/lib/dashboard/src/modals/TermsOfServiceModal.tsx

+        }
+      })
+      .then(data => {
+        invariant(data != null, 'Invalid terms of service response')


@somebody1234 I think it's time to add a schema-validation library, WDYT?

we already have ajv as a dependency, is that not sufficient?

i guess ajv doesn't do type predicates...

I guess ajv should be sufficient

Do we need ajv though?

app/ide-desktop/lib/dashboard/src/modals/TermsOfServiceModal.tsx

app/ide-desktop/lib/dashboard/src/providers/LocalStorageProvider.tsx

somebody1234 · 2024-05-17T09:19:56Z

CR: mostly fine apart from the points above?
i'm not sure about:

firstly, requiring the user to go to another page to see the EULA
secondly, putting the EULA below the confirmation checkbox

ideally we'd be showing the EULA in the app itself - I'm not sure whether an iframe works both in browser and in Electron, but if it is then that would be sufficient IMO (although we would probably want a special HTML page without the website header and footer, alongside the existing one

PabloBuchu · 2024-05-17T09:19:59Z

How do I QA this?

somebody1234 · 2024-05-17T09:21:38Z

@PabloBuchu see the PR description:

We expect to test the changes after merging https://github.com/enso-org/website/pull/7

MrFlashAccount · 2024-05-17T10:48:14Z

@somebody1234 , @PabloBuchu just merged the changes on the website and I expect this PR to be ready for QA in a few minutes.

MrFlashAccount · 2024-05-17T13:49:40Z

PR is ready for review/QA

PabloBuchu · 2024-05-20T11:04:29Z

@MrFlashAccount truly minor things. When I hit Accept without checking the terms n cons. Can we make the checkbox and the text red? and maybe add Please accept before continuing

Also I think the checkbox should toggle when I click on text not only exactly in the square

MrFlashAccount · 2024-05-20T11:49:16Z

Also I think the checkbox should toggle when I click on text not only exactly in the square

True, we're currently limited with the components we have. Eventually, we'll improve this. I moved this out of scope because this will unreasonably increase the scope of the PR.

MrFlashAccount · 2024-05-20T11:50:25Z

When I hit Accept without checking the terms n cons. Can we make the checkbox and the text red? and maybe add Please accept before continuing

We currently focus on the checkbox but it is barely visible. Let's add an error message

somebody1234 · 2024-05-20T11:52:33Z

True, we're currently limited with the components we have

not sure this is true, a plain HTML label is more than enough to make this work. (similarly a react-aria-components Checkbox also provides this functionality out of the box.)

MrFlashAccount · 2024-05-20T13:58:45Z

not sure this is true, a plain HTML label is more than enough to make this work. (similarly a react-aria-components Checkbox also provides this functionality out of the box.)

React aria expects the label to be passed as a child: https://react-spectrum.adobe.com/react-aria/Checkbox.html#example
But if I do so, our current checkbox implementation puts the label inside the checkmark box.
I tried to connect the external label with the checkbox component but it seems like not working.

Reimplementing the checkbox will increase the scope of the feature and delay the release

Checkbox
Documentation for Checkbox in the React Aria package.

somebody1234 · 2024-05-20T14:17:15Z

tried to connect the external label with the checkbox component

HTML label should work fine without any ids if you just contain both the checkbox and the text inside the label. but if that doesn't work then it's fine to leave it as-is for now.

MrFlashAccount · 2024-05-20T16:00:20Z

@somebody1234 can we approve the code and QA then?

somebody1234

CR ✅
QA not done (it's late here)

PabloBuchu · 2024-05-22T10:28:01Z

ready to merge. please fix failing ci checks

MrFlashAccount commented May 17, 2024

View reviewed changes

MrFlashAccount marked this pull request as ready for review May 17, 2024 09:06

MrFlashAccount requested review from PabloBuchu, indiv0 and somebody1234 as code owners May 17, 2024 09:06

MrFlashAccount added g-dashboard CI: No changelog needed Do not require a changelog entry for this PR. labels May 17, 2024

MrFlashAccount self-assigned this May 17, 2024

somebody1234 reviewed May 17, 2024

View reviewed changes

app/ide-desktop/lib/dashboard/src/modals/TermsOfServiceModal.tsx Outdated Show resolved Hide resolved

somebody1234 reviewed May 17, 2024

View reviewed changes

app/ide-desktop/lib/dashboard/src/providers/LocalStorageProvider.tsx Outdated Show resolved Hide resolved

somebody1234 reviewed May 17, 2024

View reviewed changes

app/ide-desktop/lib/dashboard/src/providers/LocalStorageProvider.tsx Outdated Show resolved Hide resolved

MrFlashAccount force-pushed the wip/sergeigarin/terms-of-use branch from efdf4ec to 075aba7 Compare May 20, 2024 14:12

somebody1234 approved these changes May 20, 2024

View reviewed changes

PabloBuchu approved these changes May 22, 2024

View reviewed changes

PabloBuchu added the CI: Ready to merge This PR is eligible for automatic merge label May 22, 2024

MrFlashAccount force-pushed the wip/sergeigarin/terms-of-use branch 2 times, most recently from ebd8559 to b4ec86c Compare May 22, 2024 15:59

MrFlashAccount force-pushed the wip/sergeigarin/terms-of-use branch 4 times, most recently from 8335254 to e64e699 Compare May 22, 2024 18:57

MrFlashAccount added 2 commits May 22, 2024 22:01

Terms of service dialog. Improve loading animations.

dabdf61

fix dialog

ba0620e

MrFlashAccount force-pushed the wip/sergeigarin/terms-of-use branch from e64e699 to ba0620e Compare May 22, 2024 19:04

MrFlashAccount added 4 commits May 22, 2024 22:19

fix dialog

dc943ff

fix prettier

bcb350c

fix animation

4879866

small improvements

26caea5

mergify bot merged commit 5ed5c71 into develop May 23, 2024
63 of 64 checks passed

mergify bot deleted the wip/sergeigarin/terms-of-use branch May 23, 2024 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Terms of Service Dialog #9975

New Terms of Service Dialog #9975

MrFlashAccount commented May 16, 2024 •

edited

MrFlashAccount May 17, 2024

somebody1234 May 17, 2024

somebody1234 May 17, 2024

MrFlashAccount May 17, 2024

MrFlashAccount May 17, 2024

MrFlashAccount May 17, 2024

somebody1234 commented May 17, 2024

PabloBuchu commented May 17, 2024

somebody1234 commented May 17, 2024

MrFlashAccount commented May 17, 2024

MrFlashAccount commented May 17, 2024

PabloBuchu commented May 20, 2024 •

edited

MrFlashAccount commented May 20, 2024

MrFlashAccount commented May 20, 2024

somebody1234 commented May 20, 2024

MrFlashAccount commented May 20, 2024 •

edited by unfurl-links bot

somebody1234 commented May 20, 2024

MrFlashAccount commented May 20, 2024

somebody1234 left a comment

PabloBuchu commented May 22, 2024

New Terms of Service Dialog #9975

New Terms of Service Dialog #9975

Conversation

MrFlashAccount commented May 16, 2024 • edited

Pull Request Description

Tl;dr

Note during the demo the changes on the website were not merged yet, so when I clicked "view terms and conditions" link, I was redirected to the / page. Once the changes on the website are merged, this link will lead to the "Terms and Conditions" page

This Change:

Test Plan:

Important Notes

Checklist

MrFlashAccount May 17, 2024

Choose a reason for hiding this comment

somebody1234 May 17, 2024

Choose a reason for hiding this comment

somebody1234 May 17, 2024

Choose a reason for hiding this comment

MrFlashAccount May 17, 2024

Choose a reason for hiding this comment

MrFlashAccount May 17, 2024

Choose a reason for hiding this comment

MrFlashAccount May 17, 2024

Choose a reason for hiding this comment

somebody1234 commented May 17, 2024

PabloBuchu commented May 17, 2024

somebody1234 commented May 17, 2024

MrFlashAccount commented May 17, 2024

MrFlashAccount commented May 17, 2024

PabloBuchu commented May 20, 2024 • edited

MrFlashAccount commented May 20, 2024

MrFlashAccount commented May 20, 2024

somebody1234 commented May 20, 2024

MrFlashAccount commented May 20, 2024 • edited by unfurl-links bot

somebody1234 commented May 20, 2024

MrFlashAccount commented May 20, 2024

somebody1234 left a comment

Choose a reason for hiding this comment

PabloBuchu commented May 22, 2024

MrFlashAccount commented May 16, 2024 •

edited

PabloBuchu commented May 20, 2024 •

edited

MrFlashAccount commented May 20, 2024 •

edited by unfurl-links bot