Bug 15374 - Translatability: Fix issues on OPAC summary page
Summary: Translatability: Fix issues on OPAC summary page
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Marc Véron
QA Contact: Testopia
URL:
Keywords:
Depends on: 14930
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-14 23:09 UTC by Marc Véron
Modified: 2016-12-05 21:22 UTC (History)
4 users (show)

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


Attachments
Bug 15374 - Translatability: Fix issues on OPAC summary page (8.84 KB, patch)
2015-12-14 23:25 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15374 - Translatability: Fix issues on OPAC summary page (10.57 KB, patch)
2015-12-21 07:36 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15374 - Translatability: Fix issues on OPAC summary page (10.66 KB, patch)
2015-12-22 19:42 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 15374 - Translatability: Fix issues on OPAC summary page (10.83 KB, patch)
2016-01-04 17:24 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15374 - Translatability: Fix issues on OPAC summary page (10.88 KB, patch)
2016-01-12 22:33 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 15374 - Translatability: Fix issues on OPAC summary page (11.18 KB, patch)
2016-02-03 23:01 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 15374 - Translatability: Fix issues on OPAC summary page (11.09 KB, patch)
2016-02-04 20:58 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15374 - Translatability: Fix issues on OPAC summary page (11.13 KB, patch)
2016-02-05 07:48 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15374 - Translatability: Fix issues on OPAC summary page (11.18 KB, patch)
2016-02-08 20:58 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 15374: (followup) Remove 2 obsolete lines from opac-user.pl (992 bytes, patch)
2016-02-10 10:51 UTC, Marc Véron
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 2015-12-14 23:09:57 UTC
Fix several issues on opac-user.tt with sentences splitted by tags, leading to odd translations. 

Additionally, fix a broken link that should point to opac-account.pl
Comment 1 Marc Véron 2015-12-14 23:25:21 UTC Comment hidden (obsolete)
Comment 2 Chris Kirby 2015-12-15 22:56:33 UTC
Hi Marc,

Applied the patch on Sandbox 14. 

- Logged in to OPAC
  - with blocked /debarred user (with and without comment and date)
  - with user who has his card marked as lost
  - with user with an account that is about to expire
  - with user with expired account
  - with user with uncertain address information
  - with user who has got a circulation message (from checkout page)
  - with user who has a OPAC note
- Verify that information is correct and displays nicely.  I did not find that  links in messages (for example to books in the OPAC) worked.

- Attempted to test with user who has to much fines. Created manual fines/lost items charges, but fines did NOT display in "total due".

On another sandbox, I did not apply the patch and found that for manual restriction, the expiration date picker did NOT pop up. Was fixing this part of your patch?
Comment 3 Marc Véron 2015-12-15 23:58:10 UTC
Hi Chris,

Thanks for testing!

