Copy affiliations to targeted journalists and institutions #865
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chigby
force-pushed
the
copy-affiliations-to-targeted-relations
branch
2 times, most recently
from
619da58 to
98b891f
Compare
chigby
force-pushed
the
copy-affiliations-to-targeted-relations
branch
from
98b891f to
39fe578
Compare
harrislapiroff
approved these changes
Apr 7, 2020
| inst, _ = Institution.objects.get_or_create(title=incident.affiliation) | ||
|
|
||
| targeted_journos = incident.targeted_journalists.all() | ||
| if targeted_journos: |
There was a problem hiding this comment.
Had to check my memory that empty querysets are falsey. They are 👍
We are phasing out the one-affiliation-per-incident affiliation model
in favor of an Institution model, which can either be applied on a
journalist by journalist basis, or to the incident as a whole, but as
many as desired.
For this, we must move data from the legacy affiliation field to the
new Institution field. We do this using a data migration, with two
pre- and post- migrations to work around this bug in
djang-modelcluster:
wagtail/django-modelcluster#96
For the migration, we want these steps:
1. For each incident, examine the `affiliation` field
2. If it null or set to the default value of "Independent," discard
it. The "independent" value is a placeholder for no definite
affiliation and will not be used in the new scheme.
3. Create, if it doesn't already exist, an Institution with the title
of the affiliation.
4. If this incident has any targeted journalists who do not already
have an associated institution, associate the new institution with
them.
5. If this incident does *not* have any targeted journalists, add the
new institution as a target of this incident.
This will fully bring any affiliation data into the institution model
and apply it to the incidents that this data described.
Since this information is subsumed by the institution data, I'm feeling like it should be removed from display as well.
harrislapiroff
force-pushed
the
copy-affiliations-to-targeted-relations
branch
from
39fe578 to
36bf7a7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is to correct an oversight in from #776 where the field
IncidentPage.affiliationshould have been used to initialize theInstitutionmodel and the relationships betweenInstitution,IncidentPage, andTargetedJournalist. In this pull request, we have:affiliationdata into theInstitutionmodel and its relations. See commit for further detail and rationale.affiliationdisplay on the incident page sidebar.Fixes #866
Testing and Acceptance
The crucial element here is the data manipulation code. I believe it's good, but we ought to make extra sure that: 1. we know what we want it to do, and 2. it does that correctly.