Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Consents): Migrating old consent data to new consents #1984

Conversation

lauryndbrown
Copy link
Contributor

@lauryndbrown lauryndbrown commented Jun 14, 2021

Fixes Part of #1881

This migrates the old consent data as is. It is written so that if the consents have been changed the migration does not crash.

@lauryndbrown lauryndbrown changed the title feat(ConsentsMigrating old consent data to new consents feat(Consents): Migrating old consent data to new consents Jun 14, 2021
@lauryndbrown lauryndbrown marked this pull request as draft June 14, 2021 13:39
@lauryndbrown lauryndbrown self-assigned this Jun 14, 2021
@lauryndbrown lauryndbrown added this to In progress in Consents via automation Jun 14, 2021
@lauryndbrown lauryndbrown added this to the v3.2 milestone Jun 14, 2021
@lauryndbrown
Copy link
Contributor Author

lauryndbrown commented Jun 21, 2021

Testing that it is filled in:

>>> from consents.models import *
>>> from workshops.models import *
>>> person_1 = Person.objects.create(
            personal="Neville",
            family="Longbottom",
            email="person@email.com",
            username="longbottom_neville",
            may_contact=True,
            publish_profile=True,
            data_privacy_agreement=True,
            lesson_publication_consent="yes-github"
        )
>>> person_1
<Person: Neville Longbottom <person@email.com>>
>>> [ (c.term.slug, str(c.term_option)) for c in Consent.objects.filter(person=person_1).active().select_related("term", "term_option")]
[('privacy-policy', 'None'), ('may-contact', 'None'), ('public-profile', 'None'), ('may-publish-name', 'None')]
>>> exit()
(env) ➜  amy git:(1881/migrate-old-consent-data-to-new-consents) ✗ python manage.py migrate              Operations to perform:
  Apply all migrations: admin, auth, autoemails, consents, contenttypes, dashboard, django_comments, extrequests, fiscal, reversion, sessions, sites, social_django, workshops
Running migrations:
  Applying consents.0006_auto_20210614_1234... OK
(env) ➜  amy git:(1881/migrate-old-consent-data-to-new-consents) ✗ python manage.py shell  
>>> from workshops.models import *
>>> from consents.models import *
>>> person_1 = Person.objects.get(email="person@email.com")
>>> [ (c.term.slug, str(c.term_option)) for c in Consent.objects.filter(person=person_1).active().select_related("term", "term_option")]
[('privacy-policy', ' (agree)'), ('may-contact', ' (agree)'), ('public-profile', ' (agree)'), ('may-publish-name', 'Yes, and only use my GitHub Handle (agree)')]

@lauryndbrown
Copy link
Contributor Author

lauryndbrown commented Jun 21, 2021

Ensure they're going to the correct values:

>>> person_2 = Person.objects.create(
            personal="Person",
            family="Two",
            email="person2@email.com",
            username="person2",
            may_contact=False,
            publish_profile=True,
            data_privacy_agreement=False,
            lesson_publication_consent="unset"
        )
>>> [ (c.term.slug, str(c.term_option)) for c in Consent.objects.filter(person=person_2).active().select_related("term", "term_option")]
[('privacy-policy', 'None'), ('may-contact', 'None'), ('public-profile', 'None'), ('may-publish-name', 'None')]
>>> exit()
(env) ➜  amy git:(1881/migrate-old-consent-data-to-new-consents) ✗ python manage.py migrate consents 0006
Operations to perform:
  Target specific migration: 0006_auto_20210614_1234, from consents
Running migrations:
  Applying consents.0006_auto_20210614_1234... OK
(env) ➜  amy git:(1881/migrate-old-consent-data-to-new-consents) ✗ python manage.py shell  
>>> from workshops.models import *
>>> from consents.models import *
>>> person_2 = Person.objects.get(email="person2@email.com")
>>> [ (c.term.slug, str(c.term_option)) for c in Consent.objects.filter(person=person_2).active().select_related("term", "term_option")]
[('privacy-policy', 'None'), ('may-contact', ' (decline)'), ('public-profile', ' (agree)'), ('may-publish-name', 'None')]

@lauryndbrown
Copy link
Contributor Author

lauryndbrown commented Jun 21, 2021

Test that if a term is missing, the migration does not fail:

>>> from workshops.models import *
>>> from consents.models import *
>>> person_3 = Person.objects.create(
            personal="Person",
            family="Three",
            email="person3@email.com",
            username="person3",
            may_contact=True,
            publish_profile=True,
            data_privacy_agreement=True,
            lesson_publication_consent="yes-orcid"
        )
>>> [ (c.term.slug, str(c.term_option)) for c in Consent.objects.filter(person=person_3).active().select_related("term", "term_option")]
[('privacy-policy', 'None'), ('may-contact', 'None'), ('public-profile', 'None'), ('may-publish-name', 'None')]
>>> term = Term.objects.get(slug="may-publish-name")
>>> term.archive()
>>> exit()
(env) ➜  amy git:(1881/migrate-old-consent-data-to-new-consents) ✗ python manage.py migrate consents 0006
Operations to perform:
  Target specific migration: 0006_auto_20210614_1234, from consents
Running migrations:
  Applying consents.0006_auto_20210614_1234... OK
(env) ➜  amy git:(1881/migrate-old-consent-data-to-new-consents) ✗ python manage.py shell                
>>> from workshops.models import *
>>> from consents.models import *
>>> person_3 = Person.objects.get(email="person3@email.com")
>>> [ (c.term.slug, str(c.term_option)) for c in Consent.objects.filter(person=person_3).active().select_related("term", "term_option")]
[('privacy-policy', ' (agree)'), ('may-contact', ' (agree)'), ('public-profile', ' (agree)')]

@lauryndbrown
Copy link
Contributor Author

If a term option is missing the migration does not fail.

>>> from consents.models import *
>>> from consents.models import *
>>> person_4 = Person.objects.create(
            personal="Person",
            family="Four",
            email="person4@email.com",
            username="person4",
            may_contact=False,
            publish_profile=True,
            data_privacy_agreement=True,
            lesson_publication_consent="yes-orcid"
        )
>>> [ (c.term.slug, str(c.term_option)) for c in Consent.objects.filter(person=person_4).active().select_related("term", "term_option")]
[('privacy-policy', 'None'), ('may-contact', 'None'), ('public-profile', 'None')]
>>> term = Term.objects.get(slug="may-contact")
>>> term.options
<TermOptionQuerySet [<TermOption:  (agree)>, <TermOption:  (decline)>]>
>>> term.options[1]
<TermOption:  (decline)>
>>> term.options[1].archive()
>>> exit()
(env) ➜  amy git:(1881/migrate-old-consent-data-to-new-consents) ✗ python manage.py migrate consents 0006
Operations to perform:
  Target specific migration: 0006_auto_20210614_1234, from consents
Running migrations:
  Applying consents.0006_auto_20210614_1234... OK
(env) ➜  amy git:(1881/migrate-old-consent-data-to-new-consents) ✗ python manage.py shell 
>>> from workshops.models import *
>>> from consents.models import *
>>> person_4 = Person.objects.get(email="person4@email.com")
>>> [ (c.term.slug, str(c.term_option)) for c in Consent.objects.filter(person=person_4).active().select_related("term", "term_option")]
[('privacy-policy', ' (agree)'), ('may-contact', 'None'), ('public-profile', ' (agree)')]

@lauryndbrown lauryndbrown force-pushed the 1881/migrate-old-consent-data-to-new-consents branch from a43540f to e206ed9 Compare June 21, 2021 20:08
@lauryndbrown lauryndbrown marked this pull request as ready for review June 21, 2021 20:08
@lauryndbrown
Copy link
Contributor Author

lauryndbrown commented Jun 21, 2021

note for @pbanaszkiewicz my linter doesn't appear to run on the file during pre-commit. Is it still working?

Oh I see. Maybe it skips migrations locally but not when github actions is run

black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
isort....................................................................Passed
[1881/migrate-old-consent-data-to-new-consents e1847b95] Changed lint issues
 1 file changed, 9 insertions(+), 9 deletions(-)

@pbanaszkiewicz
Copy link
Contributor

note for @pbanaszkiewicz my linter doesn't appear to run on the file during pre-commit. Is it still working?

Oh I see. Maybe it skips migrations locally but not when github actions is run

black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
isort....................................................................Passed
[1881/migrate-old-consent-data-to-new-consents e1847b95] Changed lint issues
 1 file changed, 9 insertions(+), 9 deletions(-)

Thanks for this report. I created this issue: #1987

Copy link
Contributor

@pbanaszkiewicz pbanaszkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some minor comments to consider in future. Looks good to me.

options = TermOption.objects.filter(term__slug=term_slug, archived_at=None)
may_contact_agree = [
option for option in options if option.option_type == AGREE
][0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've commented on such construct before. This calls for next().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbanaszkiewicz I may have missed your comment. Where would next have been used here?

reconsent(Consent, old_consent, may_contact_disagree)


def copy_public_profile(apps, schema_editor):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functions are very similar to each other. Perhaps in future you could try to use partial to reduce amount of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thanks for the tip.

Consents automation moved this from In progress to In Review Jun 24, 2021
@lauryndbrown lauryndbrown merged commit 96e0837 into carpentries:develop Jun 28, 2021
Consents automation moved this from In Review to Done Jun 28, 2021
@lauryndbrown lauryndbrown deleted the 1881/migrate-old-consent-data-to-new-consents branch June 28, 2021 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants