Closed Pi2048 closed 6 years ago
Fixed in commit a07546e53b7604355c36367752ca3afbe22d84d7
Fixed by showing a form validation error for the missing header image. The user is asked to remove or replace this header before the page can be saved. This warning is shown directly in the invalid header image form field, so it is clear where something is wrong.
We could instead automatically remove the headers, but that makes it difficult to give feedback to the user. I prefer the feedback + manual removal over silent automatic.
Checking header files on every page request may indeed be a bit heavy on performance.
@Pieter1024 could you review the commit and close the issue if you have no additional suggestions/comments?
I believe I could not even remove the missing headers manually because the underlying image didn't exist anymore. It seems your code does not address that.
That is strange. I did test my changes (or else they don't work by definition :) ). Did test again and works for me (or I am not testing the same thing).
Might be that the error was in the validation check function, where it tried to get the image size: https://github.com/jonge-democraten/website/blob/a07546e53b7604355c36367752ca3afbe22d84d7/website/jdpages/models.py#L35 which is now skipped due to the new validation check, which is then probably ignored on deletion. Just a guess though. Not even sure if the validation is called on deletion.
Good! I didn't test, only reviewed. I'll test it and let you know.
@Pieter1024 Can you test this?
I tested this and it seems to work satisfactorily. Funny thing: because of the use of thumbnails, removing the header image file in the media library does not break the use of the header on the front end.
Anyway, when I remove the header image file from the media library, I can still open the page on which it was used in the admin, and remove the associated header. If I do not remove the associated header, I get a clear error message when attempting to save the page.
Currently, the site breaks when some of the images to which the headers setting of a page refers are missing (see ticket #3670 in Trac), in two ways:
Ideally, the site should do the following when saving a page:
I think fixing point two would lead to a quite severe performance impact, because it requires checking whether several files exist on disk (= disk I/O), then choosing one of them at random and accessing that file on disk (= more disk I/O). I think I can live with number 2 if number 1 is fixed.