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

Change Highlights's image field to use ImageChooserBlock #4349

Closed
2 tasks
mmmavis opened this issue Mar 13, 2020 · 10 comments
Closed
2 tasks

Change Highlights's image field to use ImageChooserBlock #4349

mmmavis opened this issue Mar 13, 2020 · 10 comments

Comments

@mmmavis
Copy link
Collaborator

mmmavis commented Mar 13, 2020

https://github.com/mozilla/foundation.mozilla.org/blob/master/network-api/networkapi/highlights/models.py#L55-L60

  • Change Highlights's image field to use ImageChooserBlock as images should have alt text associated with them.
  • And make sure we update the card template accordingly.
@cadecairos cadecairos added this to the Apr 6 milestone Mar 24, 2020
@cadecairos
Copy link

Let's make this a staged PR:

  1. Create a new field that uses the ImageChooserField
  2. Ship it, migrate images to the new field
  3. switch to the new field in the template
  4. remove the old field
  5. rename the new field, update template

@cadecairos cadecairos modified the milestones: Apr 6, Apr 20 Apr 7, 2020
@mmmavis mmmavis modified the milestones: Apr 20, May 4 Apr 21, 2020
@cadecairos cadecairos modified the milestones: May 4, May 18 May 5, 2020
@mmmavis
Copy link
Collaborator Author

mmmavis commented May 14, 2020

This is a multi-step ticket so I don't think I can get it done and land it by tomorrow (the last day of this sprint). I'm moving it back to Stretch Goals.

@phildexter phildexter modified the milestones: May 18, June 2 May 19, 2020
@mmmavis
Copy link
Collaborator Author

mmmavis commented May 26, 2020

@Pomax @cadecairos If I'm reading the doc correctly, ImageChooserBlock is only available as streamfield so I can't use it to replace https://github.com/mozilla/foundation.mozilla.org/blob/master/network-api/networkapi/highlights/models.py#L55-L60. Can you suggest what I should do instead? Should I add a new text field that works as the alt text for image?

@mmmavis
Copy link
Collaborator Author

mmmavis commented May 27, 2020

Blocked. Need a hand from Wagtail folks.


From discussion on Slack,

  • We can't use ImageChooserBlock as we are not working with a streamfield
  • ImageField does not hook into the image database so it doesn't let us pick from the CMS's managed images

Pomax has tried using ImageChooserPanel but that didn't work.

image

@mmmavis
Copy link
Collaborator Author

mmmavis commented May 27, 2020

/cc @phildexter

@KalobTaulien
Copy link
Contributor

Hi @mmmavis!

You got about half way there, you're so close!

To use the ImageChooserPanel the image needs to be a foreign key to the Wagtail image class (or a custom image class you're using).

The tricky part is converting the FileField to an Image object. Depending on how big your database is I think you could do this in a single migration file but you'll need to do some hand-rolling in a data migration rather than a standard migration. Though that adds complexity and may not be what you're looking for, in which the multiple steps @cadecairos outlined would be good, it's just missing the conversion from a FileField to a ForeignKey.

@register_snippet
class Highlight(SortableMixin):
    ...
    image = models.ForeignKey(
        'wagtailimages.Image', # Or path to custom Image class
        help_text='...',
        on_delete=models.CASCADE, # Or however you prefer to handle the on_delete method 
        blank=True,
        null=True,
    )
    ...
    # Once you define panels, you'll need tp specify a panel type for all fields
    # you want to be editable in the Wagtail admin 
    panels = [
        FieldPanel("..."),
        ImageChooserPanel("image"),
    ]

Perviously I've handled that kind of conversion the "hacky" way with two fields and renaming them (again, like what @cadecairos had mentioned). Using your code, but I would have looped through the all the Highlight objects, checked if the image is a jpg/png/gif or other supported image format, then created a new Image object and attach the new Image object to the current Highlight snippet we're iterating over. Lastly, top it off with a standard migration to delete the old file field, and one more migration to rename the new file field to the old field name so you don't break any templates. Some pseudo code below:

for highlight in Highlight.objects.all():
    if highlight.image is supported_image_type(highlight.image): 
        image = Image.objects.create(...)
        highlight.new_image_field = image 
        highlight.save() 

☝️ Then the standard renaming migrations can be applied.

I hope this unblocks you @mmmavis! Feel free to tag me for more input on the matter.

@cadecairos
Copy link

Thanks for hopping in to help @KalobTaulien!

Based on what you've said, I think this is then the steps to take here:

  1. Add a new temporary field image_temp to Highlight as a Foreign key to the wagtail image class, and generate a migration for it.
  2. Edit that new migration file, add a data migration operation after the field creation that iterates over each Highlight.image record, and create a wagtailimage.Image object for each one. Then save a reference to it to the highlight.image_temp FK field.
  3. Remove Highlight.image, and generate a new migration for the change.
  4. Rename Hightlight.image_temp to image, and generate a migration for the change.
  5. Add ImageChooserPanel('image') to the panels class attribute.

One potential gotcha: We store our files remotely on S3, do you know if wagtailimages.Image can use the DEFAULT_FILE_STORAGE class we have, which interfaces with S3?

@KalobTaulien
Copy link
Contributor

Yep, that all looks good!

I'm not 100% certain on the default file storage. I know S3 is supported in regards to file storage, but in terms of migrating a FileField to an Image FK I'm not too certain on.

Maybe try creating a Wagtail Image manually from the data you have from the FileField and see if that works. Otherwise, I would err on the side of caution and I suspect (but don't know for sure) that when you delete the current image field that it might try to delete your image and renditions from S3 as well, which would be a problem. If that's the case... On every Highlight iteration you could copy the image locally, create a new Wagtail Image with the new file and double check the new image gets uploaded to S3 — then when you make the switch you can have a guarantee the image will exist in S3.

Of course that leads us down the road of: what to do if images aren't deleted from S3 and you copy them for Wagtail Image objects? You could end up paying for 2x the storage and only using one set of images. That's something to look out for as well.

@phildexter phildexter modified the milestones: June 2, June 16, Icebox Jun 2, 2020
@phildexter
Copy link

Awaiting wagtail core support.

@jamilasnell jamilasnell removed this from the Icebox milestone Dec 21, 2021
@Pomax
Copy link
Contributor

Pomax commented Mar 30, 2022

given that this issue is over 2 years old, closing this as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants