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

Allow null values for contact id on resource downloads #1180

Merged
merged 4 commits into from
Jun 29, 2021

Conversation

mwvolo
Copy link
Member

@mwvolo mwvolo commented Jun 29, 2021

No description provided.

@mwvolo mwvolo requested a review from RoyEJohnson June 29, 2021 15:42
Copy link
Contributor

@code-review-doctor code-review-doctor bot left a comment

Choose a reason for hiding this comment

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

Worth considering

Co-authored-by: django-doctor[bot] <72320148+django-doctor[bot]@users.noreply.github.com>
Copy link
Contributor

@code-review-doctor code-review-doctor bot left a comment

Choose a reason for hiding this comment

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

Looks good. Worth considering though

Copy link
Contributor

@code-review-doctor code-review-doctor bot left a comment

Choose a reason for hiding this comment

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

Worth considering

@@ -331,7 +331,7 @@ class ResourceDownload(models.Model):
last_access = models.DateTimeField()
number_of_times_accessed = models.IntegerField()
resource_name = models.CharField(max_length=255, null=True, blank=False)
contact_id = models.CharField(max_length=100, blank=True, default='')
contact_id = models.CharField(max_length=100, null=True, blank=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
contact_id = models.CharField(max_length=100, null=True, blank=True)
contact_id = models.CharField(max_length=100, default='', blank=True)

null=True on a string field causes inconsistent data types because the value can be either str or None. This adds complexity and maybe bugs, but can be solved by replacing null=True with default="".

Copy link
Contributor

@RoyEJohnson RoyEJohnson left a comment

Choose a reason for hiding this comment

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

So there's a case for null=True, after all.

@mwvolo
Copy link
Member Author

mwvolo commented Jun 29, 2021

I would have thought the default='' should have just set it to a blank string. But it looks like that doesn't happen for some reason - maybe the rest API framework doing something to it.

@mwvolo mwvolo merged commit 47310e5 into master Jun 29, 2021
@mwvolo mwvolo deleted the allow-null-contact-id branch June 29, 2021 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants