search

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 119 forks source link

1194 interim filters #4212

Closed AndyKilmory closed 2 months ago

AndyKilmory commented 6 months ago

What does this change?

The aim of this new functionality is to make it easier for Users to find and select images that have usage rights appropriate to their needs and to provide visual indicators regarding the usage rights of an image and its suitablility for a particular use.

Addition of new filter control Change amens the kahuna UI to provide a two tier top bar and re-configures the arrangement of controls across the two bars to provide more screen space for menu items. It also introduces a 'filter' dropdown option. This is a configurable control that allows for the easy construction of frequently used chip searches providing them with meaningful names. It also introduces a drop down control for the sort order options (no new ordering behaviour is added but this will be extended in future).

The metadata info panel has also been extended to include additional usage rights information and icons in a configurable fashion. This too is controlled through configuration.

The new configuration options for kahuna are;

    |usePermissionsFilter=true
    |permissionsDefault="allPermissions"
    |permissionsOptions=`[{"id":"allPermissions","label":"All permissions","mapping":"","payable":"none"},{"id":"usableForNews","label":"Usable for News","mapping":"category:agency","payable":"false"}]`

Note the permissionsOptions are configured as an array of JSON objects of the form;

{ "id": "exampleId", <-- internally used identifier "label": "Example Label", <-- the label that will be displayed in the drop down list "mapping": "category:agency,source:example", <-- a csv list of the required search chips "payable": "none|true|false" <-- whether 'include payable images' should be forced to true, false or left unchanged (none) }

Filter option mappings can include more than a single filter 'chip' - when multiple chips are required they should be a comma separated list.

All the permissions options can be changed as required. Switching usePermissionsFilter and usageRightsSummary to 'false' will leave the UI unchanged from its current behaviour.

If all the configuration settings are excluded (ie no change to current config) the UI will continue to operate in an unaltered fashion (ie once the code is merged there is no requirement to make any modifications to the config unless you want to utilise the functionality).

The new UI appears as;

Screenshot 2023-12-14 at 16 48 30

Additional Usage Right Summary Info This change introduces a new control that appears in the 'Rights and restrictions' section of the Info panel. Thie control replicates the existing wording bu then in certain circumstances adds an icon and additional text into the section to provide more guidance to the user regaridn the rights of an image. The icon, text and display rules for this addditional information are set in an organisation specific file in the kahuna/.../gr-usagerights-summay folder - e.g. gr-usagerights-bbc.tsx.

This feature is turned on and off with a kahuna config flag;

    |usageRightsSummary=true

If this config is excluded the UI will continue to work unchanged.

The usage rights summary outputs as configured by the BBC logic embedded within this PR are;

Usable for all images (green); Screenshot 2023-12-18 at 12 55 02

Usable for news (amber); Screenshot 2023-12-18 at 12 55 31

Payable Images (red); Screenshot 2023-12-18 at 12 56 55

Any icons text or logic can be configured in an appropriate tsx file.

How should a reviewer test this change?

Tester should ensure that UI behaves unchanged with config options switched to false.

User should ensure that with options switched to true all new controls appear and function as required providing correct filter URLs to the search endpoint.

Tested? Documented?

twrichards commented 6 months ago

