PT-453 | Add backend support for is_part_of_cultural_route boolean field to enrolments, queue enrolments and their reports#509
Conversation
|
KULTUS-API branch is deployed to platta: https://kultus-pr509.api.dev.hel.ninja 🚀🚀🚀 |
There was a problem hiding this comment.
Pull request overview
Adds end-to-end backend support for capturing and reporting whether an enrolment is part of a cultural route/path, spanning the enrolment domain model, GraphQL mutations, and reporting exports/serialization. Snapshot updates reflect test fixture output changes after introducing the new field.
Changes:
- Add
is_part_of_cultural_routeboolean to enrolment models (including queued enrolments) and propagate it through enrolment creation and report hydration. - Extend GraphQL enrolment mutations/inputs to accept and persist the new boolean.
- Add report serializer + CSV export support and update tests/snapshots accordingly.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| reports/views/csv_api.py | Adds a new CSV column for the cultural route flag. |
| reports/tests/test_serializers.py | Updates serializer input test to include the new field. |
| reports/serializers.py | Introduces a mapped boolean serializer field and wires it into EnrolmentReportSerializer. |
| reports/models.py | Adds the boolean field to EnrolmentReport and hydrates it from Enrolment. |
| reports/migrations/0002_enrolmentreport_is_part_of_cultural_route.py | Adds DB column for EnrolmentReport.is_part_of_cultural_route. |
| occurrences/schema.py | Adds GraphQL input field + passes it through enrolment mutations and update mutation logic. |
| occurrences/schema_services.py | Threads the new boolean through enrolment service functions. |
| occurrences/models.py | Adds the boolean field to enrolment base model and propagates it from queued enrolment to actual enrolment. |
| occurrences/migrations/0045_enrolment_is_part_of_cultural_route_and_more.py | Adds DB columns for Enrolment and EventQueueEnrolment. |
| occurrences/factories.py | Adds factory data for the new field. |
| occurrences/tests/test_api.py | Updates mutation test inputs to include the new GraphQL field. |
| organisations/tests/snapshots/test_api.ambr | Snapshot updates due to fixture value changes. |
| occurrences/tests/snapshots/test_notifications.ambr | Snapshot updates due to fixture value changes. |
| occurrences/tests/snapshots/test_api.ambr | Snapshot updates due to fixture value changes. |
| gdpr/tests/snapshots/test_gdpr_api.ambr | Snapshot updates due to fixture value changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
be0a837 to
69c8328
Compare
|
KULTUS-API branch is deployed to platta: https://kultus-pr509.api.dev.hel.ninja 🚀🚀🚀 |
69c8328 to
6a30058
Compare
|
KULTUS-API branch is deployed to platta: https://kultus-pr509.api.dev.hel.ninja 🚀🚀🚀 |
6a30058 to
e1a3e62
Compare
|
KULTUS-API branch is deployed to platta: https://kultus-pr509.api.dev.hel.ninja 🚀🚀🚀 |
e1a3e62 to
5c84b6e
Compare
|
KULTUS-API branch is deployed to platta: https://kultus-pr509.api.dev.hel.ninja 🚀🚀🚀 |
also: - with the request of the product owner don't restore any previous answer to "Is part of cultural route" question from local storage persisted Formik form's data, but instead always reset the question to its initial value i.e. no data given at all - update generated GraphQL types using backend running City-of-Helsinki/palvelutarjotin#509 i.e. the backend support for the `is_part_of_cultural_route` field in enrolments and queue enrolments - update playwright tests Refs: PT-453
also: - with the request of the product owner don't restore any previous answer to "Is part of cultural route" question from local storage persisted Formik form's data, but instead always reset the question to its initial value i.e. no data given at all - update generated GraphQL types using backend running City-of-Helsinki/palvelutarjotin#509 i.e. the backend support for the `is_part_of_cultural_route` field in enrolments and queue enrolments - update playwright tests Refs: PT-453
also: - with the request of the product owner don't restore any previous answer to "Is part of cultural route" question from local storage persisted Formik form's data, but instead always reset the question to its initial value i.e. no data given at all - update generated GraphQL types using backend running City-of-Helsinki/palvelutarjotin#509 i.e. the backend support for the `is_part_of_cultural_route` field in enrolments and queue enrolments - add tests - update playwright tests - load en/sv as well for tests instead of just fi as before - used for testing localization of "read more" url Refs: PT-453
also: - with the request of the product owner don't restore any previous answer to "Is part of cultural route" question from local storage persisted Formik form's data, but instead always reset the question to its initial value i.e. no data given at all - update generated GraphQL types using backend running City-of-Helsinki/palvelutarjotin#509 i.e. the backend support for the `is_part_of_cultural_route` field in enrolments and queue enrolments - add tests - update playwright tests Refs: PT-453
5c84b6e to
f34e42d
Compare
|
KULTUS-API branch is deployed to platta: https://kultus-pr509.api.dev.hel.ninja 🚀🚀🚀 |
f34e42d to
52e450c
Compare
|
KULTUS-API branch is deployed to platta: https://kultus-pr509.api.dev.hel.ninja 🚀🚀🚀 |
|
KULTUS-API branch is deployed to platta: https://kultus-pr509.api.dev.hel.ninja 🚀🚀🚀 |
also: - with the request of the product owner don't restore any previous answer to "Is part of cultural route" question from local storage persisted Formik form's data, but instead always reset the question to its initial value i.e. no data given at all - update generated GraphQL types using backend running City-of-Helsinki/palvelutarjotin#509 i.e. the backend support for the `is_part_of_cultural_route` field in enrolments and queue enrolments - add tests - update playwright tests Refs: PT-453
| class BaseMappedBooleanField(serializers.Field): | ||
| """ | ||
| Bidirectional serializer for a BooleanField with translatable labels for | ||
| False and True values. | ||
| """ | ||
|
|
||
| @property | ||
| def false_label(self) -> str: | ||
| raise NotImplementedError("Subclasses must define a false_label property.") | ||
|
|
||
| @property | ||
| def true_label(self) -> str: | ||
| raise NotImplementedError("Subclasses must define a true_label property.") | ||
|
|
||
| def to_representation(self, value) -> str: | ||
| return self.true_label if value else self.false_label | ||
|
|
||
| def to_internal_value(self, data) -> bool: | ||
| # This is likely worst case 6 ifs (fi/sv/en languages and two labels): | ||
| for lang_code, _lang_name in settings.LANGUAGES: | ||
| with translation.override(lang_code): | ||
| if data == self.false_label: | ||
| return False | ||
| if data == self.true_label: | ||
| return True | ||
| raise serializers.ValidationError(f"Unable to parse {data} as a boolean.") | ||
|
|
||
|
|
||
| class IsPartOfCulturalRouteField(BaseMappedBooleanField): | ||
| @property | ||
| def false_label(self) -> str: | ||
| # Django has fi/sv/en translations for "No" and "Unknown" out of the box: | ||
| return f"{_('No')} / {_('Unknown')}" | ||
|
|
||
| @property | ||
| def true_label(self) -> str: | ||
| # Django has fi/sv/en translations for "Yes" out of the box: | ||
| return _("Yes") | ||
|
|
||
|
|
There was a problem hiding this comment.
I would say this is a bit pointless, but I understand the benefit in terms of transparency and guiding the interpretation of reporting. This kind of belittles the situation where I really want to answer "NO". Since we are specifically tracking those who are cultural path users, this is just a display-level change and the expected response in the database data model is empty, this is fine.
There was a problem hiding this comment.
The "No / I don't know" is a design choice already ~2.5 years ago:
And the product owner approved it. So it is a known shortcoming/feature that "No" and "I don't know" are using the same value.
There was a problem hiding this comment.
Ah, and here the "No / Unknown" is a continuation of that, as product owner also approved that old enrolments and queue enrolments that have no data for the is_part_of_cultural_route field can be initialized to False (although they really mean "Unanswered", not "No" or "I don't know") and thus are indistinguishable from "No / I don't know" in themselves, but with EnrolmentBase.enrolment_time they are still distinguishable from "No / I don't know".
Add a required `is_part_of_cultural_route` field to enrolments and queue enrolments and their related reports. Updating enrolments through admin view intentionally does not require `is_part_of_cultural_route` to be always updated. Add `is_part_of_cultural_route` to Django admin's EnrolmentReportAdmin, to its list display and add a boolean filter for the value. Also update snapshots as they changed to some other set of random values, probably because of model change. Refs: PT-453
52e450c to
786295b
Compare
|
|
KULTUS-API branch is deployed to platta: https://kultus-pr509.api.dev.hel.ninja 🚀🚀🚀 |
also: - with the request of the product owner don't restore any previous answer to "Is part of cultural route" question from local storage persisted Formik form's data, but instead always reset the question to its initial value i.e. no data given at all - update generated GraphQL types using backend running City-of-Helsinki/palvelutarjotin#509 i.e. the backend support for the `is_part_of_cultural_route` field in enrolments and queue enrolments - add tests - update playwright tests Refs: PT-453



