Fixes for whether existing Identifier objects are preserved or not #11
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
struan
reviewed
Jan 6, 2017
| @@ -573,14 +577,22 @@ def create_identifier(self, popit_collection, popit_id, django_object): | |||
| identifier=popit_id, | |||
| ) | |||
|
|
|||
| def preserve_related(self, django_main_model, related_object): | |||
There was a problem hiding this comment.
preserve_related sounds like an action, I think something more like should_preserve_related makes it a little clearer what this is doing.
| ) | ||
| existing_person.identifiers.create( | ||
| scheme='preserve-me', | ||
| identifier="data-that-should-be-kept" |
There was a problem hiding this comment.
This doesn't seem like a helpful name here given the test output.
| person.identifiers.order_by('scheme').values_list('scheme', 'identifier'), | ||
| [(u'popolo:person', u'a1b2')] | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
I wonder if there should be a near identical test with identifiers set to [] to show that they do the same thing?
We're refactoring this in order to make it easier subsequently to allow users of this class to specify additional Identifier schemes to preserve.
If 'identifiers' was missing from a person, the old Identifier objects associated with a Person would all be preserved, but if instead identifiers was [] it would remove all Identifier objects associated with that Person except for the one that preserved the Django ID for them. This change makes the behaviour consistent between those two very similar cases - unknown identifiers are removed when syncing from the source. (This is what happens for contact details, etc. as well.)
mhl
force-pushed
the
extend-identifier-preservation
branch
from
ad332c1
to
b9f3c1d
Compare
|
Thanks, @struan, I've made the changes you suggested. |
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 pull request addresses two problems:
it was empty had confusingly different behaviour. This makes the 'missing'
case consistent with the empty array and existing handling of other related
objects.
identifiers that should be preserved; this pull request adds that facility.