Jkmarx/add config model #3177
Jkmarx/add config model #3177
Conversation
jkmarx
commented
Jan 17, 2019
•
jkmarx
commented
- Add model and api for getting and updating site profiles.
refinery/core/serializers.py
Outdated
|
|
||
| def partial_update(self, instance, validated_data): | ||
| """ | ||
| Update and return an existing `DataSet` instance, given the |
There was a problem hiding this comment.
| Update and return an existing `DataSet` instance, given the | |
| Update and return an existing `SiteProfile ` instance, given the |
refinery/core/models.py
Outdated
| about_markdown = models.TextField(blank=True) | ||
| intro_markdown = models.TextField(blank=True) | ||
| twitter_username = models.CharField(max_length=100, blank=True) | ||
| yt_videos = models.TextField(blank=True) |
There was a problem hiding this comment.
Maybe youtube_videos here instead?
Also rather than a TextField it looks like yt_videos could be better expressed through a relationship to another model?
Alternatives to that ^^^ would be:
- a"pickle-able" field (requires a third party library)
- a JSONField (requires django >= 1.9 and is PostgreSQL specific, or another third party library)
| instance.yt_videos = validated_data.get('yt_videos', | ||
| instance.yt_videos) | ||
| instance.save() | ||
| return instance |
There was a problem hiding this comment.
I think the shorthand for this would look like: instance.update(**validated_data)
Edit: Just kidding this wouldn't work... a QuerySet is required.
SiteProfile.objects.filter(pk=instance.pk).update(**validated_data) would probably work, but I haven't tried it out
refinery/core/views.py
Outdated
| return Response(serializer.data) | ||
|
|
||
| def patch(self, request): | ||
| if not request.user.is_superuser(): |
There was a problem hiding this comment.
I'm sure you're thinking of adding this, but a test to ensure admin-only access would be great
Codecov Report
@@ Coverage Diff @@
## develop #3177 +/- ##
===========================================
- Coverage 61.98% 61.97% -0.01%
===========================================
Files 438 438
Lines 27784 27676 -108
Branches 1285 1285
===========================================
- Hits 17222 17153 -69
+ Misses 10562 10523 -39
Continue to review full report at Codecov.
|
refinery/core/admin.py
Outdated
| @@ -154,6 +154,10 @@ class SiteStatisticsAdmin(AdminFieldPopulator): | |||
| change_list_template = "admin/core/sitestatistics/change_list.html" | |||
|
|
|||
|
|
|||
| class SiteVideoAdmin(AdminFieldPopulator): | |||
| list_display = ['site_profile'] | |||
There was a problem hiding this comment.
AdminFieldPopulator by default will expose all of a Model's fields in the Admin UI. Do we just want to show the site_profile or would it be useful to see the caption, source and source_id as well?
There was a problem hiding this comment.
@scottx611x It's displaying as I would expect below. Can you clarify?
| class SiteVideo(models.Model): | ||
| caption = models.TextField(blank=True) | ||
| site_profile = models.ForeignKey(SiteProfile) | ||
| source = models.CharField(max_length=100, blank=True) |
There was a problem hiding this comment.
One thing that could be useful for the source field is the use of the choices arg.
class SiteVideo(models.Model):
YOUTUBE = 'YT'
VIDEO_SOURCE_CHOICES = (
(YOUTUBE, 'Youtube'),
...
)
...
source = models.CharField(choices=VIDEO_SOURCE_CHOICES, max_length=100, blank=True)
Even if we don't envision videos coming from too many different sources, this would still solve the problem of potentially having many differing entries like: youtube, Youtube, YouTube etc.
| # delete unused videos | ||
| for video in db_site_videos: | ||
| if video.id not in new_video_list_ids: | ||
| video.delete() |
There was a problem hiding this comment.
If I'm reading this correctly an existing video will be deleted if its id isn't included in a given PATCH payload? So one would have to include all existing site_videos in a patch to preserve them even if they aren't being updated?
There was a problem hiding this comment.
@scottx611x Yes. It makes sense to me since I'm doing it though the siteProfile api which has the entire list as a field.
refinery/core/views.py
Outdated
| db_video = SiteVideo.objects.get( | ||
| id=new_video_data.get('id') | ||
| ) | ||
| except: |
There was a problem hiding this comment.
It would be good to explicitly catch the DoesNotExist exception here
