Feat: add Terms of Use accept view and middleware #1217

MyPyDavid · 2024-12-18T14:15:39Z

Description

Related issue: #141, #161

It adds a migration for the ConsentFieldValue model: accounts.0022_add_created_updated_to_consent
So it requires a python manage.py migrate.
I have added some methods that handle the consent and session to the model.

configuration in the rdmo-app

When ACCOUNT_TERMS_OF_USE is enabled, some settings need to be configured in the rdmo-app as well.

In the settings from local.py:

if ACCOUNT_TERMS_OF_USE:
    MIDDLEWARE += [
        'rdmo.accounts.middleware.TermsAndConditionsRedirectMiddleware'
    ]

An optional feature is added that sets the date on which the ToU text is valid, so that it can be easily updated when the text might change. The optional setting is TERMS_VERSION_DATE and needs to be a valid date string that is compatible with formats from django get_format('DATE_INPUT_FORMATS'). The date of this setting checks if the user has updated their consent on or after that date, otherwise the consent is invalid and asked to accept again.

# for example
TERMS_VERSION_DATE = "2025-12-30"

Optionally, in the urls.py:

if settings.ACCOUNT_TERMS_OF_USE:
    from rdmo.accounts.views import terms_of_use, terms_of_use_accept
    urlpatterns += [
        path("account/terms-of-use/accept/", terms_of_use_accept, name="terms_of_use_accept"),
        path("account/terms-of-use/", terms_of_use, name="terms_of_use"),
    ]

Motivation and Context

How has this been tested?

Screenshots (if appropriate)

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid · 2025-02-03T18:20:05Z

can be reviewed when tests pass

MyPyDavid · 2025-02-06T07:45:03Z

rdmo/accounts/models.py

+
+    def is_consent_valid(self) -> bool:
+        # optionally enable terms to be outdated
+        terms_version_date = getattr(settings, 'TERMS_VERSION_DATE', None)


As a flow for invalidating the consent (eg. when the text needs an update or the terms changed),
I thought it would be helpful to be able to simply set this date as a setting instead of manually deleting all of the ConsentFieldValue objects.
Is the name of the TERMS_VERSION_DATE ok? Should it be added to the normal core/settings as TERMS_VERSION_DATE = None ?

I like it! And (see my comment above) should have a sensible default (None) in rdmo.core.settings.

Line 166 can be removed.

Actually I would call it ACCOUNT_TERMS_OF_USE_DATE to have some kind of ACCOUNT_TERMS_OF_USE_ namespacing in the settings.

Same for the other terms of use related settings.

I would stick to having a default value for the settings used in RDMO (otherwise we are inconsistent in rdmo), but something like this could be nice for plugins.

yes, I mean in addition to setting the default. The default is None and then when another value is set, it needs to be a valid to-a-date-parse-able string.

Hmm, I would not start checking settings. If RDMO is misconfigured exceptions will happen. If you mean to check validate the settings once when starting and outside of the usage of the settings in the code, I am fine with it. But I don't want to have validate(settings.FOO_BAR) everywhere.

yes, it's exactly that, for manage.py check:

SystemCheckError: System check identified some issues: ERRORS: ?: (core.E001) ACCOUNT_TERMS_OF_USE_DATE = 2025-02-36 is not a valid date string. HINT: day is out of range for month System check identified 1 issue (0 silenced). ❯

Awesome, go for it.

jochenklar

Great work! I think I will need test it some more with ORCID etc. when we have a release candidate, but I think this approach will work for all.

jochenklar · 2025-02-06T09:06:28Z

rdmo/accounts/middleware.py

+
+from .models import ConsentFieldValue
+
+# these exclude url settings are optional


My philosophy in RDMO was to add all settings to rdmo.core.settings in order to avoid those "checking" blocks. (getattr is already better than try ... execpt.) I suggest we stick to this.

jochenklar · 2025-02-06T09:10:41Z

rdmo/accounts/middleware.py

