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 de-duplication doesn't work in the multiple upload view when using a custom Image model with required fields #8892

Open
Tijani-Dia opened this issue Jul 22, 2022 · 5 comments · May be fixed by #8916

Comments

@Tijani-Dia
Copy link
Collaborator

Issue Summary

The image de-duplication feature doesn't work in the multiple upload view when using a custom Image model that has additional required fields.

Steps to Reproduce

  1. Start a new project with wagtail start myproject
  2. Add a custom Image model:
# app_label.models.py
from wagtail.images.models import Image, AbstractImage, AbstractRendition

class CustomImage(AbstractImage):
    alt = models.CharField(max_length=255, blank=False)

    admin_form_fields = Image.admin_form_fields + ("alt",)

class CustomRendition(AbstractRendition):
    image = models.ForeignKey(CustomImage, on_delete=models.CASCADE, related_name='renditions')

    class Meta:
        unique_together = (
            ('image', 'filter_spec', 'focal_point_key'),
        )
# settings.py
WAGTAILIMAGES_IMAGE_MODEL = 'app_label.CustomImage'
  1. Make migrations -> migrate.
  2. Go to the multiple upload view:
    1. Upload a new image
    2. Fill the alt field
    3. Click the Update button
  3. Repeat step 4.

I'd expect to see a warning message telling me that the image I just uploaded might be a duplicate.

  • I have confirmed that this issue can be reproduced as described on a fresh Wagtail project: yes

Technical details

  • Python version: 3.10.2
  • Django version: 4.0.6
  • Wagtail version: 3
  • Browser version: Chrome 103
@Tijani-Dia Tijani-Dia added type:Bug status:Unconfirmed Issue, usually a bug, that has not yet been validated as a confirmed problem. labels Jul 22, 2022
@Dolidodzik
Copy link
Contributor

I also managed to reproduce that issue - when you use default image model, and try to upload the same file you get warning message that looks like this:
image
But when you use custom one (that @Tijani-Dia provided above) you got no warnings.

@Dolidodzik
Copy link
Contributor

I think I'll fix it tomorrow (that is, unless there is someone else who wants to pick it up).

@Dolidodzik
Copy link
Contributor

I tried to fix that, but get_image_form() from images/forms.py works really weird. I figured that for some reason just changing fields to something like this:

from wagtail.images.models import Image
return modelform_factory(
        model,
        form=BaseForm,
        fields=Image.admin_form_fields, # it was model.admin_form_fields before
        widgets=widgets,
        formfield_callback=formfield_for_dbfield,
    )

makes it work like it should (tho fields from CustomModel are missing so it's not really a solution) and it completely changes how submit is processed later on. With model.admin_form_fields submit goes through CreateFromUploadedImageView.save_object() that hasn't any validation. Just changing fields to Image.admin_form_fields makes it go through AddView.get_edit_object_response_data() which has validation. Note that wagtail.images.models.Image.admin_form_fields are almost the same as CustomImage.admin_form_fields in this case:

class CustomImage(AbstractImage):
    alt = models.CharField(max_length=255, blank=False)
    admin_form_fields = Image.admin_form_fields + ("alt",)

I don't really know why it works this way, and don't want to make dirty fix, so I'll leave this to someone else. Hopefully my little investigation is going to help someone later.

@Tijani-Dia
Copy link
Collaborator Author

Tijani-Dia commented Jul 25, 2022

Thanks for having a look @Dolidodzik. I think the issue here is that the logic to find duplicates wasn't added in the CreateFromUploadedImageView back when we implemented the feature.

So, I think the fix would be:
1- Add the duplicate finding logic in the save_object method of the above class or maybe here
2- Update the client-side to include an additional step when we find a duplicate (https://github.com/wagtail/wagtail/blob/main/wagtail/images/static_src/wagtailimages/js/add-multiple.js#L192).

@Dolidodzik
Copy link
Contributor

Thanks for your response @Tijani-Dia! Yeah, I understand what you mean, I tried to solve it myself like this, but when I tried I realized that both CreateFromUploadedImageView.save_object() and CreateFromUploadView.post() are called after submitting form - meaning user would get warning message about possible duplicate AFTER filling in, and submitting this form:
image
And that's the dirty fix I mentioned before - in my opinion it isn't correct way of handling that, I think user should be warned about duplicate just after uploading it.

To really solve this issue, we would need to check how upload/validation is handled from this input:
image

And well, since no one solved this yet I guess I can give it a try once more.

Dolidodzik added a commit to Dolidodzik/wagtail that referenced this issue Jul 28, 2022
@lb- lb- linked a pull request Aug 2, 2022 that will close this issue
@lb- lb- added component:Images and removed status:Unconfirmed Issue, usually a bug, that has not yet been validated as a confirmed problem. labels Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants