Feature/migrate images #265
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #265 +/- ##
========================================
Coverage 97.68% 97.68%
========================================
Files 99 99
Lines 3150 3150
========================================
Hits 3077 3077
Misses 73 73 |
| coll = Collection.objects.filter(name=name).first() | ||
| # otherwise, create it | ||
| if not coll: | ||
| coll = Collection(name=name) |
There was a problem hiding this comment.
could this be a get_or_create()?
There was a problem hiding this comment.
No, I tried that — doesn't work because it has to be added to the parent to set the hierarchy properly
| # delete all images prior to run (clear out past migration attempts) | ||
| Image.objects.all().delete() |
There was a problem hiding this comment.
should we also do this for collections?
There was a problem hiding this comment.
maybe? I have some extraneous collections right now that I think the current version of the script wouldn't create but older versions did; but once the script is stable I don't think it matters or gets us much. (I'm open to it, LMK)
There was a problem hiding this comment.
i think for consistency let's do it. gives me peace of mind to know that everything is being wiped, haha
| # NOTE: leaving this here in case we want to handle | ||
| # documents the same way | ||
| if extension in ['.pdf', '.svg', '.docx']: | ||
| continue |
There was a problem hiding this comment.
ah, this is handy - thx for the note
There was a problem hiding this comment.
yeah — thought about filtering them out when I filter out the duplicates, but wasn't sure yet how we'll handle the files
| # check files with the same name in multiple locations | ||
| for key, val in unique_filenames.items(): | ||
| if len(val) > 1: | ||
| samefile = filecmp.cmp(val[0], val[1], shallow=False) |
There was a problem hiding this comment.
this only compares the first two, right?
| # if not all the same, identify the largest | ||
| # (all are variant/cropped versions of the same image) | ||
| else: | ||
| largest_file = None | ||
| largest_size = 0 | ||
| for filepath in val: | ||
| size = os.stat(filepath).st_size | ||
| if size > largest_size: | ||
| largest_size = size | ||
| largest_file = filepath | ||
|
|
||
| extra_copies = [f for f in val if f != largest_file] |
There was a problem hiding this comment.
did we ever end up with situations where there are totally unrelated images with the same name (e.g. Untitled-1 or Upload or something generic)?
There was a problem hiding this comment.
ah, nvm - I see your report on the issue that confirms this isn't a problem.
There was a problem hiding this comment.
right — here's the list #248 (comment)
| L,M @1x W: 2560px H: 680px, @2x W: 5120px H: 1360px | ||
| S @1x W: 736px H: 320px, @2x W: 1472px H: 640px |
There was a problem hiding this comment.
fwiw the L/M 2x seems huge; I don't think we ever want to be serving stuff larger than 2500px. but fine for now; we can always revisit.
There was a problem hiding this comment.
These are Xinyi's guidelines, maybe they need to be revisited. I agree it's huge (and I think what we've always been serving, even for mobile — although I guess before we just served out whatever we had; will wagtail make images larger?). This is why I asked you about the background image and different sizes.
Do you want to make a new issue for addressing this and add it to 3.1 ? It would be a good improvement to the site.
There was a problem hiding this comment.
created #266 and added to 3.1
see #248