Bug 7654 - Add location at opac subscription view
Summary: Add location at opac subscription view
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Koha Team University Lyon 3
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-06 12:42 UTC by Koha Team University Lyon 3
Modified: 2017-04-06 06:27 UTC (History)
9 users (show)

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


Attachments
Add location an callnumber at opac subscription view (2.21 KB, patch)
2012-03-06 12:42 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Add location and callnumber to each subscription in opac subscription tab (2.69 KB, patch)
2012-03-14 10:26 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Commas showing when no data (40.87 KB, image/png)
2012-03-19 16:08 UTC, Nicole C. Engard
Details
Bug 7654: Add location and callnumber at opac subscription view (3.09 KB, patch)
2013-01-17 17:35 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
[PASSED QA] Bug 7654:Add location and callnumber at opac subscription view (3.11 KB, patch)
2013-03-21 17:11 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 7654 follow-up: reindent (2.25 KB, patch)
2013-03-21 17:11 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7654: Add location at opac subscription view (3.04 KB, patch)
2017-01-12 12:07 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Koha Team University Lyon 3 2012-03-06 12:42:24 UTC Comment hidden (obsolete)
Comment 1 delaye 2012-03-13 16:22:17 UTC
l'application du patch dans une sandbox a echoué

Applying: Add location and callnumber at OPAC subscription view
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Add location and callnumber at OPAC subscription view
Comment 2 Koha Team University Lyon 3 2012-03-14 10:26:38 UTC Comment hidden (obsolete)
Comment 3 Nicole C. Engard 2012-03-19 16:08:24 UTC
Created attachment 8306 [details]
Commas showing when no data

There are some stray commas showing up in this patch if you don't have data in all of the fields.
Comment 4 Nicole C. Engard 2012-03-19 16:09:20 UTC
When it comes to Item type it's showing the item type Code and not the description. 

For this and the previous comment I'm marking failed QA.

Nicole
Comment 5 Chris Cormack 2012-03-30 09:25:45 UTC
No new patch attached for signoff, I assume the status change was a mistake, changing back.
Comment 6 Jared Camins-Esakov 2013-01-02 02:12:36 UTC
Could you please attach your updated patch to this?
Comment 7 Koha Team University Lyon 3 2013-01-17 17:35:04 UTC Comment hidden (obsolete)
Comment 8 Pascale Nalon 2013-03-18 11:25:30 UTC
Patch applies
Comment 9 Paul Poulain 2013-03-21 17:11:24 UTC
QA comment:
 * small patch, passes koha-qa.pl
 * i've added a tiny follow-up to remove 2 trailing spaces

Passed QA
Comment 10 Paul Poulain 2013-03-21 17:11:35 UTC
Created attachment 16677 [details] [review]
[PASSED QA] Bug 7654:Add location and callnumber at opac subscription view

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 11 Paul Poulain 2013-03-21 17:11:39 UTC
Created attachment 16678 [details] [review]
[PASSED QA] Bug 7654 follow-up: reindent

