Bug 27130 - Adding local cover image at item level shows 'File type' section
Summary: Adding local cover image at item level shows 'File type' section
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 26145
Blocks:
  Show dependency treegraph
 
Reported: 2020-12-02 07:16 UTC by Fridolin Somers
Modified: 2021-02-11 22:25 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.02


Attachments
Bug 27130: Don't display itemnumber under "File type" section (2.97 KB, patch)
2020-12-02 07:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27130: Don't display itemnumber under "File type" section (3.38 KB, patch)
2020-12-02 15:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27130: Don't display itemnumber under "File type" section (3.44 KB, patch)
2020-12-02 16:08 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27130: Don't display itemnumber under "File type" section (3.44 KB, patch)
2020-12-02 16:09 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27130: [alternate] Don't display itemnumber under "File type" section (4.16 KB, patch)
2021-01-06 12:51 UTC, Nick Clemens
Details | Diff | Splinter Review
After original patch (88.85 KB, image/png)
2021-01-06 12:53 UTC, Nick Clemens
Details
Bug 27130: [alternate] Don't display itemnumber under "File type" section (7.00 KB, patch)
2021-01-06 13:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27130: Don't display itemnumber under "File type" section (7.04 KB, patch)
2021-01-07 12:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27130: Don't display itemnumber under "File type" section (7.10 KB, patch)
2021-01-11 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2020-12-02 07:16:42 UTC
When adding a local cover image at item level, after file upload there is a section 'File type'.
This is for multiple files upload into a zip for biblio level.

Not sure how to fix this.
Comment 1 Jonathan Druart 2020-12-02 07:57:40 UTC
Created attachment 114084 [details] [review]
Bug 27130: Don't display itemnumber under "File type" section

There is no need to have the "File type" section when we attach a cover
item to an item (we cannot upload a zip file)

This patch removes the legend of the fieldset.

Test plan:
Attach a cover image to an image, then to a bibliographic record.
Notice the change and confirm it makes sense.
Comment 2 Fridolin Somers 2020-12-02 15:39:49 UTC
When attaching a cover image to an item, form has a <ol> without <li> :
  <fieldset class="rows">
    <ol>
      <label for="itemnumber">Cover itemnumber: </label>
      <input type="text" id="itemnumber" name="itemnumber" value="108" size="15" readonly="readonly">
      <input type="hidden" name="filetype" value="image">
    </ol>

add a <li> ?
Comment 3 Jonathan Druart 2020-12-02 15:54:23 UTC
Created attachment 114102 [details] [review]
Bug 27130: Don't display itemnumber under "File type" section

There is no need to have the "File type" section when we attach a cover
item to an item (we cannot upload a zip file)

This patch removes the legend of the fieldset.

Test plan:
Attach a cover image to an image, then to a bibliographic record.
Notice the change and confirm it makes sense.
Comment 4 Fridolin Somers 2020-12-02 16:08:31 UTC
Created attachment 114103 [details] [review]
Bug 27130: Don't display itemnumber under "File type" section

There is no need to have the "File type" section when we attach a cover
item to an item (we cannot upload a zip file)

This patch removes the legend of the fieldset.

Test plan:
Attach a cover image to an image, then to a bibliographic record.
Notice the change and confirm it makes sense.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 5 Fridolin Somers 2020-12-02 16:09:19 UTC
Created attachment 114104 [details] [review]
Bug 27130: Don't display itemnumber under "File type" section

There is no need to have the "File type" section when we attach a cover
item to an item (we cannot upload a zip file)

This patch removes the legend of the fieldset.

Test plan:
Attach a cover image to an item, then to a bibliographic record.
Notice the change and confirm it makes sense.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 6 Nick Clemens 2020-12-21 19:17:47 UTC
I don't think this makes things much better - why do we have an itemnumber that we cannot edit? It would be better to have that in the breadcrumbs or page title, no?

Also, it seems the code for supporting multiple image uploads (zip file) has references to itemnumber, but never actually receive/calculate it.

In my opinion the bug is that one cannot choose a zip file to upload item covers.

