NBB316 - updated get_field in Attachment model#46
Open
bdaisey wants to merge 1 commit into
Open
Conversation
dcwatson
reviewed
Dec 11, 2020
| if prop.data_type == 'model' and prop.slug in self.data: | ||
| return prop.label, prop.model_queryset().get(pk=self.data.get(prop.slug, [])[0]) | ||
| pk = self.data.get(prop.slug, [])[0] | ||
| value = prop.model_queryset().get(pk=self.data.get(prop.slug, [])[0]) if pk else '' |
Member
There was a problem hiding this comment.
Couldn't this just be .get(pk=pk)?
Member
There was a problem hiding this comment.
Also, I don't know exactly how/where this is called, but returning None feels more appropriate for a default than ''. Or maybe even add a default=None kwarg that you could pass the empty string if you wanted.
ruttenb
reviewed
Dec 11, 2020
| """ | ||
| if prop.data_type == 'model' and prop.slug in self.data: | ||
| return prop.label, prop.model_queryset().get(pk=self.data.get(prop.slug, [])[0]) | ||
| pk = self.data.get(prop.slug, [])[0] |
Member
There was a problem hiding this comment.
This will raise an IndexError if the slug isn't in self.data since the default is an empty list
Member
There was a problem hiding this comment.
Brad should clearly be doing these reviews instead of me!
Author
There was a problem hiding this comment.
Ah ok, I was thinking you wanted to be looped in on all updates here, but will assign to Brad or Noah next time :)
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
@dcwatson heads up, let me know if you need any more info! Brad and I discussed this solution yesterday to give us desired behavior on NBB but let me know if there is a better way to approach this / unintended consequences.
Related to https://git.imsweb.com/bioshare/neurobiobank/-/issues/316
By default, attachments.models.Property has
required = True. This update will allow an Attachment to have a non-required Property, which will allow for the display of the Property's label even if the Property has no value set.The need for this was discovered on NBB because we want to allow attachments to have an associated property 'request-document-type' with 'required=false', so we need a way to allow the property key to exist without (necessarily) having a value set on all Attachments.
Desired behavior with a value set:
Desired behavior with NO value set:
(The current behavior is the server error described in the NBB issue, and the user will not see any popup on the front end when clicking the 'View Properties' icon on an attachment.)