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.

Sign up for GitHub

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

Jump to bottom

1443 add keywords uploadpage #4139

Merged
merged 11 commits into from
Oct 9, 2023
Merged

1443 add keywords uploadpage #4139

merged 11 commits into from
Oct 9, 2023

Conversation

AndyKilmory
Copy link
Collaborator

Purpose of change is to add 'Keywords' field to the Upload page so that when a user uploads an image they are able to add in 'Keywords' in the same way that they can add in 'Labels' at the time of uploading.

The control has been designed to mimic the look and feel of the pre-existing Add Labels control whilst the styling of the pills showing the added keywords matches that seen on the main metadata info panel.

Screenshot 2023-09-12 at 12 15 24

Testing should verify that keywords can be added and removed in similar fashion to labels and that all other metadata remains intact.

Tested? Documented?

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

@AndyKilmory AndyKilmory requested a review from a team as a code owner September 12, 2023 11:28
@paperboyo
Copy link
Contributor

Hi, @AndyKilmory! This looks very useful. Most of us are at a sports day, but just some initial comments (first point looks serious):

  1. Batch application is confused between Labels and Keywords: sometimes adding/removing one, adds/removes the other:

  2. Hovering over a keyword to launch a search, actually launches a Label search instead:
    search_links

Will have a play some more. I have some minor issues with how it works (no way of committing text field with Enter, or at least Cmd+Enter; some minor rendering/CSS issues), but they also apply to labels so are preexisting.

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Sep 12, 2023 via email

@paperboyo
Copy link
Contributor

you’re UI looks a little different from mine – so I think my configuration isn’t quite the same re. batch application

I think it’s the fact that for Batch UI to show, you need to upload more than one pic (and keep the session, eg. reloading the page will break the batch).

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Sep 12, 2023 via email

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Sep 14, 2023 •
edited
Loading

Issues 1. and 2. raised by Guardian review now addressed.

Have added task to BBC Scrum board to add event to Datalist control to respond to 'Enter' key press to trigger save event.

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Sep 15, 2023 via email

@paperboyo
Copy link
Contributor

Thanks for the fixes! Testing some more, I can see the following issue.

  1. Removing keywords doesn’t work through Batch apply:
    Steps:

    1. Upload more than one image (so Batch functionality is there)
    2. Apply more than one keyword to an image
    3. Click Apply keywords to all arrows
    4. Remove one of the keywords from an image
    5. Click Apply keywords to all arrows

    Expected result:
    The removed keyword should be removed from all images (works if one deletes all keywords, but not when one deletes just one)
    Actual result:
    Batch apply doesn’t remove deleted keyword from other images in the batch

Sadly, it looks like it affects Labels too, so it’s pre-existing! (?) Unsure, then, if it’ś fair to expect it to be fixed as part of this PR…

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Sep 15, 2023 via email

@paperboyo
Copy link
Contributor

I’ll see if I can fix it for both controls – now I know what the expected behaviour is

Thank you so much, @AndyKilmory!

@paperboyo
Copy link
Contributor

paperboyo commented Sep 20, 2023 •
edited
Loading

Hi, @AndyKilmory, testing more on our TEST env, the (preexisting) issue above is now fixed, thank you so much. There is one remaining part of it that is still not working as expected: again, this is preexisting, but very much related. It wasn’t so obvious with Labels, because we don’t read Labes from image metadata, so it would manifest itself only while reuploading images that have Labels already, but it’s more important for Keywords as those we read from images.

Basically: Batch apply arrow button should copy Labels/Keywords from the source image exactly to all other images. After your fix it removes items correctly, but not for previously existing Labels/Keywords:

  1. Upload at least two images, one with preexisting Keyword/Label (either because it was reuploaded, or for keywords: because there is one in the image metadata). Image A has preexisting Keyword Z, Image B has no Keywords.
  2. Add Keyword Y to Image B.
  3. Click Batch apply button on Image B.

Expected result: Keywords from Image B are copied verbatim to Image A. Both images have only Keyword Y. Keyword Z should be removed from Image A.

Actual result: Keyword Z is not removed from Image A, so Image A now has two Keywords (Z & Y) instead of only Y.

Again, issue is preexisting, but I thought, if easy, worth fixing as part of this PR. But do let us know if you want this PR merged and we can open an issue to fix this later!

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Sep 20, 2023 via email

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Oct 2, 2023 via email

@paperboyo
Copy link
Contributor

Sure! Have let the team know to give it a re-review and merge as is (it’s already better as it was, thanks!).

some (...) who wanted to retain the ‘apply as update’ functionality that currently exists whilst others preferred the overwrite as suggested

The Batch apply buttons were always thought of as Synchronise commands: copying the state of the relevant fields as-is to all the images in the upload batch. It was a bug that this wasn’t working for arrays (adding Keywords only made it more obvious as Labels never arrive with metadata). So we will need to fix it to behave the same for preexisting Keywords (and Labels) too regardless of any improvements.

