Bug 18002

I think it would be nice to be able to skip this step since Koha can function just fine without the use of acquisitions.

Because users can go through the web installer and onboarding tool then get straight to work cataloguing items (which is likely to be the first thing they do) I suggest that the acquisition onboarding functionality (required by this bug report) be placed as a independent onboarding tool when the user first goes into the acquisition area to help the user to create a budget, fund, vendor and basket.

I'm adding Owen for feedback on the inline styling, as he is much more experienced with making things pretty.

(in reply to M.Tompsett in comment 5)

An interesting feature I have just noticed is that loop_budget is always returned as null so that the acquisitionsModal div is always displayed even if the user has created a budget.

On my system this is not specific to bug 18002, I also get it when testing on my master branch, as the warning telling the user to define a budget is displayed even if there are multiple budgets.

Do you also find the message appears even when you have created a budget?

Please disregard comment 7 as I was logging in as the database administrator and so the acquisitions message was being displayed constantly as I was not tied to a particular branch.

(in reply to M.Tompsett in comment 5)

In the most recent commit patch I have attached to this bug I removed the inline styling into an internal style sheet at the top of the acqui-home.tt file.

Additionally I removed the errant warn from the acqui-home.pl script.

Test plan:
1. From the main page of the staff client click go to More->Acquisitions (making sure there is no currency and budget and fund in the Koha database. Also make sure you are logged in as a superlibrarian user not the database administrator).

2. Notice the yellow warning messages: One telling the user to create a currency and the other informing the user to create a budget it doesn't say you have to create a fund as well which could be confusing for new users after they have created the budget).

3. Apply this bug. 

4. Refresh the Acquisitions page and you will see a message telling the user to specify a currency outside a div box, whilst the other is in a div box (with the Koha staff client light grey/blue background colour). The message inside the coloured div box contains numbered links clearly showing what the user must do to set up acquisitions. Each link opens in a new tab, saving the user from having to go back to the main acquisitions page constantly. 

5. Create a currency

6. Refresh the main Acquisitions page and notice that the currency notice has disappeared. 

7. Click the two links to create a budget and fund then refresh the main acquisitions page and the coloured div message box has disappeared because the user has set up the Acquisitions successfully.

Still in progress

To clarify how the patches for this bug report are going to help the users set up their acquisitions area below is a description of how the developing onboarding tool include will work:

I am developing the acquisitions onboarding tool as an include displayed by the the acqui-home.tt file. So it will appear to users as a div box on the acquisitions home page.

If the user is exploring Koha for the first time and visits the acquisition home page for the first time this onboarding tool box will appear but if the user does not want to set up Acquisitions yet they can simply use the Koha global navigation to navigate to a new page, they are not forced to use the acquisitions onboarding tool.

The acquisitions onboarding tool box will be displayed everytime the user visits the acquisitions home page until they use it to set up their acquisitions.

Test plan:

1. Drop and recreate your database
2. Restart memcached: sudo service memcached restart
3. Go through the web installer
4. Go to More->acquisitions
5. Notice the warnings telling you that you need to create a budget
6. Create a budget and return to the acqui-home.pl page
7. Notice the warning to create a budget is still being displayed
8. Apply the patches in this bug: git bz apply 18002
9. Redo steps 1,2,3,4
10. Notice there is a box containing a simplified form for you to create a budget is displayed on acqui-home.pl (no warning message being displayed)
11. Create a budget using this form and submit the form
12. Notice a simplified form containing a simplified form for you to create a fund is displayed on acqui-home.pl
13. Create a fund using this form and submit it
14. Notice the fund you have created is displayed in a table in the subsequent acqui-home.pl screen
15. Click on Budgets->Add fund
16. Create and save a fund 
17.Notice how this fund form in aqbudgets.pl redirects you back to aqbudgets.pl after you have saved the fund form

Test plan:

1. Restart memcached: sudo service memcached restart

2. Drop and recreate the database

3. Go through the web installer selecting all data values (except the basic currencies)

4. After the web installer is finished log into the staff interface using database admin credentials

5. Create a patron and assign superlibrarian permissions to them

6. Log out of staff interface and log back in as the new user you just created

7. Go to More->Acquisitions

8. Notice yellow/orange warning message telling you to create a budget