Description
Add is_part_of_cultural_route boolean field support
Add a required
is_part_of_cultural_routefield to enrolments andqueue enrolments and their related reports. Updating enrolments through
admin view intentionally does not require
is_part_of_cultural_routetobe always updated. Add
is_part_of_cultural_routeto Django admin'sEnrolmentReportAdmin, to its list display and add a boolean filter for
the value.
Also update snapshots as they changed to some other set of random
values, probably because of model change.
Related
Manual testing
Tested locally by running the PR's backend + end-user frontend PR's frontend, enrolling to an event. I could enrol to an event.
Additional notes
Initializing old enrolment data to
is_part_of_cultural_routewithFalsewas ok for the product owner. This means the value does not in itself distinguish between "Unanswered" (i.e. the old enrolments with False) and "No / I don't know" (i.e. the new enrolments with False).EnrolmentBasehasenrolment_timewithauto_now_add=Truethough, which could be used to check whether the enrolment is before this functionality was deployed to production (meaning itsis_part_of_cultural_route=FalsemeansUnanswered) or after (meaning itsis_part_of_cultural_route=FalsemeansNo / I don't know).NOTE:
serializer_class = EnrolmentReportSerializerfor serializing values. It is there, possibly used by the OpenAPI endpoint schema documentation? Don't know for sure, but at least the data output doesn't seem to use it at all. This is out of scope for this PR to refactor / fix underlying issues, this PR adds a new field to relevant reports and serializes the newly added field properly. Old fields stay as they were for backward compatibility.Screenshots
Event enrolling
Finnish
Swedish
English
Trying to enrol without choosing cultural route answer
Django admin
Event queue enrolment detail view
Enrolment detail view
Enrolment report list view
Enrolment report list view added filter
Enrolment report detail view