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

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

AndyKilmory
Copy link
Collaborator

@AndyKilmory AndyKilmory commented Oct 29, 2024

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?

  • locally by committer
  • BBC Dev
  • locally by Guardian reviewer
  • on the Guardian's TEST environment
  • relevant documentation added or amended (if needed)

@AndyKilmory AndyKilmory force-pushed the t1742-leases-for-rights-category branch from 3afaa2c to 28d7488 Compare October 30, 2024 11:17
@AndyKilmory AndyKilmory force-pushed the t1742-leases-for-rights-category branch from 28d7488 to 958dea5 Compare October 30, 2024 11:26
Copy link
Contributor

@Conalb97 Conalb97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - only a couple of small comments/questions!

}
break;
default:
startDate = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startDate is already intialised as undefined - does it need to be redeclared as undefined in the default case as well?

Also maybe not relevant here but object literals look like a good alternative to switch cases in javascript: https://dev.to/mhmdjaw/an-alternative-to-the-javascript-switch-statement-1kah

kahuna/public/js/edits/image-editor.js Outdated Show resolved Hide resolved
kahuna/public/js/leases/leases.js Show resolved Hide resolved
@AndyKilmory AndyKilmory marked this pull request as ready for review November 6, 2024 14:24
@AndyKilmory AndyKilmory requested review from a team as code owners November 6, 2024 14:24
Copy link
Contributor

@Conalb97 Conalb97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@paperboyo paperboyo added the bbc label Nov 7, 2024
@paperboyo
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

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

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

Successfully merging this pull request may close these issues.

3 participants