Bug 18881 - Remove dead code in circ/view_holdsqueue.pl
Summary: Remove dead code in circ/view_holdsqueue.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marc Véron
QA Contact: Marcel de Rooy
URL:
Keywords: Academy
: 18859 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-06-29 14:19 UTC by Marc Véron
Modified: 2018-06-04 20:10 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18881: Remove comment about translated itemtype description in circ/view_holdsqueue.pl (951 bytes, patch)
2017-06-29 14:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18881: Remove dead code in circ/view_holdsqueue.pl (1.38 KB, patch)
2017-06-30 18:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18881: Remove dead code from view_holdsqueue.pl (1.02 KB, patch)
2017-07-03 07:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18881: [QA Follow-up] Remove even more (2.61 KB, patch)
2017-07-03 07:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2017-06-29 14:19:11 UTC
Translated itemtype description is not used in display of Holds queue
Comment 1 Marc Véron 2017-06-29 14:23:37 UTC
(In reply to Marc Véron from comment #0)
> Translated itemtype description is not used in display of Holds queue
 
Translated itemtype descriptions display OK, but there is a superfluous comment in circ/view_holdsqueue.pl
Comment 2 Marc Véron 2017-06-29 14:30:03 UTC
Created attachment 64726 [details] [review]
Bug 18881:  Remove comment about translated itemtype description in circ/view_holdsqueue.pl

This patch removes a superfluous FIXME comment.

To test:
Compare code, verify that comment correctly is removed
Comment 3 Mark Tompsett 2017-06-29 15:14:03 UTC
(In reply to Marc Véron from comment #1)
> (In reply to Marc Véron from comment #0)
> > Translated itemtype description is not used in display of Holds queue
>  
> Translated itemtype descriptions display OK, but there is a superfluous
> comment in circ/view_holdsqueue.pl

It is because your description is translated in the first place, or does switching languages actually give a valid translation for each language?
Comment 4 Marc Véron 2017-06-29 16:08:21 UTC
(In reply to M. Tompsett from comment #3)
> (In reply to Marc Véron from comment #1)
> > (In reply to Marc Véron from comment #0)
> > > Translated itemtype description is not used in display of Holds queue
> >  
> > Translated itemtype descriptions display OK, but there is a superfluous
> > comment in circ/view_holdsqueue.pl
> 
> It is because your description is translated in the first place, or does
> switching languages actually give a valid translation for each language?

To have translated item descriptions you can go to 
Home > Administration > Item types > Modify item type 'BK' 

There you have a link 'Translate into other languages' to create translations
To test, you can even 'translate' for English to overwrite the original description.
Comment 5 Mark Tompsett 2017-06-29 22:37:01 UTC
(In reply to Marc Véron from comment #4)
> To have translated item descriptions you can go to 
> Home > Administration > Item types > Modify item type 'BK' 
> 
> There you have a link 'Translate into other languages' to create translations
> To test, you can even 'translate' for English to overwrite the original
> description.

But that is by overwriting the actual description. Not by just changing the language selected. I think the comment was about having a way to po-i-size it. Patch is okay, but it's really a commentary on having DB translations, not just templates, I think.
Comment 6 Katrin Fischer 2017-06-30 05:46:01 UTC
Did you check the description Marc gave? The value will change with switching the language if you have configured it.

It doesn't make sense to have po files for configurable values that are different for each library. The new way to save the translations in a separate table works better, as every library can change it any time. I think Marc's patch is correct.
Comment 7 Marc Véron 2017-06-30 05:55:13 UTC
(In reply to M. Tompsett from comment #5)
> (In reply to Marc Véron from comment #4)
> > To have translated item descriptions you can go to 
> > Home > Administration > Item types > Modify item type 'BK' 
> > 
> > There you have a link 'Translate into other languages' to create translations
> > To test, you can even 'translate' for English to overwrite the original
> > description.
> 
> But that is by overwriting the actual description. Not by just changing the
> language selected. I think the comment was about having a way to po-i-size
> it. Patch is okay, but it's really a commentary on having DB translations,
> not just templates, I think.

Maybe there is a misunderstanding.

- Item types are defined by the library (or loaded together with the example values, in the language choosen for installation).
- When defining an item type, the library can define translations for other (installed) languages as well. E.g. in Switzerland a library could define an item type with code 'BUCH' and the description "Buch" for German and the translated descriptions "Livre" for French and "Libro" for Italian
- The (translated) description appears (is overlayed) depending on the language selected by the user.
- You can not po-i-size that because the library defines the item types.
Comment 8 Mark Tompsett 2017-06-30 15:25:20 UTC
(In reply to Marc Véron from comment #7)
> Maybe there is a misunderstanding.

Perhaps. I'll just remove myself for the conversation. Please continue.
Comment 9 Marcel de Rooy 2017-06-30 17:48:16 UTC
If we remove this comment, some day somebody wonders again why we do not use the translated description.
So instead of removing I would opt for adding a legitimate reason why we really do not want a translation.
Comment 10 Marc Véron 2017-06-30 18:19:17 UTC
(In reply to Marcel de Rooy from comment #9)
> If we remove this comment, some day somebody wonders again why we do not use
> the translated description.
> So instead of removing I would opt for adding a legitimate reason why we
> really do not want a translation.

The translation takes place by overlaying in the template, using the itemtype.
The whole line is dead code.

Changed bug title as appropriate.
Comment 11 Marc Véron 2017-06-30 18:30:07 UTC
Created attachment 64761 [details] [review]
Bug 18881: Remove dead code in circ/view_holdsqueue.pl

Remove a line of dead code from circ/view_holdsqueue.pl

To test:
- Apply patch
- Go to Home > Circulation > Holds queue
- Verify that description in Item type column displays as before
- Create a translation for an item type:
  Home > Administration > Item types > Modify item type 'BK'
  There you have a link 'Translate into other languages' to create translations
  To test, you can even 'translate' for English to overwrite the original
  description.
- Verify that the translated descriptions displays when changing the language
  as appropriate
- For an explanation how translation of item types descriptions work and why
  it is not done with Pootle see comment #7
Comment 12 Marcel de Rooy 2017-06-30 19:49:13 UTC
Applying: Bug 18881: Remove dead code in circ/view_holdsqueue.pl
fatal: sha1 information is lacking or useless (circ/view_holdsqueue.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 13 Marc Véron 2017-06-30 22:50:17 UTC
(In reply to Marcel de Rooy from comment #12)
> Applying: Bug 18881: Remove dead code in circ/view_holdsqueue.pl
> fatal: sha1 information is lacking or useless (circ/view_holdsqueue.pl).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.

Re-tested, applies cleanly here (on top of Bug 18859).
Comment 14 Marcel de Rooy 2017-07-03 07:49:34 UTC
*** Bug 18859 has been marked as a duplicate of this bug. ***
Comment 15 Marcel de Rooy 2017-07-03 07:51:27 UTC
Created attachment 64769 [details] [review]
Bug 18881: Remove dead code from view_holdsqueue.pl

Aleisha spotted the typo in $itemtypes and proposed a correction on bug 18859.
The description was not even used. Template calls GetDescription.

To test:
Verify that viewing the holds queue still works as expected.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2017-07-03 07:51:31 UTC
Created attachment 64770 [details] [review]
Bug 18881: [QA Follow-up] Remove even more

Removing the commented section from the template: If it does not work, it should not be here.
When it works again, put it back in.

Since @itemtypesloop is not used, remove it from the script too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2017-07-03 07:54:45 UTC
Taking the liberty to move this directly into PQA now
Comment 18 Jonathan Druart 2017-07-06 18:52:46 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 19 Fridolin Somers 2017-07-12 09:50:36 UTC
Pushed to 17.05.x, will be in 17.05.02

Dead code go to hell ;)
Comment 20 Katrin Fischer 2017-07-16 11:53:39 UTC
This won't get ported back to 16.11.x as it is an enhancement.