+        if (
+            settings.ACCOUNT_TERMS_OF_USE  # Terms enforcement enabled
+            and request.user.is_authenticated
+            and request.path != reverse("terms_of_use_accept")


Could be part of is_path_protected.

jochenklar · 2025-02-06T09:11:17Z

rdmo/accounts/models.py

+
+        obj.delete()  # Remove when consent is outdated
+        return False
+


Remove newline.

jochenklar · 2025-02-06T09:12:41Z

rdmo/accounts/models.py

+        return False
+
+
+    @classmethod


I actually likes it better when this was in utils. (I just forgot to pull and looked at an older version of the branch.)

Ok after looking at it some more, it makes sense together with the other methods.

I used the "Fat Models" approach for this and put everything relevant on the model..

jochenklar · 2025-02-06T09:14:02Z

rdmo/accounts/models.py

+
+    def is_consent_valid(self) -> bool:
+        # optionally enable terms to be outdated
+        terms_version_date = getattr(settings, 'TERMS_VERSION_DATE', None)


I like it! And (see my comment above) should have a sensible default (None) in rdmo.core.settings.

jochenklar · 2025-02-06T09:15:15Z

rdmo/accounts/models.py

+
+    def is_consent_valid(self) -> bool:
+        # optionally enable terms to be outdated
+        terms_version_date = getattr(settings, 'TERMS_VERSION_DATE', None)


Line 166 can be removed.

jochenklar · 2025-02-06T09:18:19Z

rdmo/accounts/models.py

+        # First, try standard ISO format (YYYY-MM-DD)
+        latest_terms_version_date = parse_date(terms_version_date)
+
+        # If ISO parsing fails, try localized formats


I wonder if we already use a more generic datetime parsing function in RDMO. If not we should put something like this in rdmo.core.utils. Maybe using datetime.fromisoformat(string) would suffice.

Ah I see that parse_date is already the django version of this.

jochenklar · 2025-02-06T09:22:44Z

rdmo/accounts/models.py

 from django.utils.translation import gettext_lazy as _

+from rdmo.core.models import Model as RDMOTimeStampedModel


I am not a fan of renaming in import, but I guess naming it just Model was not super clever back then.

jochenklar · 2025-02-06T09:25:58Z

rdmo/accounts/models.py

+
+    def is_consent_valid(self) -> bool:
+        # optionally enable terms to be outdated
+        terms_version_date = getattr(settings, 'TERMS_VERSION_DATE', None)


Actually I would call it ACCOUNT_TERMS_OF_USE_DATE to have some kind of ACCOUNT_TERMS_OF_USE_ namespacing in the settings.

jochenklar · 2025-02-06T09:26:33Z

rdmo/accounts/models.py

+
+    def is_consent_valid(self) -> bool:
+        # optionally enable terms to be outdated
+        terms_version_date = getattr(settings, 'TERMS_VERSION_DATE', None)


Same for the other terms of use related settings.

MyPyDavid · 2025-02-06T09:49:38Z

Great work! I think I will need test it some more with ORCID etc. when we have a release candidate, but I think this approach will work for all.

I think what is still missing is the integration with the social signup form, like the account sign up already has.

…d add checks Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid self-assigned this Dec 18, 2024

MyPyDavid added the type:feature-request label Dec 18, 2024

MyPyDavid added this to the RDMO 2.3.0 milestone Dec 18, 2024

MyPyDavid added 3 commits January 23, 2025 16:57

feat(accounts,ToU): add ToU update view, form and middleware #141 #161

1deac58

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

tests(accounts,ToU): add tests for ToU update and middleware

187c630

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

refactor(accounts, middleware): change ToU middleware to callable class

8c2a84b

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid force-pushed the feat-add-terms-of-use-middleware branch from bcbd456 to e25b960 Compare January 23, 2025 16:14

feat(accounts,ToU): add updated and created to Consent Model

3c2a3bc

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid force-pushed the feat-add-terms-of-use-middleware branch from e25b960 to 3c2a3bc Compare January 23, 2025 16:16

MyPyDavid added 2 commits February 3, 2025 11:00

feat(accounts,admin): update list display for ConsentFieldValue

0d694f7

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

feat(accounts,ToU): disable revocation and rename to accept

e52f940

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid added the status:work-in-progress label Feb 3, 2025

feat(accounts,ToU): add version date check, refactor and fix tests

22b63d4

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid marked this pull request as ready for review February 4, 2025 08:20

MyPyDavid removed the status:work-in-progress label Feb 4, 2025

MyPyDavid requested a review from jochenklar February 4, 2025 08:20

This was linked to issues Feb 4, 2025

No Terms of Use when using Shibboleth #141

Open

Social login does not show Terms of Use #161

Open

MyPyDavid changed the title ~~Feat: add Terms of Use update view and middleware~~ Feat: add Terms of Use accept view and middleware Feb 4, 2025

MyPyDavid commented Feb 6, 2025

View reviewed changes

jochenklar requested changes Feb 6, 2025

View reviewed changes

refactor(core,accounts): move ToU settings and date parser to core an…

fa8b7d1

…d add checks Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: add Terms of Use accept view and middleware #1217

Feat: add Terms of Use accept view and middleware #1217

MyPyDavid commented Dec 18, 2024 •

edited

Loading

MyPyDavid commented Feb 3, 2025

MyPyDavid Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 7, 2025

MyPyDavid Feb 7, 2025

jochenklar Feb 7, 2025

MyPyDavid Feb 7, 2025

jochenklar Feb 7, 2025

jochenklar left a comment

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

MyPyDavid Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

jochenklar Feb 6, 2025

MyPyDavid commented Feb 6, 2025


		from .models import ConsentFieldValue

		# these exclude url settings are optional

		from django.utils.translation import gettext_lazy as _

		from rdmo.core.models import Model as RDMOTimeStampedModel

Feat: add Terms of Use accept view and middleware #1217

Are you sure you want to change the base?

Feat: add Terms of Use accept view and middleware #1217

Conversation

MyPyDavid commented Dec 18, 2024 • edited Loading

Description

configuration in the rdmo-app

Motivation and Context

How has this been tested?

Screenshots (if appropriate)

MyPyDavid commented Feb 3, 2025

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochenklar 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyPyDavid commented Feb 6, 2025

MyPyDavid commented Dec 18, 2024 •

edited

Loading