Add banner to allow user to accept ToS changes #11795

rioug · 2023-11-10T05:06:54Z

⚠️ All Global work to be tracked in Clockify #11242 Discovery Endpoints - including testing, code review etc
⚠️ Aus devs to track in #11242 Task within Macdoch Regen Discovery

What? Why?

Closes Enterprise User actively consents to change of ToS #11720

We need an "automatic" way of notifying enterprise user that Terms of Service have been updated, and they need to accept the new ToS. To do so, we show a banner on every admin page which will disappear once they accept the new ToS

What should we test?

As super-admin, go to config and upload new ToS
As enterprise user login to platform
See notification, click it, it goes away
Logout, login again, check it hasn't come back

Turn on admin_style_v3 feature, and repeat above scenario

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

dacook

Great implementation, but I have a couple of suggestions to change marked with a ±.

Also some other queries.

spec/controllers/spree/admin/users_controller_spec.rb

app/controllers/spree/admin/base_controller.rb

dacook

I've just found a couple of other things to consider..

app/controllers/spree/admin/base_controller.rb

dacook

Cool, this is looking like a really tight solution.
I just have a query if we can reduce the amount of style rules by building on the existing ones? Or is that just personal preference..

Edit: also I think the testing notes will need updating.

dacook · 2023-11-20T22:52:00Z

app/webpacker/css/admin_v3/terms_of_service_banner.scss

+  bottom: 0;
+  left: 0;
+  width: 100%;
+  z-index: 1000;


With z-index, I try to only increase it as much as is needed. Often only a 1 or 2 is needed. I find it helps keep the numbers manageable, rather than trying to work out how many zeros to add on the end!
Oh, I just reviewed some of the z-indexes in the codebase, it seems they're very large already, maybe this was necessary?

It would be great to register this as a new variable in theglobals/variables.scss file, then we can at least start to see where z-index is used across the app.

I didn't put too much though into this, upon further review I think 102 should be enough for the admin pages. Added in globals/variables.scss

dacook · 2023-11-20T22:54:48Z

app/webpacker/css/admin_v3/terms_of_service_banner.scss