Before this patch, there was a trailing space between branch, location and callnumber
After this patch it's written
branch, location, callnumber
as expected

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 12 Jared Camins-Esakov 2013-03-21 23:25:05 UTC
(In reply to comment #8)
> Patch applies

Pascale,

Did you test this patch and sign off on it?
Comment 13 Pascale Nalon 2013-03-27 08:09:09 UTC
Bonjour,

Yes, I signed off this patch during the hackfest in Marseille, last week.
Comment 14 Koha Team University Lyon 3 2013-05-30 08:04:13 UTC
Hello,
I don't understand why this bug is "in discussion" after being signed-off et passed QA. I don't see in the comments an explanation about this status.
Thanks,
Sonia
Comment 15 Katrin Fischer 2013-06-06 06:17:41 UTC
I think we can fix the sign-off line in QA - switching back to signed off.
Comment 16 Jonathan Druart 2013-06-17 15:46:36 UTC
QA comment:

Why do you add a br tag in the h3 title?
The callnumber is hidden 2 line bellow, I think you can remove it.
Maybe it is better to keep the existing one (callnumber). If all values are there, the h3 title will be too long, don't you think?

Marked as Failed QA.
Comment 17 Koha Team University Lyon 3 2013-06-18 09:34:07 UTC
I suppose this br was meant to separate more clearly  the "This is a serial" block from the localisations (if I had put it at the right place, after the h2 element).
Ok for removing the hidden callnumber (didn't really understood what it does here)
As for the length of h3, for us it's ok, may be some will find it too long. I joined a capture of an example in Lyon3.
May be the closed mention ('This subscription is closed.') ,if present,  would have to be move beneath the h3.

Olivier Crouzet
Comment 18 Marc Véron 2016-05-23 15:47:55 UTC
Still valid?
Comment 19 Koha Team University Lyon 3 2016-05-27 14:23:58 UTC
No,it's no more valid

Olivier Crouzet
Comment 20 Julian Maurice 2017-01-12 12:05:48 UTC
Callnumber is already displayed in master, but we can still add the location.
Comment 21 Julian Maurice 2017-01-12 12:07:20 UTC
Created attachment 58869 [details] [review]
Bug 7654: Add location at opac subscription view

Also fix a bug that prevent branch name to be displayed
Comment 22 Mark Tompsett 2017-01-12 19:40:13 UTC
Comment on attachment 58869 [details] [review]
Bug 7654: Add location at opac subscription view

Review of attachment 58869 [details] [review]:
-----------------------------------------------------------------

Here are some untested thoughts. :)

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
@@ +796,4 @@
>                              <p>There are [% subscriptionsnumber %] subscription(s) associated with this title.</p>
>                              [% FOREACH subscription IN subscriptions %]
>                                  [% IF ( subscription.branchcode ) %]
> +                                    <h3>At library: [% Branches.GetName( subscription.branchcode ) %]</h3>

What if it gets a branch, but someone failed to enter a description or it is blank? Displaying the branchcode in that case would make sense. I do not think this correction is correct.

[% at_library = Branches.GetName( subscription.branchcode ) %]
[% SET at_library = subscription.branchcode IF at_library.length=0 %]
[% IF at_library %]
   <h3>At library: [% at_library ) %]</h3>
[% END %]

@@ -799,5 @@
> -                                    <h3>At library: [% Branches.GetName( subscription.branchname ) %]</h3>
> -                                [% ELSE %]
> -                                    [% IF ( subscription.branchcode ) %]
> -                                        <h3>At library: [% subscription.branchcode %]</h3>
> -                                    [% END %]

I'm pretty sure this code was intending to display a branch name if possible, but if not just the branch code. And clearly, it is wrong about that.

@@ +813,5 @@
>                                      <p>[% subscription.opacnote FILTER html_line_break %]</p>
>                                  [% END %]
> +                                [% IF subscription.location_description || subscription.location %]
> +                                    <p>Location: [% IF subscription.location_description %][% subscription.location_description %][% ELSE %][% subscription.location %][% END %]</p>
> +                                [% END %]

[% location_description = subscription.location_description %]
[% SET location_description = subscription.location IF ! defined location_description || location_description.length=0 %]
[% IF location_description %]
    <p>Location: [% location_description %]</p>
[% END %]
Comment 23 Katrin Fischer 2017-01-12 19:45:43 UTC
(In reply to M. Tompsett from comment #22)
> Comment on attachment 58869 [details] [review] [review]
> Bug 7654: Add location at opac subscription view
> 
> Review of attachment 58869 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Here are some untested thoughts. :)
> 
> ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
> @@ +796,4 @@
> >                              <p>There are [% subscriptionsnumber %] subscription(s) associated with this title.</p>
> >                              [% FOREACH subscription IN subscriptions %]
> >                                  [% IF ( subscription.branchcode ) %]
> > +                                    <h3>At library: [% Branches.GetName( subscription.branchcode ) %]</h3>
> 
> What if it gets a branch, but someone failed to enter a description or it is
> blank? Displaying the branchcode in that case would make sense. I do not
> think this correction is correct.

That's actually not a problem. The branchcode and branchname are both mandatory. It's not possible to have a branch without a branchname in Koha. Using the TT plugin like this is common in Koha.
Comment 24 Marc Véron 2017-04-06 06:27:12 UTC
Merge conflict in opac-detail.tt

BTW:
Bug 18388 - Standardize serials volume information displaying will touch the same file