Bug 18002 - Aquisitions onboarding tool - to help users set up acquisitions the first time
Summary: Aquisitions onboarding tool - to help users set up acquisitions the first time
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 17.05
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Thibaud Guillot
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 18060
  Show dependency treegraph
 
Reported: 2017-01-27 00:12 UTC by Alex Buckley
Modified: 2023-02-28 15:31 UTC (History)
10 users (show)

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


Attachments
Bug 18002 - Added in modal box and message (outside of modal box) for when users first visit Acquisitions. (5.54 KB, patch)
2017-02-06 04:43 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Removed the Javascript preventing the display of currency and acquisition setup messages at the same time (3.28 KB, patch)
2017-02-06 04:43 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Removed errant warn and inline styling into an internal style sheet (5.49 KB, patch)
2017-02-07 20:33 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Added in a onboardinginclude.inc which will be the include displayed if the user has not set up their acquisitions or labels areas. (6.48 KB, patch)
2017-02-26 07:41 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Added add fund form to the onboardinginclude.inc file to be displayed after users have created a budget in the add budget form also in the onboardinginclude.inc (10.68 KB, patch)
2017-02-26 07:42 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Successfully working budget and fund form display order (9.18 KB, patch)
2017-02-26 07:42 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Successful insertion of budget and fund in the acquisitions onboarding tool followed by the acquisition home page being displayed. (6.39 KB, patch)
2017-02-26 07:42 UTC, Alex Buckley
Details | Diff | Splinter Review
BUg 18002 - Started to add label layout creation form to onboardinginclude.inc file (9.67 KB, patch)
2017-02-26 07:43 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Added a new include (onboardinginclude.inc) which displays a form for the user to create a budget. (5.99 KB, patch)
2017-02-26 08:39 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Added in a onboardinginclude.inc which will be the include displayed if the user has not set up their acquisitions or labels areas. (6.48 KB, patch)
2017-02-26 08:40 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Created a budget creation onboarding form to be displayed when the user first visits the acquisition area (5.03 KB, patch)
2017-02-26 08:57 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Created two includes onboardinginclude.inc and onboardinginclude2.inc which contain the budget and fund creation forms respectively (14.92 KB, patch)
2017-02-26 08:58 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Created a budget creation onboarding form to be displayed when the user first visits the acquisition area (5.03 KB, patch)
2017-02-26 09:05 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Added in a onboardinginclude.inc which will be the include displayed if the user has not set up their acquisitions or labels areas. (6.48 KB, patch)
2017-02-26 09:06 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Created two includes onboardinginclude.inc and onboardinginclude2.inc which contain the budget and fund creation forms respectively (14.92 KB, patch)
2017-02-26 09:07 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Tidied up the UI removing close button, and cancel links (4.32 KB, patch)
2017-02-26 09:54 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Implemented currency creation form which is the first file to be displayed if the user did not install sample currencies during installation. (26.83 KB, patch)
2017-04-09 12:05 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Reduced the widths (and removed set heights) of the acquisition onboarding includes (2.07 KB, patch)
2017-04-09 12:05 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Created a budget creation onboarding form to be displayed when the user first visits the acquisition area (23.52 KB, patch)
2017-05-04 22:41 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Created a budget creation onboarding form to be displayed when the user first visits the acquisition area (23.51 KB, patch)
2017-05-04 22:52 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Created a budget creation onboarding form to be displayed when the user first visits the acquisition area (23.51 KB, patch)
2017-05-11 10:16 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Created a budget creation onboarding form to be displayed when the user first visits the acquisition area (23.53 KB, text/plain)
2017-05-14 07:44 UTC, Alex Buckley
Details
Bug 18002 - Created an acquisition onboarding tool to be displayed when no currency, budget, or fund exists (23.51 KB, patch)
2017-06-29 23:42 UTC, Alex Buckley
Details | Diff | Splinter Review
Can not scroll down Acquisition onboarding wizard (54.89 KB, image/png)
2017-06-30 08:19 UTC, Marc Véron
Details
Bug 18002 - Created an acquisition onboarding tool to be displayed when no currency, budget, or fund exists (23.38 KB, patch)
2017-07-02 08:28 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002: Made rate hidden and removed the overflow attribute of onboardingModal (2.83 KB, patch)
2017-07-10 06:57 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002: Followup - Changes based on test feedback (16.16 KB, patch)
2017-08-21 23:11 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18002 - Created an acquisition onboarding tool to be displayed when no currency, budget, or fund exists (23.44 KB, patch)
2017-10-05 15:41 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18002: Made rate hidden and removed the overflow attribute of onboardingModal (2.89 KB, patch)
2017-10-05 15:42 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18002: Followup - Changes based on test feedback (16.22 KB, patch)
2017-10-05 15:42 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18002 - Created an acquisition onboarding tool to be displayed when no currency, budget, or fund exists (23.50 KB, patch)
2017-10-13 20:45 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 18002: Made rate hidden and removed the overflow attribute of onboardingModal (2.95 KB, patch)
2017-10-13 20:45 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 18002: Followup - Changes based on test feedback (16.28 KB, patch)
2017-10-13 20:45 UTC, Charles Farmer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2017-01-27 00:12:50 UTC
Add two additional interfaces to the onboarding tool to allow the user to create a vendor and budget before logging into Koha for the first time, thereby allowing users to get acquisition requirements set up.
Comment 1 Owen Leonard 2017-01-27 15:28:26 UTC
I think it would be nice to be able to skip this step since Koha can function just fine without the use of acquisitions.
Comment 2 Alex Buckley 2017-02-05 23:14:43 UTC
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.
Comment 3 Alex Buckley 2017-02-06 04:43:17 UTC Comment hidden (obsolete)
Comment 4 Alex Buckley 2017-02-06 04:43:39 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2017-02-06 05:01:32 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2017-02-06 05:02:55 UTC
I'm adding Owen for feedback on the inline styling, as he is much more experienced with making things pretty.
Comment 7 Alex Buckley 2017-02-06 05:21:25 UTC
(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?
Comment 8 Alex Buckley 2017-02-06 23:57:45 UTC
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.
Comment 9 Alex Buckley 2017-02-07 20:33:51 UTC Comment hidden (obsolete)
Comment 10 Alex Buckley 2017-02-07 20:48:09 UTC
(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.
Comment 11 Alex Buckley 2017-02-07 21:31:56 UTC
Still in progress
Comment 12 Alex Buckley 2017-02-23 20:18:37 UTC
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.
Comment 13 Alex Buckley 2017-02-26 07:41:07 UTC Comment hidden (obsolete)
Comment 14 Alex Buckley 2017-02-26 07:42:00 UTC Comment hidden (obsolete)
Comment 15 Alex Buckley 2017-02-26 07:42:31 UTC Comment hidden (obsolete)
Comment 16 Alex Buckley 2017-02-26 07:42:57 UTC Comment hidden (obsolete)
Comment 17 Alex Buckley 2017-02-26 07:43:28 UTC Comment hidden (obsolete)
Comment 18 Alex Buckley 2017-02-26 08:39:46 UTC Comment hidden (obsolete)
Comment 19 Alex Buckley 2017-02-26 08:40:13 UTC Comment hidden (obsolete)
Comment 20 Alex Buckley 2017-02-26 08:57:28 UTC Comment hidden (obsolete)
Comment 21 Alex Buckley 2017-02-26 08:58:19 UTC Comment hidden (obsolete)
Comment 22 Alex Buckley 2017-02-26 09:05:58 UTC Comment hidden (obsolete)
Comment 23 Alex Buckley 2017-02-26 09:06:30 UTC Comment hidden (obsolete)
Comment 24 Alex Buckley 2017-02-26 09:07:15 UTC Comment hidden (obsolete)
Comment 25 Alex Buckley 2017-02-26 09:54:48 UTC Comment hidden (obsolete)
Comment 26 Alex Buckley 2017-02-26 10:05:25 UTC
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
Comment 27 Alex Buckley 2017-04-09 12:05:34 UTC Comment hidden (obsolete)
Comment 28 Alex Buckley 2017-04-09 12:05:59 UTC Comment hidden (obsolete)
Comment 29 Alex Buckley 2017-04-09 12:46:09 UTC
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
Comment 30 Katrin Fischer 2017-05-04 06:34:15 UTC
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).
Comment 31 Katrin Fischer 2017-05-04 06:36:27 UTC
Patches eyeball well on first glance otherwise!
Comment 32 Alex Buckley 2017-05-04 22:41:28 UTC Comment hidden (obsolete)
Comment 33 Alex Buckley 2017-05-04 22:52:28 UTC Comment hidden (obsolete)
Comment 34 Marc Véron 2017-05-07 18:58:22 UTC
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
Comment 35 Alex Buckley 2017-05-11 10:16:45 UTC Comment hidden (obsolete)
Comment 36 Alex Buckley 2017-05-11 10:26:13 UTC
(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?
Comment 37 Alex Buckley 2017-05-14 07:44:44 UTC Comment hidden (obsolete)
Comment 38 Alex Buckley 2017-05-14 07:49:23 UTC
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.
Comment 39 Katrin Fischer 2017-05-14 08:34:47 UTC
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
Comment 40 Alex Buckley 2017-06-29 23:42:55 UTC Comment hidden (obsolete)
Comment 41 Marc Véron 2017-06-30 08:19:18 UTC
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.
Comment 42 Marc Véron 2017-06-30 08:24:10 UTC
The active currency must have an exchange rate of 1.
However, the wizard allows to enter a different value.
Comment 43 Marc Véron 2017-06-30 08:52:05 UTC
(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.
Comment 44 Alex Buckley 2017-06-30 08:56:19 UTC
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?
Comment 45 Marc Véron 2017-06-30 09:28:15 UTC
(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 :-)
Comment 46 Alex Buckley 2017-07-02 08:28:10 UTC Comment hidden (obsolete)
Comment 47 Marc Véron 2017-07-03 08:22:41 UTC
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?
Comment 48 Alex Buckley 2017-07-10 06:57:00 UTC Comment hidden (obsolete)
Comment 49 Katrin Fischer 2017-08-16 17:21:26 UTC
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.
Comment 50 Alex Buckley 2017-08-21 23:11:04 UTC Comment hidden (obsolete)
Comment 51 Alex Buckley 2017-08-21 23:20:21 UTC
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
Comment 52 Katrin Fischer 2017-08-22 05:29:06 UTC
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?
Comment 53 Alex Buckley 2017-10-03 00:16:30 UTC
(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
Comment 54 David Bourgault 2017-10-05 15:41:58 UTC Comment hidden (obsolete)
Comment 55 David Bourgault 2017-10-05 15:42:10 UTC Comment hidden (obsolete)
Comment 56 David Bourgault 2017-10-05 15:42:17 UTC Comment hidden (obsolete)
Comment 57 David Bourgault 2017-10-05 15:45:50 UTC
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.
Comment 58 Charles Farmer 2017-10-13 20:45:08 UTC
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>
Comment 59 Charles Farmer 2017-10-13 20:45:17 UTC
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>
Comment 60 Charles Farmer 2017-10-13 20:45:28 UTC
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>
Comment 61 Charles Farmer 2017-10-13 20:50:28 UTC
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.