guardian / grid

The Guardian’s image management system
https://www.theguardian.com/info/developer-blog/2015/aug/12/open-sourcing-grid-image-service
Apache License 2.0
1.44k stars 121 forks source link

Automatically add/remove leases when a rights category is selected for an image/s based on application level config #4358

Open AndyKilmory opened 4 weeks ago

AndyKilmory commented 4 weeks ago

What does this change?

When a user selects a rights category for an image it may be necessary for leases to be added or removed from the image based on the rights rules associated with that rights category. Up to now it has been the responsibility of the user to understand those rules and apply the correct leases.

This PR introduces the ability to automatically add or remove leases based on the selected rights category in conjunction with application level config defining the leases that should be applied in relation to a rights category.

The application config takes the form; Screenshot 2024-10-29 at 16 30 49

When the user selects a rights category in the image edit screen then any leases defined in config should be applied to the image on save and any previous leases removed. If the selected rights category does not have an leases defined but the previoous rights category had a lease defined the old lease should be removed. The functionality also applies in the grid view to single or multiple selections of images.

The functionality also applies in the upload page to single and batch uploads.

How should a reviewer test this change?

The following use cases are supported; 1) In the Image Edit screen the User selects a Rights Category for which a lease or leases have been defined in config. On save of the rights category the appropriate lease/s should be added to the image replacing any previous leases. 2) In the Image Edit screen if the User selects a Rights Category that has no leases defined in config, but the prior Rights Category for the image had a lease, or leases, defined in config then the matching leases should be removed from the image when the new Rights Category is saved. 3) All other changes to Rights Categories in the Image Edit screen should work a previously if the new rights category and the previous rights category have no leases defined in config.

4) In the Image Grid view if one or more images are selected and the Rights category is updated the leases for all selected images should be updated in line with rules defined in use cases 1 to 3.

5) In the Image Upload page when a user selects a rights category for an uploaded image then on 'save' of the rights category any leases defined in config should be added to the image.

6) During batch upload of images if a rights category added to one image (including config defined leases) is propagated to the other images in the batch then leases chould be added to those images in line with config.

Who should look at this?

Tested? Documented?

paperboyo commented 1 week ago

Hi. Sorry for a delay reviewing this. Looks very feature-complete, thank you! Some questions/observations:

  1. I would love the UI to reuse established convention where editing one field will affect the other: orange border (like with Templates and recently with UsageRight-supplied instructions etc). I think it would be much less confusing if after making edits but before committing them, the UI would already indicate what’s gonna happen (we would need a new design: for leases which are about to get removed: orange bars?)
  2. It was very unexpected for me that images whose Usage Rights were assigned automatically on ingestion (as opposed to chosen manually), haven’t goth configged Leases added. Is this expected? I would argue it should work?
  3. Worth at least documenting that if Lease is dependent on a bit of info that isn’t there (eg. lack of dateTaken on image), there won’t be a Lease added?
  4. [minor] Wondered if other periods than year should be configurable for Duration? Was thinking a short Allow lease for some less safe categories… (hours, maybe days)

Intrigued, I have tested migration scenario and a configged Lease dependent on uploadTime, survived intact: 👏!

image
AndyKilmory commented 1 week ago

Hi Mateusz, Thanks for your comments - responses below... 1) We did discuss the need to use orange bordered elements as part of the update pattern - currently the implemented code performs the updates on the save of the rights category field - and so the leases we see are those post save and the pattern of orange borders relates to changes that will come into effect on save/apply. We're aware that this is a good pattern to deploy in this case but are keen to get the basic functionality in place for users and will look to extend to include the notifications of what will change on save in a future iteration. 2) Currently the rights categories that are impacted by this functionality are being ingested from capture or direct user upload and the addition of leases is being handled by the capture ingest process - so currently separated from the direct upload and edit. Again its functionality we have discussed and agreed would be the correct approach but would like it to be part of a phase 2 - incorporating into the auto-ingest pipelines 3) Will do - this is a specific requirement from product side of business in relation to images without TX dates. 4) Happy to incorporate this particular extension as it would take much to modify. Thanks Andy

paperboyo commented 1 week ago

Thanks, Andy! Great to hear about pts. 1–2 and absolutely fine to improve later :-). P.t 3: 👍. Of course, pt. 4 can also be improved later, but I just thought having 1y, 2m, 5d and 6h as options might be useful (or whatever syntax makes most sense).

Let us know if you plan to do any work as part of this PR or is this ready for a formal review as-is.

AndyKilmory commented 6 days ago

Hi Mateusz, I spoke with the team today and they agree with all your observations and we've written up tasks to implement items 1, 2 and 4 as part of phase 2 development and I'll add documentation into the readme regarding the functionality in general and also the specifics about what happens in case of missing dates. We're keen to get this in front of users asap (they're allocating usage rights and forgetting to add leases!) so could this please go forward as-is for formal review. Thanks