Now, being able to
a) apply only specific members of an array to all other uploads
b)remove all array members from an image in on go (to potentially Batch apply this later to all others)
are both potentially useful. At least a) already exists in the Browser (on a search page) when editing arrays when multiple images are selected (little plus sign to the left of an array member). Not sure if adding them to the upload page wouldn’t be confusing, but it is a possibility.
As to b), I think it’s a good idea (especially for keywords as these can be very messy), but any solution should also work in the Browser and consistently too.

In short, while I think it’s possible to improve things (always!), the fix that I suggested above (for Batch apply to work the same for preexisting as for new array members) isn’t related to any future improvements. Until we do, we have an inconsistent behaviour only for preexisting array members.

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Oct 3, 2023 via email

twrichards
twrichards approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@twrichards twrichards left a comment

Choose a reason for hiding this comment

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

code LGTM (although I've left some suggestions for avoiding let where possible, although there's probably more that I didn't spot) - @paperboyo has tested functionality and I appreciate its been a long time since this PR was raised - thanks for the contribution(s).

Please could you rebase off (or merge in) latest main (we've been making changes to the CI GitHub Action, which this PR needs to sit on top of) then I can merge this in...

kahuna/public/js/components/gr-add-keyword/gr-add-keyword.js
'$window', '$scope', '$q', 'imageAccessor', 'editsService',
function ($window, $scope, $q, imageAccessor, editsService) {

let ctrl = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

not re-assigned so needn't be a let

Suggested change
let ctrl = this;
const ctrl = this;

it is mutated too, but that's angular's way 😢

kahuna/public/js/components/gr-add-keyword/gr-add-keyword.js
ctrl.descriptionOption = overwrite.key;

const updateImages = (images, metadataFieldName, valueFn) => {
let uptodateImages = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let uptodateImages = [];
const upToDateImages = [];

kahuna/public/js/components/gr-add-keyword/gr-add-keyword.js
//-ensure metadata in image is up-to-date-
let tmpImages = $scope.$parent.ctrl.imageAsArray.filter(img => img.uri == image.uri);
if (tmpImages.length > 0) {
let uptodateImage = tmpImages[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let uptodateImage = tmpImages[0];
const upToDateImage = tmpImages[0];

kahuna/public/js/components/gr-add-keyword/gr-add-keyword.js
metadataFieldName,
(image) => {
const currentXInImage = accessor(image);
return currentXInImage ? [...currentXInImage, ...addedX] : [...addedX];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return currentXInImage ? [...currentXInImage, ...addedX] : [...addedX];
return currentXInImage ? [...currentXInImage, ...addedX] : addedX;

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Oct 9, 2023 via email

@twrichards
Copy link
Contributor

hi @AndyKilmory - sorry for the delay, but if you could please do one last rebase/merge of main into your branch, then CI should succeed and I can merge - thank you

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Oct 9, 2023 via email

@prout-bot
Copy link

Seen on image-loader, media-api (created by @AndyKilmory and merged by @twrichards 43 hours, 19 minutes and 40 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on metadata-editor, collections, cropper, usage (created by @AndyKilmory and merged by @twrichards 43 hours, 19 minutes and 46 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on leases (created by @AndyKilmory and merged by @twrichards 43 hours, 19 minutes and 51 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on thrall (created by @AndyKilmory and merged by @twrichards 43 hours, 19 minutes and 58 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on auth, kahuna (created by @AndyKilmory and merged by @twrichards 43 hours, 20 minutes and 3 seconds ago) Please check your changes!

@paperboyo
Copy link
Contributor

Hi, @AndyKilmory.

Messaging here in addition to Slack (see there for details). We’ve noticed a regression bug and we verified it worked correctly before this PR:

Batch-applying Labels, People and Keywords (arrays) using a plus icon (for when not all, but only some of the selected images carry the value) stopped working (console prints error in list-editor.js). Adding a fresh label/person to a selection (where none of the selected images have it) works OK as does deleting a label/person where it’s only carried by some of the selected images.So it’s only adding that’s broken.

Untitled

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Nov 3, 2023 via email

@AndyKilmory
Copy link
Collaborator Author

AndyKilmory commented Nov 3, 2023 via email

@paperboyo
Copy link
Contributor

No need to apologise and mega-thanks for speedy fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twrichards twrichards twrichards approved these changes

Assignees
No one assigned
Labels
Seen-on-auth Seen-on-collections Seen-on-cropper Seen-on-image-loader Seen-on-kahuna Seen-on-leases Seen-on-media-api Seen-on-metadata-editor Seen-on-thrall Seen-on-usage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AndyKilmory @paperboyo @twrichards @prout-bot