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
Implement Image Gallery Functionality & Endpoints #3312
Conversation
330227e
to
9625172
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3312 +/- ##
==========================================
+ Coverage 88.16% 88.19% +0.02%
==========================================
Files 664 665 +1
Lines 20946 21049 +103
==========================================
+ Hits 18467 18564 +97
- Misses 2479 2485 +6
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nice. this looks very good. Just some questions that need addressing and some discussions to be had :)
Also, the naming could be better IMO 😬. (ImageGallery is very vague, and something like CoverImageGallery
for the endpoint makes more sense)
06fb210
to
ddb60de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some permission stuff missing, but then this should be good to go!
Would also be nice if you could add some tests for the endpoints you created here :)
(check different perms, and that the response is correct in each case)
ddb60de
to
8b89a65
Compare
8b89a65
to
816d4ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome with the tests!
Just a few comments to clean things up and this should be good to go :)
816d4ae
to
52ea193
Compare
52ea193
to
d353eb5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
if self.instance.token != data["token"]: | ||
raise PermissionDenied() | ||
if "save_for_use" not in data: | ||
raise ValidationError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to include an error message. but not critical
if serializer.is_valid(raise_exception=True): | ||
serializer.save() | ||
return Response(data=serializer.data, status=status.HTTP_200_OK) | ||
else: | ||
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you have raise_exception
in is_valid()
, you won't execute the code path in the else block no?
Not an issue though :)
return reverse("api:v1:files-imagegallery", kwargs={"key": key}) | ||
|
||
|
||
class SetSaveForUserTest(BaseAPITestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a super clean and thorough test! Love it!! 💯
d353eb5
to
8379438
Compare
Implement imagegallery endpoints