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

Image support webapp #67

Merged
merged 17 commits into from
Jul 16, 2021
Merged

Image support webapp #67

merged 17 commits into from
Jul 16, 2021

Conversation

Tijani-Dia
Copy link
Collaborator

This PR adds image support to the webapp.

I also renamed the model DummyChannel to Channel and refactored/lint the js code; that's why it comes with so many additions and deletions.

Copy link
Collaborator

@lucasmoeskops lucasmoeskops left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. Looks good and want to merge it, only a small question about the delete view which I'm not fully sure how it can work :-)

def get_image_title(self, image):
"""See base class."""

return image["image"]["name"].replace("-", " ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it maybe nice to also remove the extension from the name here? (I'm assuming it's there because of the get_image_mimetype method)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright!

class DummyChannelSerializer(serializers.ModelSerializer):
"""DummyChannel serializer"""
class ImageListSerializer(serializers.ListSerializer):
def get_value(self, dictionnary):
Copy link
Collaborator

Choose a reason for hiding this comment

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

dictionary instead of dictionnary if I'm nitpicking ;-)

def get_image_content(self, image):
"""See base class."""

return Image.objects.get(id=image["id"]).image
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this give a problem with the Delete view you also added below? It seems to only delete the image, but this view might raise an Image.DoesNotExist then? Or is it never called anymore?

Copy link
Collaborator Author

@Tijani-Dia Tijani-Dia Jul 16, 2021

Choose a reason for hiding this comment

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

@lucasmoeskops I tried hard but I can't get your point.
Is it the DeleteImageView that you are talking about?
If so, what problem might occur and when?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, in the DeleteImageView, the only thing that seems to be done is to delete the specified Image instance. But can the image not still be used in an existing message, so that this code will raise an error when trying to do .get(id=image["id"])?

I mean basically, the image seems to be deleted, but the message seems to still exist. Trying to reproduce it, but having some issues with csrf token validation at the moment interestingly :-(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I expect that the user will click "Save changes" in which case the image will no longer be in the message. If we send an update, the id of the message deleted won't be in the update so I think we should not have an error there.

I get sometimes a csrf token error but when I refresh or close and reopen the tab it works fine. Maybe there is something else too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right! It is no issue because a different message is sent. All right :-) Then I'll merge the request!

@lucasmoeskops lucasmoeskops merged commit a5ae72f into main Jul 16, 2021
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