hey @AndyKilmory the various loader re-architecture work is merged (#4201 mainly) so please rebase this PR onto latest main, and then @paperboyo can give it a spin in our TEST environment

AndyKilmory commented 6 months ago

Thanks Tom – will do.

Regards Andy

Andy Downing (he/him) Senior Software Engineer (Images) Business Systems and Applications Broadcast & End-User Technology (BEUT) BBC Technology Group

From: Tom Richards @.> Reply to: guardian/grid @.> Date: Tuesday, 19 December 2023 at 18:10 To: guardian/grid @.> Cc: Andrew Downing @.>, Mention @.***> Subject: Re: [guardian/grid] 1194 interim filters (PR #4212)

hey @AndyKilmoryhttps://github.com/AndyKilmory the various loader re-architecture work is merged (#4201https://github.com/guardian/grid/pull/4201 mainly) so please rebase this PR onto latest main, and then @paperboyohttps://github.com/paperboyo can give it a spin in our TEST environment

— Reply to this email directly, view it on GitHubhttps://github.com/guardian/grid/pull/4212#issuecomment-1863257643, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A6UE4XQBNM4HMPX6PE7PPJLYKHKBTAVCNFSM6AAAAABAVE6XTCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRTGI2TONRUGM. You are receiving this because you were mentioned.Message ID: @.***>

http://www.bbc.co.uk This e-mail (and any attachments) is confidential and may contain personal views which are not the views of the BBC unless specifically stated. If you have received it in error, please delete it from your system. Do not use, copy or disclose the information in any way nor act in reliance on it and notify the sender immediately. Please note that the BBC monitors e-mails sent or received. Further communication will signify your consent to this.

AndyKilmory commented 6 months ago

Hi @twrichards, I've rebased to the latest guardian main branch and I think the PR is now ready for testing and review. Thanks Andy

AndyKilmory commented 6 months ago

Hi Tom, Thanks for the guidance – I’ll take a look at reconfiguring the config structures for the filter settings.

Regards Andy

Andy Downing (he/him) Senior Software Engineer (Images) Business Systems and Applications Broadcast & End-User Technology (BEUT) BBC Technology Group

From: Tom Richards @.> Reply to: guardian/grid @.> Date: Friday, 22 December 2023 at 13:31 To: guardian/grid @.> Cc: Andrew Downing @.>, Mention @.***> Subject: Re: [guardian/grid] 1194 interim filters (PR #4212)

@twrichards commented on this pull request.

In kahuna/app/lib/KahunaConfig.scalahttps://github.com/guardian/grid/pull/4212#discussion_r1435058953:

@@ -39,6 +39,15 @@ class KahunaConfig(resources: GridConfigResources) extends CommonConfig(resource

val restrictDownload: Option[Boolean] = booleanOpt("restrictDownload")

val useReaper: Option[Boolean] = booleanOpt("useReaper")

I haven't looked over this PR at all, but @paperboyohttps://github.com/paperboyo and I ended up discussing the config aspect when he was playing with this PR in our TEST environment... I was wondering why we're not using HOCON arrays here (https://docs.tibco.com/pub/str/latest/doc/html/hocon/hocon-syntax-reference.html) but instead splitting on , & # AFAICT (which is harder to read/configure but also means command and hash cannot be used in the search term either)

— Reply to this email directly, view it on GitHubhttps://github.com/guardian/grid/pull/4212#pullrequestreview-1794486892, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A6UE4XS7ZTSJKWXZ7TC7I4LYKWDTBAVCNFSM6AAAAABAVE6XTCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJUGQ4DMOBZGI. You are receiving this because you were mentioned.Message ID: @.***>

AndyKilmory commented 6 months ago

Revised structure for filter definition config now added

paperboyo commented 6 months ago

Hello (and happy NY!). Thank you, we have been testing this on TEST. It’s quite a lot of functionality, at least three distinct parts from our point of view: sort UI, Show paid, Permissions. We are not sure what we will adopt here at the Guardian yet. We would definitely want Sort UI as it doesn’t feel org-specific in any way. Show paid is already different between orgs, as ours say Show Free and is controlled by our Permissions (this part breaks with this PR currently), so this part is probably least likely to be shared and would be behind its own config. The coolest part by far is Permissions. Thank you for the extensive config! We think it warrants further discussions IRL, let’s start them at the next Grid Hour.

We think a good practical step would be to ask you to separate Sort UI from this PR. We can then discuss it in isolation. It will need to be added to existing top bar, replacing current sorting UI. It should not be behind any config. I think we should have Added to collection (old to new) as well, only so that we stick to the rule that every value has two orders. I also found small bugs, but we can discuss them in this new PR: (1. clicking current option shouldn’t reload Grid – it should do nothing and 2. clicking Date uploaded (old to new) and then clicking Home correctly resets the order, but dropdown shows wrong value). We can also discuss aesthetics and accessibility there (I can’t operate the dropdown via keyboard).

Thanks again and sorry for extra work and delay in response: festive period got in the way a bit ;-).

AndyKilmory commented 6 months ago

Hi Mateusz, Happy New Year and thanks for the feedback. I’ll share comments with the rest of the team and we can pick up discussions on Monday.

Regards, Andy

Andy Downing (he/him) Senior Software Engineer (Images) Business Systems and Applications Broadcast & End-User Technology (BEUT) BBC Technology Group

From: Mateusz @.> Reply to: guardian/grid @.> Date: Friday, 12 January 2024 at 13:25 To: guardian/grid @.> Cc: Andrew Downing @.>, Mention @.***> Subject: Re: [guardian/grid] 1194 interim filters (PR #4212)

Hello (and happy NY!). Thank you, we have been testing this on TEST. It’s quite a lot of functionality, at least three distinct parts from our point of view: sort UI, Show paid, Permissions. We are not sure what we will adopt here at the Guardian yet. We would definitely want Sort UI as it doesn’t feel org-specific in any way. Show paid is already different between orgs, as ours say Show Free and is controlled by our Permissions (this part breaks with this PR currently), so this part is probably least likely to be shared and would be behind its own config. The coolest part by far is Permissions. Thank you for the extensive config! We think it warrants further discussions IRL, let’s start them at the next Grid Hour.

We think a good practical step would be to ask you to separate Sort UI from this PR. We can then discuss it in isolation. It will need to be added to existing top bar, replacing current sorting UI. It should not be behind any config. I think we should have Added to collection (old to new) as well, only so that we stick to the rule that every value has two orders. I also found small bugs, but we can discuss them in this new PR: (1. clicking current option shouldn’t reload Grid – it should do nothing and 2. clicking Date uploaded (old to new) and then clicking Home correctly resets the order, but dropdown shows wrong value). We can also discuss aesthetics and accessibility there (I can’t operate the dropdown via keyboard).

Thanks again and sorry for extra work and delay in response: festive period got in the way a bit ;-).

— Reply to this email directly, view it on GitHubhttps://github.com/guardian/grid/pull/4212#issuecomment-1889199858, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A6UE4XQGDFCSKB7JFAPCGK3YOE2UJAVCNFSM6AAAAABAVE6XTCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBZGE4TSOBVHA. You are receiving this because you were mentioned.Message ID: @.***>

paperboyo commented 2 months ago

Closing in favour of https://github.com/guardian/grid/pull/4263. Still useful discussion here… Let us know if you want to keep this open, @AndyKilmory.