Hmm, you talk about date picker and links to books, did you really test the OPAC summary page?
Home > [Patron's name] > Your summary
[server]/cgi-bin/koha/opac-user.pl

It is the page that appears immediately after a log in to the OPAC ( at the left the tab 'your summary').

After log in you should see messages like the following (depending on the user's status):
----
 Please note: Your account has expired as of 15.12.2015. Please contact the library if you wish to renew your account.

    Please note: According to our records, we don't have up-to-date contact information. Please contact the library. Update your contact information (Please note: there may be a delay in restoring your account if you submit online.)
    Please note: Your library card has been marked as lost or stolen. If this is an error, please contact the library.
    Please note: You cannot renew your books online. Reason: Your fines exceed 100.00. Please pay your fines if you wish to renew your books. Go to your account page
----
Comment 4 Marc Véron 2015-12-16 18:00:36 UTC
Note: To test for a user who has to much fines, you can

- go to staff client

- set syspref 'OPACFineNoRenewals' e.g. to 100 
  (if empty nothing is calculated)

- add a manual fine for this user, e.g. 200
  (Search a user, 'Fines' in lefthand menue, 
   Tab 'Create manual invoice')
Comment 5 Aleisha Amohia 2015-12-20 23:30:36 UTC
Testing with a user who has too many fines did not work. I had the syspref set correctly and created a manual invoice of $100 but there was no error message saying the account was blocked due to fines. A 'Fines' tab showed up saying 'Fines (100)', which I think would be better to say $100.00.
Didn't see my circulation message, not sure how this is meant to affect the OPAC?
Comment 6 Marc Véron 2015-12-21 07:36:28 UTC Comment hidden (obsolete)
Comment 7 Marc Véron 2015-12-21 07:58:21 UTC
(In reply to Aleisha Amohia from comment #5)
> Testing with a user who has too many fines did not work. I had the syspref
> set correctly and created a manual invoice of $100 but there was no error
> message saying the account was blocked due to fines. A 'Fines' tab showed up
> saying 'Fines (100)', which I think would be better to say $100.00.
> Didn't see my circulation message, not sure how this is meant to affect the
> OPAC?

Hi Aleisha, thanks for testing.

- I amended the patch to format fines tab as well. (100.00 instead of 100). Regarding the display of the currency sign see Bug 4078 (waiting for Sign-off)

Regarding a user who has too many fines:
- I re-tested with syspref OPACFineNoRenewals set to 100 and a patron 'Tester' with fines (200) and logged in to the OPAC with this user. (Expecting: A message that now renewals are possible due to fines.)

Without patch, I get after log-in (cgi-bin/koha/opac-user.pl):
- Warning message:
Please note:  Since you have  more than 100.00 in fines, you cannot renew your books online. Please pay your fines if you wish to renew your books.

- Tab Heading:
Fines (200.00)

With patch, I get:
- Warning message:
Please note: You cannot renew your books online. Reason: Your fines exceed 100.00. Please pay your fines if you wish to renew your books. Go to your account page
[As expected. Better wording / no sentence splitting for translatability]

- Tab Heading:
Fines: 200.00
Comment 8 Marc Véron 2015-12-21 08:30:50 UTC
Note: I amended "Bug 4078 - Display active currency symbol on currency output and input" in a way that it can be tested on top of this bug. (But it does not depend on this bug nor blocks this bug).
Comment 9 Aleisha Amohia 2015-12-21 20:15:14 UTC
> With patch, I get:
> - Warning message:
> Please note: You cannot renew your books online. Reason: Your fines exceed
> 100.00. Please pay your fines if you wish to renew your books. Go to your
> account page
> [As expected. Better wording / no sentence splitting for translatability]
> 
> - Tab Heading:
> Fines: 200.00

The fines tab looks much better now but I'm still not seeing any warning message when I log in. I get a warning message when I try to place a hold on something but not on the summary page when I log in.

Also I tested with your patch for Bug 4078 and it works well, so can I sign off on that one?
Comment 10 Marc Véron 2015-12-22 07:29:15 UTC
(In reply to Aleisha Amohia from comment #9)

>(...)I'm still not seeing any warning message when I log in. 

I can reproduce the behaviour. It is related to a system preference

OpacRenewalAllowed: [Allow] patrons to renew their own books on the OPAC.
Comment 11 Aleisha Amohia 2015-12-22 19:42:20 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2016-01-04 13:43:27 UTC
CONFLICT (content): Merge conflict in opac/opac-user.pl
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
Comment 13 Marc Véron 2016-01-04 17:24:20 UTC Comment hidden (obsolete)
Comment 14 Aleisha Amohia 2016-01-12 22:33:11 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2016-01-13 10:17:51 UTC
Marc, I have not tested the patch, but it seems that a conflict with bug 15344 has not been fixed correctly.
BORROWER_INFO should not be BORROWER_INF
Comment 16 Aleisha Amohia 2016-02-03 23:01:53 UTC Comment hidden (obsolete)
Comment 17 Marc Véron 2016-02-04 19:39:27 UTC
Does not apply...
Comment 18 Marc Véron 2016-02-04 19:42:30 UTC
Should be fixed on top of 14930
Comment 19 Marc Véron 2016-02-04 20:00:03 UTC
Comment on attachment 47626 [details] [review]
Bug 15374 - Translatability: Fix issues on OPAC summary page

Oh, the latest patch contains merge markers (<<<<<<< HEAD etc)
Comment 20 Marc Véron 2016-02-04 20:58:27 UTC Comment hidden (obsolete)
Comment 21 Aleisha Amohia 2016-02-04 21:18:51 UTC
Applying: Bug 15374 - Translatability: Fix issues on OPAC summary page
fatal: sha1 information is lacking or useless (opac/opac-user.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 15374 - Translatability: Fix issues on OPAC summary page
Comment 22 Marc Véron 2016-02-04 23:21:43 UTC
Hi Aleisha,

I just re-tested. PThe patch applies fine on top of Bug 14930.
Comment 23 Aleisha Amohia 2016-02-04 23:30:33 UTC
(In reply to Marc Véron from comment #22)
> Hi Aleisha,
> 
> I just re-tested. PThe patch applies fine on top of Bug 14930.

Oops I completely forgot to apply the dependency (silly me). Will retest
Comment 24 Aleisha Amohia 2016-02-04 23:48:03 UTC
There are a couple of problems with the patch.

When the user has too many fines, there is no message on the summary page to say the user is blocked from check outs.
Also, small typo: When the account has a restriction, the message says "If shows your account to be clear", which doesn't make sense. Perhaps change to "If your account is shown to be clear"?
Comment 25 Marc Véron 2016-02-05 07:41:25 UTC
Hmm, testing...  Seems OK to me...

- Syspref OpacRenewalAllowed: Allow
- Syspref OPACFineNoRenewals: 2
- Patron has fines: Total due: 	5.01

- Log-in to Opac with patron, go to 'your summary'
Result: "Please note: You cannot renew your books online. Reason: Your fines exceed 2.00. Please pay your fines if you wish to renew your books. Go to your account page"
OK

- Syspref OPACFineNoRenewals: 10
- Patron has fines: Total due: 	5.01
- Refresh 'your summary'
Result: No message
OK


- Syspref OPACFineNoRenewals: 10
- Add manual fine for patron (10). Now total due is: 	15.01
- Refresh 'your summary'
Result: "Please note: You cannot renew your books online. Reason: Your fines exceed 10.00. Please pay your fines if you wish to renew your books. Go to your account page"
OK

- Syspref OPACFineNoRenewals: 0
- Patron has fines: Total due: 	15.01
- Refresh 'your summary'
Result: "Please note: You cannot renew your books online. Reason: You have fines. Please pay your fines if you wish to renew your books. Go to your account page"
OK

- Syspref OPACFineNoRenewals: (empty)
- Refresh 'your summary'
Result: No message
OK

- Syspref OPACFineNoRenewals: xyndjeksfjasdf 
  (non-numeric, is not validated by syspref form)
- Refresh 'your summary'
Result: No message
OK

- Syspref OPACFineNoRenewals: (one space char) 
  (non-numeric, is not validated by syspref form)
- Refresh 'your summary'
Result: No message
OK
Comment 26 Marc Véron 2016-02-05 07:48:03 UTC Comment hidden (obsolete)
Comment 27 Marc Véron 2016-02-05 07:54:38 UTC
Hi Aleisha,
Thank you very much for continuous testing. 

I could not reproduce your issue from comment #24, see my tests in comment #25. If I missed something, please provide information on how you got your result.

I attached a patch with fixed wording.
Comment 28 Aleisha Amohia 2016-02-08 20:57:27 UTC
(In reply to Marc Véron from comment #27)
> Hi Aleisha,
> Thank you very much for continuous testing. 
> 
> I could not reproduce your issue from comment #24, see my tests in comment
> #25. If I missed something, please provide information on how you got your
> result.
> 
Hi Marc,

I mean that when a user has been blocked from checkouts because their fine balance is over the limit, I feel like a message should show up saying this, regardless of if OpacRenewalAllowed is set to Allow or not.
Comment 29 Aleisha Amohia 2016-02-08 20:58:35 UTC
Created attachment 47772 [details] [review]
Bug 15374 - Translatability: Fix issues on OPAC summary page

Fix several issues on opac-user.tt with sentences splitted by tags,
leading to odd translations.

Additionally, fix a broken link that should point to opac-account.pl

To test:
- Apply patch
- Log in to OPAC
  - with blocked /debarred user (with and without comment and date)
  - with user who has to much fines
  - with user who has his card marked as lost
  - with user with an account that is about to expire
  - with user with expired account
  - with user with uncertain address information
  - with user who has got a circulation message (from checkout page)
  - with user who has a OPAC note
- Verify that information is correct and displays nicely and that links
  in messages work
- Carefully examine code
- Search for regressions

(Amended to add price formatting in fines tab label)

Can see the warning message now :) signing off

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>

(Amended to make patches apply, see comment #12)

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>

(Amended to make it apply and to fix BORROWER.INF -> BORROWER->INFO.
 Needs new sign-off. / MV)
(Amended to fix typo from comment #24 / mv)

Amended to fix merge conflict
Comment 30 Marc Véron 2016-02-09 07:06:45 UTC
(In reply to Aleisha Amohia from comment #28)
> (In reply to Marc Véron from comment #27)
> > Hi Aleisha,
> > Thank you very much for continuous testing. 
> > 
> > I could not reproduce your issue from comment #24, see my tests in comment
> > #25. If I missed something, please provide information on how you got your
> > result.
> > 
> Hi Marc,
> 
> I mean that when a user has been blocked from checkouts because their fine
> balance is over the limit, I feel like a message should show up saying this,
> regardless of if OpacRenewalAllowed is set to Allow or not.

Hi Aleisha,

Is that something that changes with this path ( = a regression) or is the behaviour the same without patch?

- If it is a regression, please fail the patch

- If the behaviour is the same with and without patch, I propose to file a new Bug on top of this one.
Comment 31 Aleisha Amohia 2016-02-09 19:57:18 UTC
(In reply to Marc Véron from comment #30)
> Hi Aleisha,
> 
> Is that something that changes with this path ( = a regression) or is the
> behaviour the same without patch?
> 
> - If it is a regression, please fail the patch
> 
> - If the behaviour is the same with and without patch, I propose to file a
> new Bug on top of this one.

Ah yes I understand now. The behaviour is the same with and without the patch so I will file a new bug and sign off on this one now.

Sorry for messing you around a bit!
Comment 32 Marc Véron 2016-02-10 09:12:43 UTC
(In reply to Aleisha Amohia from comment #31)
> (In reply to Marc Véron from comment #30)
> > Hi Aleisha,
> > 
> > Is that something that changes with this path ( = a regression) or is the
> > behaviour the same without patch?
> > 
> > - If it is a regression, please fail the patch
> > 
> > - If the behaviour is the same with and without patch, I propose to file a
> > new Bug on top of this one.
> 
> Ah yes I understand now. The behaviour is the same with and without the
> patch so I will file a new bug and sign off on this one now.
> 
> Sorry for messing you around a bit!

Hi Aleisha,
Thanks for SO and no problem - somtimes to much trees prevent us to see the forest, as a German proverb says... 
:-)
Comment 33 Jonathan Druart 2016-02-10 09:27:14 UTC
Comment on attachment 47772 [details] [review]
Bug 15374 - Translatability: Fix issues on OPAC summary page

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

::: opac/opac-user.pl
@@ +123,5 @@
>      $borr->{'amountoutstanding'} = -1 * ( $borr->{'amountoutstanding'} );
>  }
>  
> +my @bordat;
> +$bordat[0] = $borr;

hum?
Looks like a merge conflict issue.
Comment 34 Marc Véron 2016-02-10 10:51:04 UTC
Created attachment 47842 [details] [review]
Bug 15374: (followup) Remove 2 obsolete lines from opac-user.pl

This patch removes 2 obsolete lines from opac-user.pl, see comment #33
Comment 35 Marc Véron 2016-02-12 08:43:01 UTC
I give up this bug in favor of Bug 15775.
Maybe some of the code here can then be used on top of 15775.