Bug 15049 - Add warning about "No active currency" to Acquisitions start page
Summary: Add warning about "No active currency" to Acquisitions start page
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marc Véron
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15987
  Show dependency treegraph
 
Reported: 2015-10-22 09:43 UTC by Marc Véron
Modified: 2017-06-14 22:02 UTC (History)
5 users (show)

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


Attachments
Bug 15049 - Add warning about "No active currency" to Acquisitions start page (1.21 KB, patch)
2015-10-22 09:49 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15049 - Add warning about "No active currency" to Acquisitions start page (1.99 KB, patch)
2015-10-26 21:06 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15049 - Add warning about "No active currency" to Acquisitions start page (3.42 KB, patch)
2015-10-27 14:08 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15049 - Add warning about "No active currency" to Acquisitions start page (3.33 KB, patch)
2015-10-27 14:13 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15049: Add warning about "No active currency" to Acquisitions start page (3.43 KB, patch)
2015-10-27 15:03 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15049: Hide currency but preserv alert (2.73 KB, patch)
2015-10-29 12:29 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15049: Hide currency but preserv alert (2.73 KB, patch)
2015-10-29 13:59 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15049: (Signed off) Hide currency but preserv alert (2.94 KB, patch)
2015-12-01 15:32 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15049: Add warning about "No active currency" to Acquisitions start page (3.48 KB, patch)
2015-12-17 15:11 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 15049: (Signed off) Hide currency but preserv alert (3.00 KB, patch)
2015-12-17 15:11 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 15049: (followup) Add warning about "No active currency" (3.47 KB, patch)
2015-12-18 15:05 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15049: Add warning about "No active currency" to Acquisitions start page (3.55 KB, patch)
2015-12-28 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15049: (Signed off) Hide currency but preserv alert (3.07 KB, patch)
2015-12-28 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15049: (followup) Add warning about "No active currency" (3.54 KB, patch)
2015-12-28 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15049: Simplify code in aqplan.tt (2.07 KB, patch)
2015-12-28 10:30 UTC, Jonathan Druart
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-10-22 09:43:55 UTC
If no active currency is defined, Acquisitions dose not work properly and there can occur software errors while receiving.
This patch adds the warning about missing active currency to Home > Acquisitions
Comment 1 Marc Véron 2015-10-22 09:49:38 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-10-26 21:06:33 UTC Comment hidden (obsolete)
Comment 3 Bernardo Gonzalez Kriegel 2015-10-26 22:16:16 UTC
You need to use

currency => GetCurrency()->{currency},

in acqui/acqui-home.pl, if not you get "Currency = HASH(0x763c5a8)"

Save for that, it works Ok & without errors.
Comment 4 Marc Véron 2015-10-26 22:31:59 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #3)
> You need to use
> 
> currency => GetCurrency()->{currency},
> 
> in acqui/acqui-home.pl, if not you get "Currency = HASH(0x763c5a8)"
> 
> Save for that, it works Ok & without errors.

Hi Bernardo,

Thanks for pointing that out.

However with GetCurrency()->{currency} I get:

Can't use an undefined value as a HASH reference at /usr/share/kohaclone/acqui/acqui-home.pl line 117.

The reason is that GetCurrency does a SELECT * FROM currency where active = '1'
(C4/Budgets.pm, after line 932)