9. Apply the patches attached to this bug report

10. Repeat steps 1,2,3,4,5,6,7

11. Notice instead of warning messages a currency creation form appears along with explanation of what a currency is and why the user is being asked to fill this form out. Fill out and submit this form. Notice active checkbox is ticked by default

12. A budget creation form is displayed along with an explanation of budgets above it. Fill this form out and submit it. Notice active checkbox is ticked by default. 

13. A fund creation form appears (again with explanation text above it), fill it out and submit it

14. The coloured acquisition onboard box disappears and a table of created funds is displayed and should show the fund you just created


15. On the left of the screen below the text 'Administration' click on 'Funds' and delete the fund you just created

16. Click on More->Acquisitions and observe how the funds form is displayed rather than the currency or budget forms (because we already have a currency and budget in the database). Fill out the fund form and submit it.

17. Again you'll notice the fund table is displayed


18. On the left of the screen click on 'Budget' and delete your budget

19. Navigate to More->Acquisitions and observe how this time the budget form is displayed. Fill out this form and submit it. You should be redirected to the Budget Information page (aqbudgetperiods.pl) where you can click Actions->Add fund to add a fund.


20. Repeat steps 1,2,3,4,5,6,7 but making sure to install all sample data in the web installer including the basic currencies

21. Notice this time because you have installed basic currencies the first form to appear is the budget creation form

Starting to take a look at this (finally!). There are some complaints by the QA script:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/onboardinginclude.inc
   OK	  forbidden patterns
   FAIL	  git manipulation
		The file has been added and deleted in the same patchset
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/onboardinginclude3.inc
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   FAIL	  valid_template
		: filter not found

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/acqui-home.tt
   OK	  forbidden patterns
   OK	  git manipulation
   FAIL	  spelling
		 seperate  ==> separate
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgetperiods.tt
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 337)
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

As there have been no sign-offs yet, sqashing the patches should usually be alright if you want to (for the remove/add complaint).

Patches eyeball well on first glance otherwise!

Works nicely.

QA tools still have something to complain about:

FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/onboardinginclude3.inc
   FAIL	  valid_template
		: filter not found

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/acqui-home.tt
   FAIL	  spelling
		 seperate  ==> separate

(In reply to Marc Veron in comment 34)

Hi Marc

For some reason when I run the qa tool it only shows that all qa tests are successful. 

However I was able to fix up the error in acqui-home.tt easily which is in the rebased patch I have just attached to this bug report, but I am not sure how fix up the issue with the onboardinginclude3.tt could you please suggest how you would solve that issue?

I have fixed up the spelling mistake that was causing a qa error in acqui-home.tt and I have implemented [% USE Price %] at the top of the onboardinginclude3.inc file (thanks to Marc Veron for the suggestion), so both errors should now be fixed.

The two qa errors were not being thrown on my machine before making these changes and so I cannot guarantee that this patch has fixed the two errors.

Just tested, looks all green for me now:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ qa -c 1 -v 2
testing 1 commit(s) (applied to 830786a 'Bug 17461: Warn about plugins that ca')

Processing files before patches
|========================>| 12 / 12 (100.00%)
Processing files after patches
|========================>| 12 / 12 (100.00%)

 OK	C4/Budgets.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	acqui/acqui-home.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	admin/aqbudgetperiods.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	admin/aqbudgets.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	admin/currency.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/budgets-active-currency.inc
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/onboardinginclude1.inc
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/onboardinginclude2.inc
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/onboardinginclude3.inc
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/acqui-home.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgetperiods.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgets.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

Created attachment 64746 [details]
Can not scroll down Acquisition onboarding wizard

If the browser window is not high enough for the Acquisition onboarding wizard, you can not scroll down to make visible the whole modal dialog.

The active currency must have an exchange rate of 1.
However, the wizard allows to enter a different value.