When attaching a cover to the biblio we actually can edit the biblionumber, which seems like a bad idea, but we should at least be consistent
Comment 7 Jonathan Druart 2021-01-05 06:21:13 UTC
(In reply to Nick Clemens from comment #6)
> I don't think this makes things much better - why do we have an itemnumber
> that we cannot edit? It would be better to have that in the breadcrumbs or
> page title, no?

I don't understand what you mean.
When I upload a cover image for an item I am at /cgi-bin/koha/tools/upload-cover-image.pl?itemnumber=37&filetype=image

I don't see any info about the biblio or the item. Are you suggesting we should add them on this view?

> Also, it seems the code for supporting multiple image uploads (zip file) has
> references to itemnumber, but never actually receive/calculate it.
> 
> In my opinion the bug is that one cannot choose a zip file to upload item
> covers.

Yes, it's a limitation from bug 26145. We could eventually implement it but it's not the point of this bug report.

> When attaching a cover to the biblio we actually can edit the biblionumber,
> which seems like a bad idea, but we should at least be consistent

Indeed, we could remove the readonly attribute.
Comment 8 Nick Clemens 2021-01-06 12:51:21 UTC Comment hidden (obsolete)
Comment 9 Nick Clemens 2021-01-06 12:53:22 UTC
Created attachment 114889 [details]
After original patch

The form now includes a disabled itemnumber input
Comment 10 Nick Clemens 2021-01-06 12:54:16 UTC
(In reply to Jonathan Druart from comment #7)
> I don't understand what you mean.
> When I upload a cover image for an item I am at
> /cgi-bin/koha/tools/upload-cover-image.pl?itemnumber=37&filetype=image
> 
> I don't see any info about the biblio or the item. Are you suggesting we
> should add them on this view?

I attached an image and an alternate patch that removes the fieldset all together and moves the info
Comment 11 Jonathan Druart 2021-01-06 13:22:35 UTC
both patches are identical :)
Comment 12 Nick Clemens 2021-01-06 13:44:11 UTC
Created attachment 114890 [details] [review]
Bug 27130: [alternate] Don't display itemnumber under "File type" section

There is no need to have the "File type" section when we attach a cover
item to an item (we cannot upload a zip file)

This patch removes the fieldset abd adds the itemnumebr to the breadcrumbs and headings.
It also adjusts the help text, removing ZIP as an option when uploading for a biblio or item

Lastly it adjusts the results page to use a heading to make things a bit clearer

Test plan:
 1 - Enable 'LocalCoverImages' system preference
 2 - Browse to the details page for a record
 3 - Under the 'Images' tab click 'Upload'
 4 - Note the display includes File type section with a biblionuber input
 5 - Upload an image and note the results page
 6 - Return to the biblio, this time on the 'Holdings' tab
 7 - Click the down arrow next to 'Edit' and choose 'Upload image'
 8 - Note the form has a File type section with an itemnumber input
 9 - Apply patch
10 - Repeat steps 2-9, making sure changes to page make sense
11 - Note the breadcrumbs have been updated
12 - Confirm you can complete the upload of the cover for an item and that results look as expected
Comment 13 Nick Clemens 2021-01-06 13:44:56 UTC
(In reply to Jonathan Druart from comment #11)
> both patches are identical :)

Heh, 'git commit --amend' != 'git commit --amend -a'
Comment 14 Jonathan Druart 2021-01-07 10:36:39 UTC
Nick, that's not possible to completely hide the input.
If you are coming from the tools module you need to pass the biblionumber

 Home › Tools › Upload local cover image
Comment 15 Nick Clemens 2021-01-07 11:46:57 UTC
(In reply to Jonathan Druart from comment #14)
> Nick, that's not possible to completely hide the input.
> If you are coming from the tools module you need to pass the biblionumber
> 
>  Home › Tools › Upload local cover image

I don't hide the input when coming from tools - upload a file - select 'Image file' as type, and the biblionumber input appears - maybe I misunderstand?
Comment 16 Jonathan Druart 2021-01-07 12:12:46 UTC
Created attachment 114917 [details] [review]
Bug 27130: Don't display itemnumber under "File type" section

There is no need to have the "File type" section when we attach a cover
item to an item (we cannot upload a zip file)

This patch removes the fieldset abd adds the itemnumebr to the breadcrumbs and headings.
It also adjusts the help text, removing ZIP as an option when uploading for a biblio or item

Lastly it adjusts the results page to use a heading to make things a bit clearer

Test plan:
 1 - Enable 'LocalCoverImages' system preference
 2 - Browse to the details page for a record
 3 - Under the 'Images' tab click 'Upload'
 4 - Note the display includes File type section with a biblionuber input
 5 - Upload an image and note the results page
 6 - Return to the biblio, this time on the 'Holdings' tab
 7 - Click the down arrow next to 'Edit' and choose 'Upload image'
 8 - Note the form has a File type section with an itemnumber input
 9 - Apply patch
10 - Repeat steps 2-9, making sure changes to page make sense
11 - Note the breadcrumbs have been updated
12 - Confirm you can complete the upload of the cover for an item and that results look as expected

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2021-01-07 12:13:27 UTC
That's me, the behaviour is great with your patch, thanks!
Comment 18 Martin Renvoize 2021-01-11 12:14:09 UTC
Created attachment 115014 [details] [review]
Bug 27130: Don't display itemnumber under "File type" section

There is no need to have the "File type" section when we attach a cover
item to an item (we cannot upload a zip file)

This patch removes the fieldset abd adds the itemnumebr to the breadcrumbs and headings.
It also adjusts the help text, removing ZIP as an option when uploading for a biblio or item

Lastly it adjusts the results page to use a heading to make things a bit clearer

Test plan:
 1 - Enable 'LocalCoverImages' system preference
 2 - Browse to the details page for a record
 3 - Under the 'Images' tab click 'Upload'
 4 - Note the display includes File type section with a biblionuber input
 5 - Upload an image and note the results page
 6 - Return to the biblio, this time on the 'Holdings' tab
 7 - Click the down arrow next to 'Edit' and choose 'Upload image'
 8 - Note the form has a File type section with an itemnumber input
 9 - Apply patch
10 - Repeat steps 2-9, making sure changes to page make sense
11 - Note the breadcrumbs have been updated
12 - Confirm you can complete the upload of the cover for an item and that results look as expected

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-01-11 12:14:40 UTC
Much better and works as expected, no regressions found.

Passing QA
Comment 20 Jonathan Druart 2021-01-12 10:43:09 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 21 Fridolin Somers 2021-01-14 16:28:08 UTC
Pushed to 20.11.x for 20.11.02
Comment 22 Andrew Fuerste-Henry 2021-01-22 15:58:52 UTC
Missing dependency, not backported to 20.05