If no active currency is set the return value is not defined.
Comment 5 Bernardo Gonzalez Kriegel 2015-10-27 12:25:02 UTC
(In reply to Marc Véron from comment #4)
> 
> However with GetCurrency()->{currency} I get:
> 
> Can't use an undefined value as a HASH reference at
> /usr/share/kohaclone/acqui/acqui-home.pl line 117.
> 
> The reason is that GetCurrency does a SELECT * FROM currency where active =
> '1'
> (C4/Budgets.pm, after line 932)
> 
> If no active currency is set the return value is not defined.

Yeap, but 'budgets-active-currency.inc' expects the currency and not a hashref.
Other files that use the include put "currency => $cur->{currency}"

Perhaps this code

my $cur = GetCurrency();
if ( $cur ) {
   $template->param(
        currency      => $cur->{currency},
    );
}
Comment 6 Marc Véron 2015-10-27 14:07:46 UTC
Additionaly there was an issue if an user had no permission to manage the currencies table. In such case the warning message should not display a link to the currencies.

Patch follows.
Comment 7 Marc Véron 2015-10-27 14:08:08 UTC Comment hidden (obsolete)
Comment 8 Marc Véron 2015-10-27 14:13:39 UTC Comment hidden (obsolete)
Comment 9 Bernardo Gonzalez Kriegel 2015-10-27 15:03:38 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-10-29 09:14:54 UTC
Now there is a "Currency = EUR" on top of the acqui home page.
I am not sure it is useful to display it there.
Comment 11 Bernardo Gonzalez Kriegel 2015-10-29 12:29:58 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2015-10-29 12:32:39 UTC
I propose a followup to hide the currency
Comment 13 Bernardo Gonzalez Kriegel 2015-10-29 13:59:48 UTC Comment hidden (obsolete)
Comment 14 Bernardo Gonzalez Kriegel 2015-10-29 14:32:37 UTC
Sorry, but my test was bad, only tried with superlibrarian.

Marc, are you sure that needed permission check is

CAN_user_parameters_parameters_remaining_permissions

Can't found another instance of that.
Comment 15 Marc Véron 2015-10-29 15:50:47 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #14)
> Sorry, but my test was bad, only tried with superlibrarian.
> 
> Marc, are you sure that needed permission check is
> 
> CAN_user_parameters_parameters_remaining_permissions
> 
> Can't found another instance of that.

Bernardo,

I wanted the link not to display if the user is not able to edit the currencies table.

Who should be able to display and edit the currencies table:

When I go through the permissions, I think the following should be able to display and edit the table:

- superlibrarian (yes)
- paramters (yes)
  - manage_circ_rules (no, but see remark below)
  - parameters_remaining_permissions (yes)

Strange enough, with manage_circ_rules I can not only edit the circulation rules (what I would expect), but it seems that it allows to edit ALL of the parameters. I think this is a bug, what is yout opinion about?

My implementation tests for parameters_remaining_permissions (i.e.  CAN_user_parameters_parameters_remaining_permissions internally as I figured out). This triggers the 'mother'-permission for parameters as well.

I tested with two browsers, one as superlibrarian for setting the permission and one on an other computer with the non-superlibrarian. It worked well for me.

But anyway, it is not really a very important enhancement. I'd rather like to see the C4:Dates sign-offs for Bug 14870 'Delete C4/Dates.pm from System' and Bug 12072 'New dateformat dd.mm.yyyy' move forward.
Comment 16 Marc Véron 2015-12-01 15:32:24 UTC Comment hidden (obsolete)
Comment 17 Marc Véron 2015-12-01 15:38:31 UTC
Note regarding comment #15
(...)
Strange enough, with manage_circ_rules I can not only edit the circulation rules (what I would expect), but it seems that it allows to edit ALL of the parameters. I think this is a bug, what is yout opinion about?
(...)
This is not related to this bug. If it is an issue it should have it's own bug.
Comment 18 Marc Véron 2015-12-01 15:40:51 UTC
If there is no ogjection I propose to switch the status to signed-off.
Comment 19 Marc Véron 2015-12-08 17:09:08 UTC
Tested, still applies.

@Bernardo (comment #15): Yes, it is CAN_user_parameters_parameters_remaining_permissions
Comment 20 jmbroust 2015-12-17 09:19:55 UTC
Tested on sandbox 1 biblibre with superlibrarian permissions. Please tell in test plan if I should try with other permissions and which ones.Thx
Comment 21 sandboxes@biblibre.com 2015-12-17 15:11:14 UTC
Patch tested with a sandbox, by JM Broust <jean-manuel.broust@univ-lyon2.fr>
Comment 22 sandboxes@biblibre.com 2015-12-17 15:11:39 UTC Comment hidden (obsolete)
Comment 23 sandboxes@biblibre.com 2015-12-17 15:11:42 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-12-18 14:15:13 UTC
1/ Should not we update admin/aqplan.tt at the same time?

2/ Should not we put the "please" into the link, to ease the translations?
Comment 25 Marc Véron 2015-12-18 15:05:04 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-12-28 10:30:28 UTC
Created attachment 45995 [details] [review]
Bug 15049: Add warning about "No active currency" to Acquisitions start page

If no active currency is defined, Acquisitions does not work properly and software
can occur while receiving.
This patch adds the warning message about missing active currency to Home > Acquisitions

To test:
- Apply patch
- Go to Home > Administration > Currencies & Exchange Rates > Currencies
- Make sure that no active currency is set
- Go to Home > Acquisitions
- Verify that a warning displays:
  - If user has at least permission for parameters_remaining_perissions,
    the warning should display a link to Currencies and exchange rates (currency.pl)
  - If the user has no permission to edit the Currencies and exchange rates,
    no link is displayed.
- Set an active currency
- Veryfy that the warning no longer displays

(Amended to remove superfluous line / mv)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described. No errors

Signed-off-by: JM Broust <jean-manuel.broust@univ-lyon2.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2015-12-28 10:30:35 UTC
Created attachment 45996 [details] [review]
Bug 15049: (Signed off) Hide currency but preserv alert

This follow up hides currency, but the alert pops
if no currency is active.

To test:
1) Apply the patch
2) Unset any active currency
3) Alert must show on Acquisitons home
4) Set a currency active, no currency display on Acqui home
5) Check that currency IS displayed on /cgi-bin/koha/admin/aqbudgets.pl
or /cgi-bin/koha/admin/aqbudgetperiods.pl

Amended for better translation

Currency does / does not display as described in test plan.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: JM Broust <jean-manuel.broust@univ-lyon2.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2015-12-28 10:30:40 UTC
Created attachment 45997 [details] [review]
Bug 15049: (followup) Add warning about "No active currency"

Fix translatibiity issues and use include in admin/aqplan.tt as well.
See comment #24

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2015-12-28 10:30:45 UTC
Created attachment 45998 [details] [review]
Bug 15049: Simplify code in aqplan.tt

There was no need to duplicate this code in the template.
The only change with before this patch is that the "Currency = CUR"
and "Each cell contain both actual and estimated values." are now
displayed on 2 lines instead of 1.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Brendan Gallagher 2015-12-30 23:38:29 UTC
pushed to master - thanks