(In reply to Marc Véron from comment #42)
> The active currency must have an exchange rate of 1.
> However, the wizard allows to enter a different value.

I think 
- the checkbox "Active" should be checked and read-only (maybe hidden?)
- the exchange rate should be set to 1 and read-only (maybe hidden?)

BTW I'm not sure, should not the currency be already defined in the onboarding process immediately after installing? - Since the active currency is important not only for acquisition, but should / will be used for fines etc. as well (at least the symbol), see Bug 4078.

Thanks for testing and the feedback Marc

At present the currency is not set in the onboarding process and will only be set if the user chooses to install currencies in the web installer.

However the creation of a currency in the onboarding too could be a good enhancement to the onboarding tool I will make a bug report for that if you like?

(In reply to Alex Buckley from comment #44)
> Thanks for testing and the feedback Marc
> 
> At present the currency is not set in the onboarding process and will only
> be set if the user chooses to install currencies in the web installer.
> 
> However the creation of a currency in the onboarding too could be a good
> enhancement to the onboarding tool I will make a bug report for that if you
> like?

+1 :-)

Hi Alex

It works great. Two small issues:

- The modal box now shows sliders (inactive), they are not necessary. I think it's in #onboardingModal (overflow) or something with the dimensions.
- The rate can not be changed, that is ok now, but users can enter oter values and try to save. They will need an explanation why the exchange rate can not be changed. Or maybe better hide the exchange rate?

Not sure if this one is valid, but the QA tools get upset about missing POD:

 FAIL	C4/Budgets.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   FAIL	  pod coverage
		POD is missing for GetHighestBudgetPeriod
   OK	  spelling
   OK	  valid

Some small stuff from code review:

1) We should not have unconditional warns:
Budgets.pm
+    warn my $data = $dbh->selectrow_array("SELECT MAX(budget_period_id) FROM aqbudgetperiods");

2) Small typo here:
<p>Start by completing this form to create a currency which <strike>you</strike> will be used to create your first budget and fund.<br>

3) Wrong link to the manual - always use http://koha-community... now.
<p>Start by completing this form to create a currency which you will be used to create your first budget and fund.<br>

4) Why do you comment out the hint? And why only on one of the 2 date fields?
+<!--<div class="hint">[% INCLUDE 'date-format.inc' %]</div>-->

5) Please include current test plans in commit message - otherwise it's often hard to spot the right one and it will help people looking at git later. :)

6) Some little spelling things:
Vendor Information page = vendor information page 
according to our capitalization rules. Also budget administration page etc.

Some more from testing, overall the feature works alright and is a nice addition. I am mostly concerned about the next point:

6) Wondering: Is there no way to skip the Onboarding process from the installer? Or the Acquisitions onboarding? 

People who have done this a lot of times before might want to skip the process. Because you want to add more currencies at the same time, add your whole fund structure etc. I would like it better if it was visible, but less invasive maybe? 

Also: Some libraries never set up acq, but need to be able to manage vendors in acquisitions. So it might be nice if the onboarding tool could be hidden if not needed.

Some ideas: 
- Don't show the onboarding tool if the user has only vendor_manage permission.
- Only show the onboarding tool, if the user has full acq permissions or is a superlibrarian.
- I am not sure how helpful the 'next steps' are. Maybe they would be better suited to go into the manual or help file? As it is the bug button always remains visible, even if you are a power user of acq.

Hi Katrin

Thanks for testing.

I have just attached a patch that fixes the issues you found. I have two queries:

1. At the moment in the new followup patch I have implemented a conditional so only superlibrarian users can see the acquisition onboarding tool. I tried using the haspermission() and the get_user_subpermissions() functions from the Auth.pm in the acqui-home.pl script however I found that when testing using these functions with a user that had full acq permissions these functions returned that the user did not have acq permissions and so I found myself at a roadblock.

Can you suggest any ideas for implementing checks on the users permissions so that users with full acq permissions and that are not superlibrarians can also view the acq onboarder?

2. I have run the QA test tool and as the GetBudgetHighestPeriod() function POD has now been added that passes, however I now get this error:
 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/onboardinginclude3.inc
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   FAIL	  valid_template
		Can't call method "config" on unblessed reference at Koha/Database.pm line 56.

I have been researching this error however I am rather confused about how to fix it, can you please point me in the right direction for fixing it? 

Cheers
Alex

Hi Alex, I was not aware that it only showed up for superlibrarians, that makes it 'less invasive' already :) Maybe we can leave it like that for now.

About the other error I am not sure - maybe check with some of the other devs on IRC?

(In reply to Katrin Fisher in comment 52)