@@ -0,0 +1,31 @@
+.banner-container {


I'm wondering if we could just add the form-actions class to the element and get most of these styles for free, without having to duplicate them?

app/controllers/spree/admin/base_controller.rb

mkllnk

Great work. But this doesn't work for most users because they are not allowed to access the users controller.

app/controllers/spree/admin/base_controller.rb

app/views/spree/layouts/_admin_body.html.haml

app/webpacker/controllers/terms_of_service_banner_controller.js

db/migrate/20231103061213_add_terms_of_service_accepted_at_to_spree_users.rb

spec/controllers/spree/admin/users_controller_spec.rb

dacook

Great, this looks like it should be more maintainable 👍

I just have a query if the user ID parameter can be removed?

app/views/admin/_terms_of_service_banner.html.haml

mkllnk

Very cool. This is heaps better. I did find some more things to change though. 😄

app/reflexes/enterprise/user_reflex.rb

db/migrate/20231103061213_add_terms_of_service_accepted_at_to_spree_users.rb

mkllnk · 2023-11-24T00:13:43Z

app/helpers/admin/terms_of_service_helper.rb

+
+      current_spree_user.terms_of_service_accepted_at.present? &&
+        current_spree_user.terms_of_service_accepted_at > file_uploaded_at &&
+        current_spree_user.terms_of_service_accepted_at < DateTime.now


That's an interesting condition. So if I, for some weird reason, manage to tell the OFN database that I will accept future ToS until 3rd of March 2025 then this will be seen as invalid and it looks like I didn't accept the ToS. Until time passes and on the 3rd of March 2025, suddenly, I appear to have accepted the ToS. This is an impossible scenario at the moment but I wonder why you included it?

Also, if I just accepted the ToS right now and our computers are so fast that the rendering happens in the same fraction of a second then it looks like I didn't accept the ToS?

Luckily timestamps are very accurate, although, the database is less accurate and therefore, within the accuracy of the database now would always be bigger than the stored timestamp. But I think that would need to be within a microsecond. Those zeros are nanoseconds:

Thu, 12 Oct 2023 10:51:46.765599000 AEDT +11:00

David asked a similar question, see my answer here: #11795 (comment)

I admit it's a bit overkilled.

spec/requests/spree/admin/overview_spec.rb

dacook

Looking pretty good. I commented about how we could make the spec more efficient, but I think it would be ok as it is.

dacook · 2023-11-28T02:17:43Z

spec/system/admin/tos_banner_spec.rb

+  context "when updating Terms of Service" do
+    let(:test_file_path) { "public/Terms-of-service.pdf" }
+
+    it "shows the banner" do
+      # ToS has been accepted
+      admin_user.update!(terms_of_service_accepted_at: 2.days.ago)
+
+      # Upload new ToS
+      visit admin_terms_of_service_files_path
+      attach_file "Attachment", Rails.root.join(test_file_path)
+      click_button "Create Terms of service file"
+
+      # check it has been uploaded
+      expect(page).to have_link "Terms of Service"
+
+      expect(page).to have_content("Terms of Service have been updated")
+    end
+  end


I would suggest that we just need to test showing/hiding, and don't need this second spec about exact conditions of when it shows/hides, because that's covered in the request spec. But maybe that's just personal preference.

However, I would at least suggest that instead simulating the upload of new ToS file, you could just update it directly in the model, which would be a lot quicker.
Uploading of the new ToS is covered in spec/system/admin/configuration/terms_of_service_files_spec.rb.

I thought that, too, and just went to do this, but this test has only one page visit and a form submission. The test conditions are then carried out on the same page. Replacing this doesn't result in any speed-up of the test. So it's better to run real user interaction instead of manual database changes then.

mkllnk

Nice!

I agree with David about optimising the spec. I'm seeing a few other things outside this code that I would like to optimise at the same time, so I'll do that now. Stay tuned.

drummer83 · 2023-11-30T00:58:21Z

Hi @rioug,
Thanks for working on this! 💪
I tested this implementation and here are the results...

Positive test cases

There's a new attribute in the DB. ✔️
Use the deploy time as time of acceptance. ✔️
Display banner on every page in back office if ToS have changed. ✔️
Update the DB when new ToS are accepted by clicking the button in the banner. ✔️
Don't show the banner if the setting "Enterprises must accept Terms of Service" is disabled. ✔️
The banner works vor admin style v3 as well as legacy design. ✔️

Here is the query for reference:
SELECT id, email, terms_of_service_accepted_at FROM spree_users WHERE email = '[email protected]';

Negative test cases

If no ToS file has been uploaded, then all users are asked to accept the ToS in the banner, but the link redirects to /admin. Even clicking on 'accept' displays the banner again and again. Probably because TermsOfServiceFile.updated_at doesn't exist if there is no file. I think the case of missing ToS file should be treated in the same way as if the setting "Enterprises must accept Terms of Service" is disabled.
If an existing user signs up for an enterprise he/she has to accept ToS during registration, but still the banner is showing, because the time of acceptance is still the time of deployment.
If a new user signs up for an enterprise he/she has to accept ToS during registration, but still the banner is showing, because the time of acceptance is empty.

Both of the latter issues have the same root cause, which is that accepting the ToS during registration of an enterprise is not updating the terms_of_service_accepted_at entry in the DB.

Conclusion

I think we should fix those test cases before releasing.
I will move it back to In Dev.

Thanks again!

Plus specs

Plus spec, this is tested on the dashboard page. The banner will show if the user accepted_at is before the tos file updated at time.

The banner can in some case overlap element we are trying to interact with. Add a fake ToS file and make sure user have accepted the ToS, so that the banner is not shown

Timecop intefere with the fake terms of service, so we need to manually accept the terms of service to make the banner disappear

It shouldn't be possible for the update to fail, as we are not sending any parameter. Any other failure should be handled by rails already, ie missing csrf token.

…yment

Now that the banner isn't displayed if enterprise are not required to sign ToS, the fix is useless

Spree::Admin::UserController is for super admin user only. Moving to a reflex simplifies the code by getting rid of a new route and a new stimulus controller

It easier to understand when we can see the logic to display the banner in the view.

Reuse existing css when possible, and use variable for z-index so its easier to track usage of z-index

It's not needed, as the reflex get the curent user based on the user session

Now that we check if there is a ToS file before displaying the banner it's not needed anymore

Actually create a ToS file instead of using a fake one

These styles should only apply to the form-actions in the products form.

drummer83 · 2023-12-22T13:49:15Z

Issues for the admin style v3 parts are here:

[Redesigning notifications] Unify and improve width and positioning of notifications/banners wishlist#493
[BUU] Terms of Service (ToS) can't be accepted or read when additional notification is displayed #11971

rioug force-pushed the 11720-enterprise-user-consent-to-ToS-changes branch from 802c32c to d19016d Compare November 10, 2023 05:24

rioug marked this pull request as ready for review November 10, 2023 05:28

rioug force-pushed the 11720-enterprise-user-consent-to-ToS-changes branch from d19016d to 718aaec Compare November 10, 2023 06:34

rioug self-assigned this Nov 10, 2023

rioug force-pushed the 11720-enterprise-user-consent-to-ToS-changes branch 3 times, most recently from ca48a5f to efefb1c Compare November 13, 2023 01:02

dacook self-requested a review November 14, 2023 23:31

dacook requested changes Nov 15, 2023

View reviewed changes

dacook reviewed Nov 16, 2023

View reviewed changes

app/controllers/spree/admin/base_controller.rb Outdated Show resolved Hide resolved

app/controllers/spree/admin/base_controller.rb Outdated Show resolved Hide resolved

app/controllers/spree/admin/base_controller.rb Outdated Show resolved Hide resolved

rioug force-pushed the 11720-enterprise-user-consent-to-ToS-changes branch from 6add097 to b775e24 Compare November 16, 2023 03:57

sigmundpetersen changed the title ~~Add banner to allow user to accepte ToS changes~~ Add banner to allow user to accept ToS changes Nov 17, 2023

rioug requested a review from dacook November 20, 2023 00:02

dacook reviewed Nov 20, 2023

View reviewed changes

mkllnk requested changes Nov 22, 2023

View reviewed changes

rioug requested review from dacook and mkllnk November 23, 2023 06:09

dacook reviewed Nov 23, 2023

View reviewed changes

app/views/admin/_terms_of_service_banner.html.haml Outdated Show resolved Hide resolved

rioug requested a review from dacook November 23, 2023 22:49

mkllnk requested changes Nov 24, 2023

View reviewed changes

rioug requested a review from mkllnk November 27, 2023 01:04

dacook approved these changes Nov 28, 2023

View reviewed changes

mkllnk approved these changes Nov 29, 2023

View reviewed changes

drummer83 self-assigned this Nov 29, 2023

drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Nov 29, 2023

drummer83 removed their assignment Nov 30, 2023

drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Nov 30, 2023

mariocarabotta added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Nov 30, 2023

rioug and others added 25 commits December 22, 2023 13:20

Add /admin/users/:id/accept_terms_or_service end point

ee79fd8

Plus specs

Add Tos banner on all admin pages

8e31e35

Plus spec, this is tested on the dashboard page. The banner will show if the user accepted_at is before the tos file updated at time.

Set up data to hide ToS banner

94fb139

The banner can in some case overlap element we are trying to interact with. Add a fake ToS file and make sure user have accepted the ToS, so that the banner is not shown

Fix terms and condition spec

8e3e9ad

Timecop intefere with the fake terms of service, so we need to manually accept the terms of service to make the banner disappear

Per review, check terms_of_service_accepted_at has been updated

44f22dc

Per review, remove useless code pass

5c9abfe

It shouldn't be possible for the update to fail, as we are not sending any parameter. Any other failure should be handled by rails already, ie missing csrf token.

Add Terms of Service banner styling for admin V3

25d1511

Update translation to be inline with design mockup

6226067

Don't show the banner if enterprises are not required to accepte ToS

8371ead

Update migration to set terms_of_service_accepted_at to time of deplo…

d2b210c

…yment

Remove now useless fix

91862c1

Now that the banner isn't displayed if enterprise are not required to sign ToS, the fix is useless

Move acepting of ToS change to a reflex

d0ba881

Spree::Admin::UserController is for super admin user only. Moving to a reflex simplifies the code by getting rid of a new route and a new stimulus controller

Per review, move logic to display ToS banner to a helper

aaa8f3f

It easier to understand when we can see the logic to display the banner in the view.

Per review, stream line the css

56b75ad

Reuse existing css when possible, and use variable for z-index so its easier to track usage of z-index

Update only User who are enterprise owner

f643ba5

Remove user.id parameter

b3acdaf

It's not needed, as the reflex get the curent user based on the user session

Add system spec for ToS Banner

b28f40b

Move to simple UserReflex

91e5227

Per review, revert change

17fa6b6

Don't show the banner if no ToS file uploaded

9609ba4

Remove the fact ToS file

a1d3b20

Now that we check if there is a ToS file before displaying the banner it's not needed anymore

Fix tos system spec

d0edf57

Actually create a ToS file instead of using a fake one

Fix CSS selector

041d74b

These styles should only apply to the form-actions in the products form.

Move banner up to not cover existing button

772c641

Move banner to orignal postion at the bottom of page

1bb4a66

drummer83 force-pushed the 11720-enterprise-user-consent-to-ToS-changes branch from 4ae9da7 to 1bb4a66 Compare December 22, 2023 12:20

drummer83 merged commit 72c824a into openfoodfoundation:master Dec 22, 2023
50 checks passed

This was referenced Dec 22, 2023

[Redesigning notifications] Unify and improve width and positioning of notifications/banners openfoodfoundation/wishlist#493

Open

[BUU] Terms of Service (ToS) can't be accepted or read when additional notification is displayed #11971

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add banner to allow user to accept ToS changes #11795

Add banner to allow user to accept ToS changes #11795

rioug commented Nov 10, 2023 •

edited

Loading

dacook left a comment

dacook left a comment

dacook left a comment •

edited

Loading

dacook Nov 20, 2023

rioug Nov 23, 2023

dacook Nov 20, 2023

mkllnk left a comment

dacook left a comment

mkllnk left a comment

mkllnk Nov 24, 2023

mkllnk Nov 24, 2023

rioug Nov 26, 2023

dacook left a comment

dacook Nov 28, 2023

dacook Nov 28, 2023

mkllnk Nov 29, 2023

mkllnk left a comment

drummer83 commented Nov 30, 2023 •

edited by sigmundpetersen

Loading

drummer83 commented Dec 22, 2023

Add banner to allow user to accept ToS changes #11795

Add banner to allow user to accept ToS changes #11795

Conversation

rioug commented Nov 10, 2023 • edited Loading

What? Why?

What should we test?

Release notes

dacook left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

drummer83 commented Nov 30, 2023 • edited by sigmundpetersen Loading

Positive test cases

Negative test cases

Conclusion

drummer83 commented Dec 22, 2023

rioug commented Nov 10, 2023 •

edited

Loading

dacook left a comment •

edited

Loading

drummer83 commented Nov 30, 2023 •

edited by sigmundpetersen

Loading