I have just run the qa test tool again (after applying these patches on a clean up to date branch) and all files pass, so could you please test again?

vagrant@kohadevbox:kohaclone(test)$ sudo koha-shell kohadev
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ qa -c 3 -v 2
testing 3 commit(s) (applied to 7a60e52 'Bug 19276: Update the number of tests')

Processing files before patches
|========================>| 12 / 12 (100.00%)
Processing files after patches
|========================>| 12 / 12 (100.00%)

 OK	C4/Budgets.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	acqui/acqui-home.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	admin/aqbudgetperiods.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	admin/aqbudgets.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	admin/currency.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/budgets-active-currency.inc
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/onboardinginclude1.inc
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/onboardinginclude2.inc
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/onboardinginclude3.inc
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/acqui-home.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgetperiods.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgets.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

Tested as described in test plan, everything worked exactly as announced.

QA tool still warned of a fail in pod coverage for C4/Budgets.pm. Changing =head1 to =head2 fixed the problem on my end, and by looking at the other comments in the file seemed to be standard. I amended the signed-off/attached patch to reflect those minor (1 char) changes.

Created attachment 68108 [details] [review]
Bug 18002 - Created an acquisition onboarding tool to be displayed when no currency, budget, or fund exists

Implemented scrolling for acquisition onboarding tool.

Hidden the active checkbox for currencies and budget, and made exchange
rate readonly (displayed).

Same test plan as comment 29.

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Created attachment 68109 [details] [review]
Bug 18002: Made rate hidden and removed the overflow attribute of onboardingModal

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Created attachment 68110 [details] [review]
Bug 18002: Followup - Changes based on test feedback

In this patch the following changes have been made based on test
feedback:

* Added POD to GetHighestBudgetPeriod
* Removed unconditional warns in Budgets.pm
* Fixed typos and uncommented hint and date field
* Changed manual link

Made the acquisition onboarding tool less visible in the right hand side
of the screen rather than in the middle obscuring the rest of the
acquisition home page

Implemented conditional so this tool is only displayed when the user
is superlibrarian

Test plan:
1. Restart memcached: sudo service memcached restart

2. Drop and recreate the database

3. Go through the web installer selecting all data values (except the
basic currencies)

4. After the web installer is finished log into the staff interface
using database admin credentials

5. Create a patron and assign superlibrarian permissions to them

6. Log out of staff interface and log back in as the new user you just
created

7. Go to More->Acquisitions

8. Notice yellow/orange warning message telling you to create a budget

9. Apply the patches attached to this bug report

10. Repeat steps 1,2,3,4,5,6,7

11. Notice instead of warning messages a currency creation form appears
on the right hand side of the page along with explanation of what a currency is and why the user is being
asked to fill this form out. Fill out and submit this form.

12. A budget creation form is displayed along with an explanation of
budgets above it. Fill this form out and submit it.

13. A fund creation form appears (again with explanation text above it),
fill it out and submit it

14. The acquisition onboard tool disappears from the right of the screen and a table of
created funds is displayed and should show the fund you just created

15. On the left of the screen below the text 'Administration' click
on 'Funds' and delete the fund you just created

16. Click on More->Acquisitions and observe how the funds form is
displayed rather than the currency or budget forms (because we
already have a currency and budget in the database). Fill out the fund form and submit it.

17. Again you'll notice the fund table is displayed

18. On the left of the screen click on 'Budget' and delete your
budget

19. Navigate to More->Acquisitions and observe how this time the
budget form is displayed. Fill out this form and submit it. You
should be redirected to the Budget Information page
(aqbudgetperiods.pl) where you can click Actions->Add fund to add a
fund.

20. Repeat steps 1,2,3,4,5,6,7 but making sure to install all sample
data in the web installer including the basic currencies

21. Notice this time because you have installed basic currencies the
first form to appear is the budget creation form

22. Repeat step 15 and then create a patron that does not have the superlibrarian permissions
and log out and log back in.

23. Go to the Acquisitions module and notice that the yellow message
telling you that your administrator must define a budget is displayed

Sponsored by: Catalyst IT

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

The patch behaved as described in the test plan, and qa-test-tools showed no abnormalities since David's latest modification.

Signed off a second time.