Bug 17855

I apologize that some of the commit messages have duplicates of other commit messages inside of them making them very long. I am still learning git and so will fix this as soon as possible

I was hoping to test this, but got:
Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17855- In web installer step 3, automatically selected default data values are successfully being inserted into the database.
Applying: Bug 17855 - As with the last patch the updated web installers step 3 is showing bulletpoints for default data values which stops the user being able to unselect data neccisary to use Koha.
Applying: Bug 17855 - Implemented basic and advanced setup modes in the web installer avalable on the MARC flavour selection page of step 3.
Applying: Bug 17855 - Finished front end of onboarding tool which user is redirected to (after authentication) after finishing the web installer.
/home/vagrant/kohaclone/.git/rebase-apply/patch:391: trailing whitespace.

/home/vagrant/kohaclone/.git/rebase-apply/patch:402: trailing whitespace.
            <div>
/home/vagrant/kohaclone/.git/rebase-apply/patch:410: trailing whitespace.

/home/vagrant/kohaclone/.git/rebase-apply/patch:416: trailing whitespace.
[% ELSE %]
/home/vagrant/kohaclone/.git/rebase-apply/patch:470: trailing whitespace.
[% END %]
fatal: sha1 information is lacking or useless (installer/onboarding.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 17855 - Finished front end of onboarding tool which user is redirected to (after authentication) after finishing the web installer.
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-17855---Finished-front-end-of-onboarding-tool--YKACAI.patch

Perhaps you have forgotten to obsolete older patches on this bug as well?
Click Details, Edit details, check obsolete, and everything else is hopefully obvious. :)

M. Tompsett. Thank you of informing me of this. 

I have now gone through and obsoleted older patches including the patch that was causing the failure.

So try seeing if it will apply for you now.

(In reply to Alex Buckley from comment #16)
> M. Tompsett. Thank you of informing me of this. 
> 
> I have now gone through and obsoleted older patches including the patch that
> was causing the failure.
> 
> So try seeing if it will apply for you now.

Still fails. I'm thinking you are patching an old version of master. You may need to rebase.

$ git checkout master
$ git pull
$ git checkout -b bug_17855_retest1 origin/master
$ git bz apply 17855
-- This should fail.
$ git bz apply --abort
$ git checkout bug_17855
-- probably discover your are out some changes, not just these.
-- I'm assuming you generally name your branches bug_#####.
$ git rebase origin/master
$ git bz attach -e HEAD^^^^^^^^..HEAD
-- there are 8 ^'s. That should get all your patches.
$ git checkout bug_17855_retest1
-- probably some messages about being out of sync, doesn't matter.
$ git reset --hard origin/master
$ git bz apply 17855
-- hopefully this will work now.

I will go through and rebase shortly and then push up the commits again for testing.

(In reply to Alex Buckley from comment #23)
> This is a combination of 6 commits.

And it still doesn't apply. I attempted interactively to apply each of the patches individually just to master, and none of them do. I was thinking they are out of order, did you git pull AND git rebase origin/master.

Did you get the same 'fatal: sha1 information is lacking or useless' error stopping the patch apply?

(In reply to Alex Buckley from comment #25)
> Did you get the same 'fatal: sha1 information is lacking or useless' error
> stopping the patch apply?

$ git checkout -b bug_17855_round2 origin/master
$ git pull
$ git remote update -p
-- now I know this new branch is up to date.

-- this is the beginning of a looping block
$ git bz apply 17855
...
i
-- for interactive
-- There are 4 patches, comment out all but the first, the first time.
-- Comment out all but the second, the second time.
-- Comment out all but the third, the third time.
-- Comment out all but the fourth, the fourth time.
-- comments are lines beginning with #
:wq
-- I use vim, so whatever writes and quits is what you want
-- now you will see the massive failure to apply
$ git bz apply --abort
$ git reset --hard origin/master
-- loop 4 times from the beginning of the looping block

And yes, yes it was. Attempting all four patches individually means I know that you didn't accidentally screw up the order of the patches on this bug, which is way to easy to do.

Looking forward to testing something once it applies.

Seeing as it is your bug, Alex, I fixed the assignee. :)

(In reply to M. Tompsett in comment 27)
Thanks for doing that.

I gone through and pulled the master, rebased, and attached the patches.
I have just applied the bug in a new branch off the master and it applied successfully for me.

So it will hopefully work for you now.

Oh wow! Okay, I didn't understand the coolness of this until I started testing. It's going to take too long to test, and I need sleep, but so far so good.

Oh no! Spoke too soon. Seriously?! What is it with programmers taking away my symbols as part of passwords? Sorry, but I experienced this on another website (www.scene.ca) that I use as well, so the issue is a hot spot for me right now.

https://www.owasp.org/index.php/Password_special_characters
http://www.ibm.com/support/knowledgecenter/SSEQTP_8.5.5/com.ibm.websphere.base.doc/ae/csec_chars.html

I can't enter a password for Koha that I have used before. :(

(In reply to M. Tompsett from comment #32)
> Oh wow! Okay, I didn't understand the coolness of this until I started
> testing. It's going to take too long to test, and I need sleep, but so far
> so good.
> 
> Oh no! Spoke too soon. Seriously?! What is it with programmers taking away
> my symbols as part of passwords? Sorry, but I experienced this on another
> website (www.scene.ca) that I use as well, so the issue is a hot spot for me
> right now.
> 
> https://www.owasp.org/index.php/Password_special_characters
> http://www.ibm.com/support/knowledgecenter/SSEQTP_8.5.5/com.ibm.websphere.
> base.doc/ae/csec_chars.html
> 

This must be just a JavaScript regex Alex, I'd drop that part of the check.

> I can't enter a password for Koha that I have used before. :(
Mark what? Koha can't possibly know what you old password was. Do you mean you can't enter a password that is already in use? That's way outside the scope of this then.

Please remember too to keep chill when you are dealing with a first time commiter. Or we will never have 2nd time commiters :)

I have gone through and made the changes recommended in comments 33, 34, and 35, and attached the commit (see its name below) to this bug:

Bug 17855 - Removed html regex on patron password fields. Ran perltidy to indent and space code appropriately. Ran git bz configuration commands to remove whitespace. Removed unnecissary uses in the summary.pl script 

I have tested the bug in a clean branch and it applies successfully for me.

In reply to M. Tompsett from comment 38)
I removed that comment because I had already added that if statement to check if the creation of the library was successful, see line 27 of onboardingstep1.tt

I have now made all arrows -> throughout the onboarding tool, thank you for noticing that.

The reason I added the additional paths in the summary.tt is because I wanted to make the summary.tt consistent with the onboarding tool pages outlining how to create a new library, patron category, patron, item type and circulation rule. 

I have added back in the 'use CGI qw ( -utf8 );' into summary.pl

Last piece... better handling of when passwords mismatch. :)

I removed the userid javascript validation because it is unnecessary, due to the following reasons:

*There is already a html required="required" attribute on this input field
*And a html regex on this field 

Therefore javascript validation is not needed.

And thankyou for the note about the comments position, would you like  me to change that now or just remember it for future commits?

Created attachment 58895 [details] [review]
bug 17855 - Updated web installer finished

The onboarding tool has been implemented and integrated with the updated
web installer. The onboarding tool has the following functionality:
* After the user has finished the web installer they are automatically
redirected to the onboarding tool with no authentication in between
required
* Ability to create library, patron category, patron (set with
superlibrarian priviliages), item type and circulation rule.
* If the uaser installed sample libraries, patron categories and/or item
types then the step to create one in the onboarding tool will be skipped
and a skipping message will be displayed.
*HTML regxes on all text input fields in all forms in the onboarding
tool this prevents the user from writing in special characters
*Working date picker on the patron category form

Have moved all data values from mandatory to default.
Now all default data values are displayed with a bulletpoint rather than
a selectable checkbox stopping users from unticking neccisary data
values

Test plan:
1. Go through the web installer and onboarding tool with the same steps
as outlined in this below guide and check that the functionality
outlined in the guide exists in the patch:
https://koha-community.org/manual/installation/html/

 This is a combination of 67 commits.
Initial 3 HTML screens of the onboarding tool and
the perl onboarding.pl script

Note: The advanced setup option has not yet been
implemented, so what you will see is just the basic setup.

Altered web installer

Moved files from optional to mandatory. Edited
MARC screen so that MARC21 is selected by default.

Conflicts:
	koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt

Changed mandatory to default for MARC values

Changed a few file name descriptions to reduce
spacing in installer step 3 screen.

Changed the radiobuttons to bulletpoints for default settings

removed = sign:wq

Fixed bug in installer where a loop was created if
you're database structure needed to be updated.
Redirected this to the marcflavour screen instead
of logging straight into Koha and skipping over
the basic configuration steps.

Changed the bulletpoints to disabled checkboxes
which are checked by default

Backup of step3.tt

Successfully working insertion of the default
options which are shown as bulletpoints

Removed test text

Working insertion of default and optional options

Correctly selected default marc flavour of marc21

NOTE: Mark Tompsett merged the first two commits

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 58896 [details] [review]
Finished initial web installer and onboarding tool

https://bugs.koha-community.org/show_bug.cgi?id=17855

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 58897 [details] [review]
Bug 17855 - Removed html regex on patron password fields. Ran perltidy to indent and space code appropriately. Ran git bz configuration commands to remove whitespace. Removed unnecissary uses in the summary.pl script

These changes are based on feedback from testing.

Test plan:
1. Go through the web installer and onboarding tool as outlined in this
guide:
https://koha-community.org/manual/installation/html/

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 58898 [details] [review]
Bug 17855 - Second update following testing feedback

Made arrows in onboarding tool pages
-> for consistency. Additionally removed the 'OR'

Added the use CGI qw ( -utf8 );

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 58899 [details] [review]
Bug 17855 -Third update follwing testing feedback

Removed the second path to creating a library, patron category, patron,
item type, and circulation rule. In onboarding .tt files and summary.tt

Fixed typo in summary.tt

Made consistent single space before and after -> in all onboarding .tt
and summary.tt files.

Made single <br> tag before path to create library, patron category,
item type, and circulation rule in the onboarding .tt files (patron does
not need this as its layout is slightly different)

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 58900 [details] [review]
Bug 17855- Fourth update following testing feedback.

Changed button and link names in web installer and onboarding tool

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 58901 [details] [review]
Bug 17855 -Fifth update following tester feedback.

Implemented Javascript password check to determine if the values in the
'password' and 'confirm password' fields are consistent.

If passwords are not consistent a message informing the user is displayed and they only have
to change the password values rather than re-fill out the whole form.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 58902 [details] [review]
Bug 17855 - Fixed the errors from the koha qa test

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 58903 [details] [review]
Bug 17855 -Fifth update following tester feedback.

Implemented Javascript password check to determine if the values in the
'password' and 'confirm password' fields are consistent.

If passwords are not consistent a message informing the user is displayed and they only have
to change the password values rather than re-fill out the whole form.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 58904 [details] [review]
Bug 17855 - Fixed the errors from the koha qa test

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 58905 [details] [review]
Bug 17855 - Removed use:C4/Auth from the onboarding.pl

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Comment on attachment 58893 [details] [review]
Bug 17855- Sixth update following testing feedback.

Whitespace fixes generate empty patch from my install.

Created attachment 58906 [details] [review]
Bug 17855- Sixth update following testing feedback.

Created attachment 58907 [details] [review]
Bug 17855 - Removed use:C4/Auth from the onboarding.pl

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Squashed first two patches to solve koha qa test tool issue.
Ran basic/advanced unimarc/marc combinations.
Looks great.

I have retested this bug 17855 and it works as I intend it to do so

fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep3.tt).

Ok, fixed

Hi Alex,
This looks great, good job!

In my opinion, this is not ready yet to be pushed.
I have a few questions and remarks:

1/ I have tried the basic and advanced mode, the only difference I noticed is that checkboxes do not appear on the next screen.
If it is the only difference, I do not see the point of that parameter.

2/ Before your patch, the "Select all options" was a button. It was intended and is the correct element to use

3/ When all the default data are select, we get pages saying "there is nothing to do because the data are already inserted". I think it would be better to skip these screen and not to see them at all.

4/ The forms are not using the usual Koha style

5/ About the circulation rule, how did you pick the value to define?
For instance "Current on-site checkouts allowed" is not configurable and the default value becomes "unlimited". I do not think it is the desirable behavior.

6/ I would suggest to add an option to skip the onboarding Should be an option, we need a way to skip it

7/ At the end of the installer, the button "finish" has been replaced with "set koha version system preference". The wording sounds wrong.

Maybe we need to have 2 choices at that point:
- Set up Koha with the onboarding tool
- Finish (and skip the onboarding tool)

8/ Cardnumber:
From the template you use "minlength_cardnumber", but it's not pass from the script (I may missed something).
The behavior of the validation is not correct, a cardnumber can be composed with alpha chars.

I think it would be good to get Owen point of views about this change.

I agree that the inconsistent style is a problem. I wonder if we shouldn't simply include the default staff client CSS in addition to the installer-specific CSS.

I think if you have chosen the basic installation the "Select all options" button should be down next to the options which one can select.

I don't think there needs to be a "We're just going to set up a few more things..." page. We were told on the previous page that this step was coming.

I think the "Add a patron category" stage should make it clear that the category you define is going to be the one which the new administrator patron will have.

I think the "Create patron" stage should say something like "Create Koha administrator patron" so that it is very clear that the patron you create will will be a superlibrarian.

The "Create patron" stage wouldn't accept the card number I typed in. The validator said it wasn't a number. It would only let me increment the number that was prefilled.

The "Create a new Item type" should have some text explaining what an item type is, including the important point that it is what Koha uses to apply circulation rules.

Please run all the onboarding steps through an HTML validator. I use this addon in Firefox: http://users.skynet.be/mgueury/mozilla/

Capitalization is all over the place. Only the first letter of heading, label, etc. should be capitalized:

https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML4:_Upper_and_lower_cases_in_strings

I think this is going to be a really good addition to Koha. It just needs a few fixups!

Forgive the interjection, but I have commentary on some of this.

> 1/ I have tried the basic and advanced mode, the only difference I noticed
> is that checkboxes do not appear on the next screen.
> If it is the only difference, I do not see the point of that parameter.

http://irc.koha-community.org/koha/2017-01-12#i_1893213


> 2/ Before your patch, the "Select all options" was a button. It was intended
> and is the correct element to use

Sorry for not catching that.


> 3/ When all the default data are select, we get pages saying "there is
> nothing to do because the data are already inserted". I think it would be
> better to skip these screen and not to see them at all.

Sometimes it is educational to see what you are skipping. Not to mention it is easier to code that way. ;)


> 4/ The forms are not using the usual Koha style

Granted. I think Owen's commentary covers that. And given the Owen's expertise, some good suggestions in that regard.


> 5/ About the circulation rule, how did you pick the value to define?
> For instance "Current on-site checkouts allowed" is not configurable and the
> default value becomes "unlimited". I do not think it is the desirable
> behavior.

Is this a "keep it blank it still goes" problem?


> 6/ I would suggest to add an option to skip the onboarding Should be an
> option, we need a way to skip it

http://irc.koha-community.org/koha/2017-01-12#i_1893279
This would give Advanced more meaning. But rather than keep expanding the scope of this bug, let's push that to another bug.


> 7/ At the end of the installer, the button "finish" has been replaced with
> "set koha version system preference". The wording sounds wrong.

http://irc.koha-community.org/koha/2017-01-12#i_1893252
If you read the code, that is what actually seems to happen.


> Maybe we need to have 2 choices at that point:
> - Set up Koha with the onboarding tool
> - Finish (and skip the onboarding tool)

See back to the Basic/Advanced discussion.


> 8/ Cardnumber:
> From the template you use "minlength_cardnumber", but it's not pass from the
> script (I may missed something).
> The behavior of the validation is not correct, a cardnumber can be composed
> with alpha chars.

Oops. Sorry for missing that.

(In reply to Owen Leonard from comment #71)
> I think if you have chosen the basic installation the "Select all options"
> button should be down next to the options which one can select.

How about floating near the top (or bottom) like the save/cancel on... is it the system preference pages?


> I don't think there needs to be a "We're just going to set up a few more
> things..." page. We were told on the previous page that this step was coming.

There is system preference setting code run when that page is displayed.
This ties into the notion of advanced would let you skip onboarding completely or particular steps.


[SNIP Other great suggestions]
> I think this is going to be a really good addition to Koha. It just needs a
> few fixups!

Which is why I was working really hard on this sign off. I think it is a fantastic idea. :)

koha-tmpl/intranet-tmpl/prog/en/modules/installer/Backupstep3.tt

Is this file used for something?

(In reply to M. Tompsett from comment #73)
> (In reply to Owen Leonard from comment #71)
> > I think if you have chosen the basic installation the "Select all options"
> > button should be down next to the options which one can select.
> 
> How about floating near the top (or bottom) like the save/cancel on... is it
> the system preference pages?

No, that's not necessary. It's on the page where you select data to import. The "select all" control is only relevant, in this case, to the options which can be checked or unchecked. It should be displayed at the top of that section of the page.

> > I don't think there needs to be a "We're just going to set up a few more
> > things..." page.
> 
> There is system preference setting code run when that page is displayed.
> This ties into the notion of advanced would let you skip onboarding
> completely or particular steps.

I'm sorry, I don't understand this comment.

(In reply to Owen Leonard from comment #75)
> (In reply to M. Tompsett from comment #73)
> > There is system preference setting code run when that page is displayed.
> > This ties into the notion of advanced would let you skip onboarding
> > completely or particular steps.
> 
> I'm sorry, I don't understand this comment.

        <input type="hidden" name="step" value="3" />
        <input type="hidden" name="op" value="finish" />
"koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt"

    elsif ( $op && $op eq 'finish' ) {
        $installer->set_version_syspref();
...
        $template->param( "$op" => 1 );
    }
"installer/install.pl"

[% IF ( finish ) %]
    <h1>Congratulations, installation complete</h1>
    <p>If this page does not redirect in 5 seconds, click <a href="/">here</a>.</p>
[% END %]
"koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt"

-- which reminds me. Can someone change this to 10, or the 10 in the meta to 5?

Hi Jonathan,

5/ About the circulation rule, how did you pick the value to define?
For instance "Current on-site checkouts allowed" is not configurable and the default value becomes "unlimited". I do not think it is the desirable behavior.

Addressing this question, it is my feeling that overwhelming new users with the entire list of options for the circulation rule is unnecessary in the onboarding process. 

The goal of the tool is to get a brand new user of of an essentially empty Koha to a working, circulating Koha the first time they log in. A brand new user will not know what an on-site checkout is, or if/how they want to set up fines.

If you think about your most basic user, that person wants to log in, add a book, and then circulate it straight away, just to know that Koha is working. They will be delighted when it all works straight away, instead of binning the software as unusable or too hard. 

The rules are editable in the Circulation and fines rules interface, additional values can be added later. I'm opposed to adding more fields to the onboarding rule creation. I want people to get through the installer in 5 minutes with as little friction as possible. 

Cheers,
Liz

(In reply to Jonathan Druart in comment 70 and Owen Leonard in comment 71)

Today I have gone through and implemented your suggestions in comments 70 and 71. I will attach my commits to this bug tommorow. 

In the meantime I would like to clarify the onboarding tool a bit:

1. The reason for having the basic and advanced setups is that data values necessary for a fully-functioning Koha instance are selected and displayed with bulletpoints in the Basic setup because the basic setup is recommended for new, inexperienced users and it prevents them from unknowingly unticking an important data values. i.e. the basic setup protects new users from mistakes and lack of knowledge. 

Whereas the advanced setup (recommended for more experienced Koha users) gives them complete control over what is installed in their Koha database by displaying all data values with selectable checkboxes(as is available in the current web installer in Koha master).

2. I tried linking to the staff-global.css stylesheet on the onboarding tool pages today however it looked worse than the current onboarding tool styling. 

Therefore I propose leaving the current styling as it is, and then in the future a patch with the singular scope of improving the onboarding tool styling could be implemented. 

Another reason to leave the onboarding tool with the installer styling is because the onboarding tool is considered part of the installer process.

This is shown by the fact the login page is after the onboarding tool rather than the web installer, and the fact both are documented together (https://koha-community.org/manual/installation/html/) separate from the staff interface documentation. 

Thus the web installer and onboarding tool having the same styling is logical. 


3. I do not believe having a button at the end of the web installer to skip the onboarding tool is wise. 

The onboarding tools purpose is to help users (particularly new users) to get everything set up in their Koha database so they can log in and get straight to work. If you provide a button to skip the onboarding tool then new users may select it, then they will end up logging in as database administrator. If they havent installed optional sample data in the web installer then they will have to spend time creating sample libraries, patron types etc. and so defeating the purpose of having the onboarding tool in the first place.

If experienced Koha users want to skip the onboarding tool they could always open Koha in another tab, additionally they are not stuck with what they create in the onboarding tool they can simply modify or delete the onboarding records. 

Cheers
Alex Buckley

(In reply to Alex Buckley from comment #78)
> 1. ... i.e. the basic setup protects new users from
> mistakes and lack of knowledge.

JD, this is an enhancement, as advanced is the current default.
There is value to basic (enhanced protection against user error)
and there is value to advanced (some koha support providers may
have there own data and actually know what is needed).

Perhaps to push this along, leave the code in for basic/advanced, but set the UI to choose advanced behind the scenes. After all, advanced is the current behaviour.


> 2. [styling speel snip]

Okay, that's all fine and dandy, Alex, but there was a visual disconnect in styling and positioning of elements. The first time I ran the onboarding step the koha logo was weird to me. Still is. Compare it's position and styling with the previous steps, it doesn't match nicely.


> 3. I do not believe having a button at the end of the web installer to skip
> the onboarding tool is wise.
>
[SNIP]
> 
> If experienced Koha users want to skip the onboarding tool they could always
> open Koha in another tab, 

-- Or you could actually give them a GUI which let's them shoot themselves in the foot, because they chose Advanced. We are not supposed to be the nanny-state of data and configuration. But if they choose Basic, then clearly they want help and do not need a set of skip buttons.

That being said, this skipping issue seems better suited for a secondary bug, rather than hold this one up. And on that bug, make the basic/advanced visible again and add all the skipping stuff based on Advanced being selected.

That all said, I'm looking forward to seeing the issues in comment #71 addressed. Keep it up, Alex. :)

For the record, I disagree with hiding the basic/advanced step. It is a good enhancement and will help users.
I also disagree with removiing the skip, if we allow users to skip they will end up logging into Koha with the DB user which as we know can cause a lot of problems. 
For dev companies or experienced users they can just close the tab at that point.

(In reply to Chris Cormack from comment #80)
> For the record, I disagree with hiding the basic/advanced step. It is a good
> enhancement and will help users.
> I also disagree with removiing the skip, if we allow users to skip they will
> end up logging into Koha with the DB user which as we know can cause a lot
> of problems. 
> For dev companies or experienced users they can just close the tab at that
> point.

+1 to all of this. 

Enhancements or changes to the advanced setup should be addressed in further bugs. 

We have built the bike shed, we can paint it later.

Created attachment 59037 [details] [review]
Bug 17855 - Seventh update following testing feedback.

1. Moved the "select all options" button down to just above the selectable options
in the basic setup, and positioned at the top of the page for the advanced setup.

2. Removed the "there is nothing to do because the sample data is
already installed" page so if the user has install sample data in the web
installer then the step to create that object in the onboarding tool is
skipped altogether.

3. Removed the "We're just going to setup a few more things" page

4. Changed the title of the 'Create patron page'

5. Added clarifying information to the "Add a patron category" page

6. Added clarifying information to the "Create item type" page

7. Fixed the capitalization in the onboarding tool

8. Removed the BackupStep3.tt file which was un-used

9. Changed the redirect time displayed to user in step3.tt to 10

10. Changed the cardnumber on the create patron administrator page so
that it is only populated when the user installed sample patrons. Also
changed the regex so that it allows letters and number.

11. Used the HTMl validator to identify errors in the template and have
corrected all possible in the onboarding templates.

Because my internship is until the 10 Feb if we can sign off this onboarding tool soon,  then I can write additional patches to perfect the onboarding tool.

Created attachment 59051 [details] [review]
Bug 17855 - removing some errant warns

Just removing some warns that crept in during debugging.

(In reply to Alex Buckley from comment #78)
> Therefore I propose leaving the current styling as it is, and then in the
> future a patch with the singular scope of improving the onboarding tool
> styling could be implemented. 

I have an installer style overhaul in progress, so I will bring it up to date and try to apply it on top of these patches.

I think the regex on the patron card number is creating the side-effect that the form will not accept a card number with a zero in it.

(In reply to Owen Leonard from comment #86)
> I think the regex on the patron card number is creating the side-effect that
> the form will not accept a card number with a zero in it.

pattern="[A-Za-z1-9 ]+"
-- This is a problem for description in onboarding step 4, not just cardnumber is onboarding step 3.

pattern="[A-Za-z1-9][A-Za-z0-9 ]*"
-- This forces the first character to not be 0 or space, and have at least one character. I think this will work for cardnumber and description.

pattern="^[A-Za-z]{0,10}$"
Okay, but then the text in multiple places suggests 3, 1 or 2, 2 or 3. That 0 is wrong, unless empty is valid.

Maybe we could be less strict and allow any characters in the cardnumber? Removing accidentally entered whitespace at beginning and end might be enough.

(In reply to Katrin Fischer from comment #89)
> Maybe we could be less strict and allow any characters in the cardnumber?
> Removing accidentally entered whitespace at beginning and end might be
> enough.

I think it would be good to prevent a cardnumber of 0. :)

pattern="^[^0\s].*"
-- I believe this is starts with at least 1 character that is anything that is not 0 or whitespace.

(in reply to M.Topmsett in comment 90)

Yes you are correct that the regex ^[^0\s].* prevents inputting of 0 or a whitespace as the first character of the cardnumber, which is good. However I have tested it on the following website:
http://regexr.com/

And that regex will accept special characters as part of the cardnumber, which is obviously not what we want.

Also other than this regex issue was there any other reason that my bug was given 'Failed QA'?

(In reply to Alex Buckley from comment #91)

> Yes you are correct that the regex ^[^0\s].* prevents inputting of 0 or a
> whitespace as the first character of the cardnumber, which is good.

I marked it 'Failed QA' because I couldn't enter a card number which contained a 0 in any position. My library has thousands of library cards which start with leading zeros.

What kind of validation is typically done server-side on a card number? I don't think this form should be more strict than other places in Koha.

The idea of checking the data entered is good especially for the basic setup, but I think this might make it more complicate than it needs to be right now. 

I think it's a possibility that a library will try to use one of their existing cards there for testing, so we should not be too strict in order to not turn them away. Leading zeros is pretty common, also alphanumeric. We even had a library with ß in their regular cardnumbers once. I think Koha is pretty much ok with everything in there, as long as it's not too long (fits in the db field)

(In reply to Alex Buckley from comment #91)
> (in reply to M.Tompsett in comment 90)
> And that regex will accept special characters as part of the cardnumber,
> which is obviously not what we want.

"obviously not what we want" -- sounds a little dictatorial in how people generate their data. That's not what Koha is about. ;)

Remember the fuss I had because I couldn't use my previous Koha password due to pattern matching? You'll likely get the same fuss from other libraries about card numbers and descriptions.

If Katrin in comment #93 recommends removing the pattern matching to keep it simple, it probably is best. We have been rather lax in our data validation previously. It doesn't make sense to start.

(In reply to Owen Leonard in comment 92 and M.Tompsett in comment 94)

Apologies if it sounded dictatorial that is certainly never my intent. I was thinking that special characters should not be permitted due to the security implications of the user being able to input code into these input fields.

Regarding the server-side validation of entered cardnumbers in memberentry.pl if the user entered the form with a blank cardnumber field then the cardnumber is treated as null, as the below code shows:

298     # If the cardnumber is blank, treat it as null.
299     $newdata{'cardnumber'} = undef if $newdata{'cardnumber'} =~ /^\s*$/;

Then if there is an error from the checkcardnumber function the error is handed to the @errors array, which I have already implemented in my bug. 


In the template memberentrygen.tt the html inputs value attribute has a '| html' in it (e.g. <input name="cardnumber" value="cardnumber | html">, as I understand it this stops any special characters entered into the input field from being executed as code. Therefore I will implement this in the cardnumber field and remove the html regex so that the same level of data validation in the staff interface and onboarding tool exists. 

I personally do not believe we have been lax in our data validation, for the following reason:
All text input fields have had html regexs on them in the onboarding tool from the start, to prevent users from inputting special characters to fields.

Created attachment 59140 [details] [review]
Bug 17855 - Removed the html regex from the cardnumber field in the 'Create patron administrator' page of the onboarding tool. Added '|html' into the the value attribute of the cardnumber input field to filter the inputted cardnumber.

So now the same level of validation exists for validating the inputted
cardnumber in the staff interface and the onboarding tool.

Created attachment 59141 [details] [review]
Bug 17855 - Removed the unnecessary javascript cardnumber validation  code from onboardingstep3.tt

(In reply to Alex Buckley from comment #95)
> Apologies if it sounded dictatorial that is certainly never my intent. I was
> thinking that special characters should not be permitted due to the security
> implications of the user being able to input code into these input fields.

*sigh* No, you are forcing Romanized a-zA-Z. What about accented characters?


> I personally do not believe we have been lax in our data validation,

-- With respect to unusual characters like ñ. Now you are enforcing a-zA-Z.

I think we had some cross talk. I get wanting to clean inputs so you don't get the equivalent of SQL Injections and the like, but I don't get forcing a specific range that excludes diacritics. The description, which I hinted at in comment #94, found in onboarding step 4 may contain diacritics, may it not?

And what about the {0,10} issue I raised in comment #88 also in onboarding step 4?

And please, don't think my "it's in onboarding step 4" means it isn't elsewhere. Treat it as a class of problem, and look for it elsewhere too, because I haven't.

Sorry, setting to Failed QA.

(In reply to Alex Buckley from comment #95)
> In the template memberentrygen.tt the html inputs value attribute has a '|
> html' in it (e.g. <input name="cardnumber" value="cardnumber | html">, as I
> understand it this stops any special characters entered into the input field
> from being executed as code. 

Nope it's a protection to XSS attacks.

I do not really understand why this is Failed QA, Marc could you detail?
The check on the cardnumber has been removed.

There was far too much discussion about the cardnumber validation check, I'd suggest now to have one *at all* and deal with it on another bug report.

Created attachment 59174 [details] [review]
Bug 17855 -Removed HTML regexes from all fields on all screens of onboarding tool except branchcode, patron category code and item type code. Also ensured these three remaining regexes are all {1,10} for length as they are all required.

Comment on attachment 59174 [details] [review]
Bug 17855 -Removed HTML regexes from all fields on all screens of onboarding tool except branchcode, patron category code and item type code. Also ensured these three remaining regexes are all {1,10} for length as they are all required.

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

Thanks for removing the pattern= and tweaking some regexp's to be 1,10.
In some cases you removed the title= piece, which means no tooltip. Tooltips are great to have. I would have left them, just without the length specification stuff.

::: koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep1.tt
@@ +65,4 @@
>                   <ol>
>                       <li>
>                          <label for="branchcode" class="required">Library code: </label>
> +                        <input type="text"  pattern="[0-9A-Za-z]{1,10}" title="Please enter up to 10 letters and/or numbers" name="branchcode" id="branchcode" size="10" maxlength="10" value="[% library.branchcode %]" class="required" required="required" />

Never hurts to have a |html, just to make sure no injections could accidentally happen.

::: koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep3.tt
@@ +128,4 @@
>                      <h3>Patron identity</h3>
>                          <li>
>                              <label for="surname" class="required">Surname: </label>
> +                            <input type="text" id="surname" name="surname"  value="[% surname %]" class="required" required="required" />

Probably should have |html in the surname too.

@@ -156,5 @@
>                              <span class="required">Required</span>
>                          </li>
>                          <li>
>                              <label for="firstname" class="required">First name: </label>
> -                            <input  type="text"  pattern="[a-zA-Z ]+" title="Please only enter letters in the firstname field" name="firstname" id="firstname" size="20" value="[% firstname |html %]" class="required" required="required">

|html was okay.

@@ -214,5 @@
>                              <span class="required">Required</span>
>                          </li>
>                          <li>
>                              <label for="passwordlabel" class="required">Password: </label>
> -                            <input type="password" name="password" id="password" size="20" value="[% member.password |html %]" class="required" required="required">

This was probably okay to leave with the |html.

::: koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep4.tt
@@ -55,5 @@
>                      </li>
>  
>                      <li>
>                          <label for="description" class="required">Description: </label>
> -                        <input type="text" name="description" pattern="[A-Za-z1-9 ]+" title="Please only enter letters or numbers into this item type description" id="description" size="42" value="[% itemtype.description |html %]" class="required" required="required">

Probably okay to leave the |html.

Created attachment 59188 [details] [review]
Bug 17855 - Added |html and added in input title attribute to input fields where it existed previously

I notice that onboarding.pl has a lot of SQL operations. This is a violation of our coding guidelines:

https://wiki.koha-community.org/wiki/Coding_Guidelines#SQL8:_SQL_code_in_.pl_scripts

I also notice that there are quite a lot of "use" statements at the top of that script. Are they all really necessary?

(in reply to Owen Leonard in comment 104)

When writing this script I noticed that several of the administration scripts (itemtypes.pl (to create an item type)and smart-rules.pl ( to create a circulation rule)) had significant amounts of sql in them. 

Therefore what I am planning to do is if we can get this patch signed off I will have time to be able to go through and move the sql code from the onboarding.pl, itemtypes.pl, and smart-rules.pl.

Additionally in that second patch I would be able to go through and see which of the 'use's I can remove from onboarding.pl

I have written this bug report (bug 17944) which I will work on immediately after this bug (17855) is signed off. 

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944

Can we push this through first, and then discuss what to fix on it in another bug? I'm almost done my sign off testing. :)

Created attachment 59277 [details] [review]
[SIGNED-OFF] Bug 17855 - Other followups

- Removed the html regex from the cardnumber field in the
'Create patron administrator' page of the onboarding tool. Added
'|html' into the the value attribute of the cardnumber input
field to filter the inputted cardnumber.

So now the same level of validation exists for validating the
inputted cardnumber in the staff interface and the onboarding
tool.

- Removed the unnecessary javascript cardnumber
validation code from onboardingstep3.tt

- Removed HTML regexes from all fields on all screens
of onboarding tool except branchcode, patron category code
and item type code. Also ensured these three remaining regexes
are all {1,10} for length as they are all required.

- Added |html and added in input title attribute to
input fields where it existed previously

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 59279 [details] [review]
[SIGNED-OFF] bug 17855 - Updated web installer finished

The onboarding tool has been implemented and integrated with the updated
web installer. The onboarding tool has the following functionality:
* After the user has finished the web installer they are automatically
redirected to the onboarding tool with no authentication in between
required
* Ability to create library, patron category, patron (set with
superlibrarian priviliages), item type and circulation rule.
* If the uaser installed sample libraries, patron categories and/or item
types then the step to create one in the onboarding tool will be skipped
and a skipping message will be displayed.
*HTML regxes on all text input fields in all forms in the onboarding
tool this prevents the user from writing in special characters
*Working date picker on the patron category form

Have moved all data values from mandatory to default.
Now all default data values are displayed with a bulletpoint rather than
a selectable checkbox stopping users from unticking neccisary data
values

Test plan:
1. Go through the web installer and onboarding tool with the same steps
as outlined in this below guide and check that the functionality
outlined in the guide exists in the patch:
https://koha-community.org/manual/installation/html/

 This is a combination of 67 commits.
Initial 3 HTML screens of the onboarding tool and
the perl onboarding.pl script

Note: The advanced setup option has not yet been
implemented, so what you will see is just the basic setup.

Altered web installer

Moved files from optional to mandatory. Edited
MARC screen so that MARC21 is selected by default.

Changed mandatory to default for MARC values

Changed a few file name descriptions to reduce
spacing in installer step 3 screen.

Changed the radiobuttons to bulletpoints for default settings

removed = sign

Fixed bug in installer where a loop was created if
you're database structure needed to be updated.
Redirected this to the marcflavour screen instead
of logging straight into Koha and skipping over
the basic configuration steps.

Changed the bulletpoints to disabled checkboxes
which are checked by default

Backup of step3.tt

Successfully working insertion of the default
options which are shown as bulletpoints

Removed test text

Working insertion of default and optional options

Correctly selected default marc flavour of marc21

NOTE: Mark Tompsett merged several commits

- Removed html regex on patron password fields. Ran perltidy to
indent and space code appropriately. Ran git bz configuration
commands to remove whitespace. Removed unnecissary uses in
the summary.pl script

- Made arrows in onboarding tool pages
  -> for consistency. Additionally removed the 'OR'

- Added the use CGI qw ( -utf8 );

- Removed the second path to creating a library, patron
category, patron, item type, and circulation rule. In onboarding
.tt files and summary.tt

- Fixed typo in summary.tt

- Made consistent single space before and after -> in all
onboarding .tt and summary.tt files.

- Made single <br> tag before path to create library, patron
category, item type, and circulation rule in the onboarding
.tt files (patron does not need this as its layout is slightly
different)

- Changed button and link names in web installer and onboarding
tool

- Implemented Javascript password check to determine if the values
in the 'password' and 'confirm password' fields are consistent.

If passwords are not consistent a message informing the user
is displayed and they only have to change the password values
rather than re-fill out the whole form.

- Fixed the errors from the koha qa test

- Removed use:C4/Auth from the onboarding.pl

- Moved the "select all options" button down to just above
the selectable options in the basic setup, and positioned at
the top of the page for the advanced setup.

- Removed the "there is nothing to do because the sample data
is already installed" page so if the user has install sample
data in the web installer then the step to create that object
in the onboarding tool is skipped altogether.

- Removed the "We're just going to setup a few more things"
page

- Changed the title of the 'Create patron page'

- Added clarifying information to the "Add a patron category"
page

- Added clarifying information to the "Create item type" page

- Fixed the capitalization in the onboarding tool

- Removed the BackupStep3.tt file which was un-used

- Changed the redirect time displayed to user in step3.tt to 10

- Changed the cardnumber on the create patron administrator
page so that it is only populated when the user installed
sample patrons. Also changed the regex so that it allows
letters and number.

- Used the HTMl validator to identify errors in the template
and have corrected all possible in the onboarding templates.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 59280 [details] [review]
[SIGNED-OFF] Bug 17855 - removing some errant warns

Just removing some warns that crept in during debugging.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 59281 [details] [review]
[SIGNED-OFF] Bug 17855 - Other followups

- Removed the html regex from the cardnumber field in the
'Create patron administrator' page of the onboarding tool. Added
'|html' into the the value attribute of the cardnumber input
field to filter the inputted cardnumber.

So now the same level of validation exists for validating the
inputted cardnumber in the staff interface and the onboarding
tool.

- Removed the unnecessary javascript cardnumber
validation code from onboardingstep3.tt

- Removed HTML regexes from all fields on all screens
of onboarding tool except branchcode, patron category code
and item type code. Also ensured these three remaining regexes
are all {1,10} for length as they are all required.

- Added |html and added in input title attribute to
input fields where it existed previously

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

I tested this in conjunction with bug 17942. There are only a couple things on 17942 holding me back from signing off on that too.

Make sure to test by selecting but all sample data and not selecting it, so as to trigger the onboarding steps or to skip them nicely.

Comment on attachment 59279 [details] [review]
[SIGNED-OFF] bug 17855 - Updated web installer finished

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

Sorry I didn't catch this the first time.

::: installer/onboarding.pl
@@ +381,5 @@
> +            $newdata{categorycode}   = $input->param('categorycode_entry');
> +            $newdata{userid}         = $input->param('userid');
> +            $newdata{password}       = $input->param('password');
> +            $newdata{password2}      = $input->param('password2');
> +            $newdata{dateexpiry}     = '12/10/2016';

No no no! I was trying to test under Xenial Kohadevbox, and it blew up!
"DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect date value: '12/10/2016' for column 'dateexpiry' at row 1 at /home/vagrant/kohaclone/Koha/Object.pm line 120"
Dates that will be used directly with SQL queries need to be YYYY-MM-DD.
Plus hard coded?! YUCK!

Also missing: address and city need to default to something other than undef (onboarding.pl line 378ish -- see lack of address and city). Adding blank values should be good enough.

Also questionable: onshelfholds logic. The schema says bit, but "yes" and the other possible string values are not bit values. I hacked a !(!( blah )) to get it to keep running, but that's the wrong solution.

All problems are highlighted when testing on Xenial KohaDevBox.

(in reply to M.Tompsett in comments 113 and 114)

To fix the dateexpiry issue I can simply remove the dateexpiry altogether from the onboarding.pl script, because in the database it is set to null by default.

(In reply to Alex Buckley from comment #115)
> (in reply to M.Tompsett in comments 113 and 114)
> 
> To fix the dateexpiry issue I can simply remove the dateexpiry altogether
> from the onboarding.pl script, because in the database it is set to null by
> default.

But shouldn't it be set based on the patron category? I didn't check, but I think the patron category would determine what the date expiry should be.

(In reply to M.Tompsett in comment 116)

Yes I've checked and the dateexpiry is based on the patron category and so I am currently working on retrieving the selected patron category enrolement date to be used as the value for the dateexpiry variable.

In the commit I am working on currently I have set the address and city to blank values- so that is that issue fixed. 

Regarding the onshelfholds logic, I suggest we leave that to be fixed in bug 17955 which I am also currently working on as I am removing the SQL code which forms a big portion of the onshelfholds code in the onboarding.pl script and reworking it to use DBIx.

Created attachment 59628 [details] [review]
Bug 17855 - Implemented a retrieval of patron category enrolmentdate/enrolementdateperiod to set an appropriate dateexpiry for patron accounts created with the onboarding tool.

Set address and city variables to "" in onboarding.pl

Fixed the creation/filling of the $firstpassword, $secondpassword and
$op variables in the onboarding.pl so they don't cause uninitialised
variable errors.

Comment on attachment 59628 [details] [review]
Bug 17855 - Implemented a retrieval of patron category enrolmentdate/enrolementdateperiod to set an appropriate dateexpiry for patron accounts created with the onboarding tool.

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

This is my quick eyeball. I'll test later.

::: installer/onboarding.pl
@@ +92,4 @@
>  );
>  
>  #Store the value of the template input name='op' in the variable $op so we can check if the user has pressed the button with the name="op" and value="finish" meaning the user has finished the onboarding tool.
> +my $op = $input->param('op') || "";

For future reference q{} is perlcritic level 1 friendly. Single quotes are also more perlcritic friendly.

@@ +282,5 @@
>      $template->param( 'categories' => $categories, );
>  
> +
> +
> +

Hmmm... excess space? I'm not going to fail over this. I'll let QA fix it.

@@ +387,5 @@
> +            $newdata{city}           = "";
> +
> +#Determine the dateexpiry of the patron based on the patron category it is created from
> +            my $patron_category = Koha::Patron::Categories->find( $newdata{categorycode} );
> +            $newdata{dateexpiry} = $patron_category->get_expiry_date( $newdata{dateenrolled} );

It's so pretty!

(In reply to Alex Buckley from comment #117)
> Regarding the onshelfholds logic, I suggest we leave that to be fixed in bug
> 17955 which I am also currently working on as I am removing the SQL code
> which forms a big portion of the onshelfholds code in the onboarding.pl
> script and reworking it to use DBIx.

I'm wondering if the wrong thing is being accessed.
I was reading updatedatabase.pl around 3.19.00.017's update.
The drop down list you give makes me think of the variable opac items holds.
I'd personally prefer the functionality clean up (does it actually generate valid values to go in the right place) here. You can do the code clean up (DBIx, making the gui pretty, etc) there.

Sorry, I just got my xenial up again to retest and ran out of time. I'll aim for 12 hours from now.

(in reply to M.Tompsett in comment 120)

I believe we are starting to scope creep. The purpose of the create circulation rule is simply to display the library, patron category and item type the user created/installed (in drop-down boxes) and then the user can choose 'All' or a specific value and the circulation rule is assigned accordingly. Thus at present the create circulation rule step does what it's supposed to do successfully. 

Regarding the onshelfholds logic I fixed it up in the bug 17955 (which needs signoff), because some of the sql queries were inserting into the onshelfholds column of the default_branch_circ_rules table (see below) (there is no such column in this table) so I changed the insert to input values into the issuingrules table. 

628             my $sth_insert = $dbh->prepare(
629                 "INSERT INTO default_branch_circ_rules
630                                         (branchcode, onshelfholds)
631                                         VALUES (?, ?)"
632             );

If you want to have this signed off, I don't want to see:

Software error:

DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect integer value: 'anyunavailable' for column 'onshelfholds' at row 1 at /home/mtompset/kohaclone/Koha/Object.pm line 120
For help, please send mail to the webmaster (webmaster@ubuntu), giving this error message and the time and date of the error.

When I attempt a Xenial install.
The other possibility is completely remove onshelfholds and just default it to something.

Created attachment 59703 [details] [review]
Bug 17855: Follow up - Fix onshelfholds

Created attachment 59705 [details] [review]
Bug 17955 - Removed unnecessary use pragmas in the onboarding.pl script

Replaced all sql code in onboarding.pl script with DBIx

Changed the logic of the onshelfholds if statement in onboarding.pl

Created attachment 59706 [details] [review]
Bug 17955 - Removed unnecessary use pragmas in the onboarding.pl script

Replaced all sql code in onboarding.pl script with DBIx

Changed the logic of the onshelfholds if statement in onboarding.pl

https://bugs.koha-community.org/show_bug.cgi?id=17855
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 59707 [details] [review]
bug 17855 - Updated web installer finished

The onboarding tool has been implemented and integrated with the updated
web installer. The onboarding tool has the following functionality:
* After the user has finished the web installer they are automatically
redirected to the onboarding tool with no authentication in between
required
* Ability to create library, patron category, patron (set with
superlibrarian priviliages), item type and circulation rule.
* If the uaser installed sample libraries, patron categories and/or item
types then the step to create one in the onboarding tool will be skipped
and a skipping message will be displayed.
*HTML regxes on all text input fields in all forms in the onboarding
tool this prevents the user from writing in special characters
*Working date picker on the patron category form

Have moved all data values from mandatory to default.
Now all default data values are displayed with a bulletpoint rather than
a selectable checkbox stopping users from unticking neccisary data
values

Test plan:
1. Go through the web installer and onboarding tool with the same steps
as outlined in this below guide and check that the functionality
outlined in the guide exists in the patch:
https://koha-community.org/manual/installation/html/

 This is a combination of 67 commits.
Initial 3 HTML screens of the onboarding tool and
the perl onboarding.pl script

Note: The advanced setup option has not yet been
implemented, so what you will see is just the basic setup.

Altered web installer

Moved files from optional to mandatory. Edited
MARC screen so that MARC21 is selected by default.

Changed mandatory to default for MARC values

Changed a few file name descriptions to reduce
spacing in installer step 3 screen.

Changed the radiobuttons to bulletpoints for default settings

removed = sign

Fixed bug in installer where a loop was created if
you're database structure needed to be updated.
Redirected this to the marcflavour screen instead
of logging straight into Koha and skipping over
the basic configuration steps.

Changed the bulletpoints to disabled checkboxes
which are checked by default

Backup of step3.tt

Successfully working insertion of the default
options which are shown as bulletpoints

Removed test text

Working insertion of default and optional options

Correctly selected default marc flavour of marc21

NOTE: Mark Tompsett merged several commits

- Removed html regex on patron password fields. Ran perltidy to
indent and space code appropriately. Ran git bz configuration
commands to remove whitespace. Removed unnecissary uses in
the summary.pl script

- Made arrows in onboarding tool pages
  -> for consistency. Additionally removed the 'OR'

- Added the use CGI qw ( -utf8 );

- Removed the second path to creating a library, patron
category, patron, item type, and circulation rule. In onboarding
.tt files and summary.tt

- Fixed typo in summary.tt

- Made consistent single space before and after -> in all
onboarding .tt and summary.tt files.

- Made single <br> tag before path to create library, patron
category, item type, and circulation rule in the onboarding
.tt files (patron does not need this as its layout is slightly
different)

- Changed button and link names in web installer and onboarding
tool

- Implemented Javascript password check to determine if the values
in the 'password' and 'confirm password' fields are consistent.

If passwords are not consistent a message informing the user
is displayed and they only have to change the password values
rather than re-fill out the whole form.

- Fixed the errors from the koha qa test

- Removed use:C4/Auth from the onboarding.pl

- Moved the "select all options" button down to just above
the selectable options in the basic setup, and positioned at
the top of the page for the advanced setup.

- Removed the "there is nothing to do because the sample data
is already installed" page so if the user has install sample
data in the web installer then the step to create that object
in the onboarding tool is skipped altogether.

- Removed the "We're just going to setup a few more things"
page

- Changed the title of the 'Create patron page'

- Added clarifying information to the "Add a patron category"
page

- Added clarifying information to the "Create item type" page

- Fixed the capitalization in the onboarding tool

- Removed the BackupStep3.tt file which was un-used

- Changed the redirect time displayed to user in step3.tt to 10

- Changed the cardnumber on the create patron administrator
page so that it is only populated when the user installed
sample patrons. Also changed the regex so that it allows
letters and number.

- Used the HTMl validator to identify errors in the template
and have corrected all possible in the onboarding templates.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 59708 [details] [review]
Bug 17855: Follow up - Fix onshelfholds

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 59709 [details] [review]
Bug 17955 - Removed unnecessary use pragmas in the onboarding.pl script

Replaced all sql code in onboarding.pl script with DBIx

Changed the logic of the onshelfholds if statement in onboarding.pl

https://bugs.koha-community.org/show_bug.cgi?id=17855
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 59710 [details] [review]
Bug 17855 - Implemented a retrieval of patron category enrolmentdate/enrolementdateperiod to set an appropriate dateexpiry for patron accounts created with the onboarding tool.

Set address and city variables to "" in onboarding.pl

Fixed the creation/filling of the $firstpassword, $secondpassword and
$op variables in the onboarding.pl so they don't cause uninitialised
variable errors.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 59711 [details] [review]
Bug 17855 - Follow up: code clean up

Removed unnecessary use pragmas in the onboarding.pl script

Replaced all sql code in onboarding.pl script with DBIx

Changed the logic of the onshelfholds if statement in onboarding.pl

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

NOTE: Initially came from 17955
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 59712 [details] [review]
Bug 17855: Follow up - Fix onshelfholds

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

*** Bug 17955 has been marked as a duplicate of this bug. ***

Alex,

Few more remarks:
1/ There is something wrong with the sql sample files for translated languages.
The mandatory and optional files for the other languages have to be moved to the default directory as well.
We will have to be meticulous to avoid regressions.

2/ You should also adapt xt/sample_notices.t, tests are failing after your patches

3/ Could you double check the module you use in installer/onboarding.pl, some are uselessly used.

4/ Double check the copyright lines you have added (+# Copyright (C) YEAR  YOURNAME-OR-YOUREMPLOYER)

5/ You copied some existing javascript functions to onboardingstep2.tt. Do not duplicate existing code.
You should also have a look at this template, the indentation in the javascript portion is weird.

6/ Same as 5 for onboardingstep3.tt

7/ summary.tt - you should not use <br/> tag, our translation tool does not handle sentences when they are split by html tag. That make the job really hard (and sometimes impossible) for translators.

8/ Copyright of summary.pl sounds totally wrong.

9/ Is this Summary info really useful? To me it looks like it should be moved to the help part. I am not sure it worth a new entry in the "More" menu.

(In reply to Jonathan Druart from comment #133)

> 9/ Is this Summary info really useful? To me it looks like it should be
> moved to the help part. I am not sure it worth a new entry in the "More"
> menu.

I agree. I wonder if we could add a persistent shortcut of some kind to the global help system? I suggest this feature be discussed in a separate bug.

(In reply to Owen Leonard from comment #134)
> I wonder if we could add a persistent shortcut of some kind to the
> global help system? I suggest this feature be discussed in a separate bug.

Yes, points 7-9 are valid issues, but probably removing summary from this, and discussing perhaps a simplified basics help page on another bug might be in order.

Created attachment 59742 [details] [review]
Bug 17855: Follow-up on broken xt test

Apologies for missing that. Here is a patch for point 2 in
comment #133.

TEST PLAN
---------
1) apply all except this patch
2) prove xt
   -- a failure
3) apply this patch
4) prove xt
   -- no failures
5) prove t
   -- only prices fails on master, and is unrelated to this patch.
6) run koha qa test tools

(In reply to M. Tompsett from comment #136)
> Created attachment 59742 [details] [review] [review]
> Bug 17855: Follow-up on broken xt test
> 
> Apologies for missing that. Here is a patch for point 2 in
> comment #133.

Though, this will need to be modified further to address point 1 in comment #133.

(in reply to Jonathan Druart and Owen Leonard in comment 133 and 134)

Just to clarify a few of your points:

Re: point 5: "5/ You copied some existing javascript functions to onboardingstep2.tt. Do not
duplicate existing code." 
Do you want me to remove the JavaScript functions and or will fixing the indentation be sufficient to pass QA?

Re: point 9: What is the final decision on summary info. To get this signed off and through QA are both of you happy for me to remove summary.pl and summary.tt. And they can be added in a separate bug?

(in reply to Jonathan Druart and M.Tompsett in comment 133, 136 and 137 and 17855)

As I am still learning Koha can I please have a more specific explanation of the issue(s) which you addressed in points 1 and 2?

And possible solutions thereof

(In reply to Alex Buckley from comment #139)
> (in reply to Jonathan Druart and M.Tompsett in comment 133, 136 and 137 and
> 17855)
> 
> As I am still learning Koha can I please have a more specific explanation of
> the issue(s) which you addressed in points 1 and 2?
> 
> And possible solutions thereof

Non-English template files are generated from the English ones. When you moved the English ones, did you check (sorry, I forgot to check) if you could generate another language and run through the onboarding tool? This is not necessarily related, but I thought I'd mention it. Too often, English speakers (myself included) fail to test other languages as well. In fact, we need to test for all the languages that we do have.

Also, you've modified the mandatory/optional directory structure and locations of the English files. It is probably a good idea to keep the structures aligned across all languages. Again, we'll need to test for all the languages that we do have.

My follow up on the broken xt test makes the broken test pass. However, any changes as triggered above, may required further changes to the same test. :)

(in reply to M.Tompsett in comment 133 and M.Tompsett in comment 140)

Yes thanks, I also asked elsewhere.

I am currently working through the issues in comment 133

(In reply to Alex Buckley from comment #138)

> Re: point 9: What is the final decision on summary info. To get this signed
> off and through QA are both of you happy for me to remove summary.pl and
> summary.tt. And they can be added in a separate bug?

I think this is the right thing to do.

In the commit I have just finished (soon to be attached to bug 17855) the following points have been addressed:

1/ I have made a new bug 18039 (assigned to myself but if someone else wants to take it on they are more than welcome to). This bug requires the movement of the mandatory and optional files of other languages into default directories. Because this is a complex job it is going to take significant testing and so I have placed the work for this in a new bug report to keep bug 17855 moving forwards.


2/ M.Tompsetts patch has addressed this issue as the xt test now returns a successful result of:
xt/sample_notices.t ......... ok    
xt/single_quotes.t .......... ok   
xt/tt_valid.t ............... ok   
xt/verify-debian-docbook.t .. ok    
xt/yaml_valid.t ............. ok     
All tests successful.
Files=5, Tests=90,  5 wallclock secs ( 0.05 usr  0.00 sys +  4.19 cusr  0.06 csys =  4.30 CPU)
Result: PASS


3/ I assume you mean the use pragmas at the top of the onboarding tool. If this is the case in my most recent commit I have removed 7 more unused use pragmas.

4. I've fixed the copyright notice

5. I now include the js from categories.js, removing the duplicate js code in onboardingstep2.tt except for the assignment of the message variables and the enrollment_period function which is different for the onboarding tool to that in the categories.js

6. I've fixed the indentation of js in onboardingstep3.tt. 
Due to my password_match js function being slightly different to the one in members.js I have left that in onboardingstep3.tt additionally I have left the form validation js code in onboardingstep3.tt as there are different fields to validate than the memberentrygen.tt. 
Making the js form validation in members.js work for both the onboardingstep3.tt and memberentrygen.tt can be done in a later bug as a lot of testing would be required.

7. I have now removed summary.tt which can be added in a later bug 

8. Removed summary.pl same reason as above

9. Removed summary info same reason as above

Created attachment 59826 [details] [review]
Bug 17855: Follow-up on broken xt test

Apologies for missing that. Here is a patch for point 2 in
comment #133.

TEST PLAN
---------
1) apply all except this patch
2) prove xt
   -- a failure
3) apply this patch
4) prove xt
   -- no failures
5) prove t
   -- only prices fails on master, and is unrelated to this patch.
6) run koha qa test tools

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 59830 [details] [review]
Bug 17855 - Follow up on copyright text, removal of summary info, removal of duplicate js and improved js indentation, removed useless use pragmas

Tets plan: Same as outlined in previous comments:
Go to and go through the web installer and onboarding tool:
https://koha-community.org/manual/installation/html/

Comment on attachment 59830 [details] [review]
Bug 17855 - Follow up on copyright text, removal of summary info, removal of duplicate js and improved js indentation, removed useless use pragmas

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

Quick eyeball. If it behaves as before, I'll sign off.

::: installer/onboarding.pl
@@ +2,4 @@
>  
>  # This file is part of Koha.
>  #
> +# Copyright (C) 2017 Catalyst IT

Addresses point 4 in comment #133.

@@ -30,5 @@
> -use C4::Members::Attributes;
> -use C4::Members::AttributeTypes;
> -use C4::Log;
> -use Koha::AuthorisedValues;
> -use Koha::Patron::Debarments;

Addresses point 3 in comment #133.

@@ -39,5 @@
>  use Koha::DateUtils;
>  use Koha::Patron::Categories;
>  use Koha::Patron::Category;
> -use Koha::Patron::HouseboundRole;
> -use Koha::Patron::HouseboundRoles;

Addresses point 3 in comment #133.

@@ -44,4 @@
>  use Koha::ItemTypes;
>  use Koha::IssuingRule;
>  use Koha::IssuingRules;
> -use Data::Dumper;

Addresses point 3 in comment #133.

::: koha-tmpl/intranet-tmpl/prog/en/includes/header.inc
@@ -42,5 @@
>                      <li><a href="/cgi-bin/koha/admin/admin-home.pl">Administration</a></li>
>                      [% END %]
>                      <li><a href="/cgi-bin/koha/about.pl">About Koha</a></li>
> -                    <li><a href="/cgi-bin/koha/summary.pl">Summary</a></li>
> -

Removal deals with point 7-9 in comment #133.

::: koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep2.tt
@@ +5,5 @@
>  <title> Add a patron category</title>
>  [% INCLUDE 'installer-doc-head-close.inc' %]
>  [% INCLUDE 'calendar.inc' %]
> +[% INCLUDE 'js_includes.inc' %]
> +[% INCLUDE 'datatables.inc' %]

Good simplification to address point 5 in comment #133.

@@ +20,4 @@
>        } else {
>               return true;
>        }
> +    }, MSG_ONE_ENROLLMENTPERIOD

Because of using included javascripts, variable name changed. Related to point 5 in comment #133.

@@ +25,2 @@
>  </script>
> +<script type="text/javascript" src="[% themelang %]/js/categories.js"></script>

Addresses point 5 in comment #133.

::: koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep3.tt
@@ +7,4 @@
>  [% INCLUDE 'installer-doc-head-close.inc' %]
>  [% INCLUDE 'calendar.inc' %]
>  [% INCLUDE 'datatables.inc' %]
> +[% INCLUDE 'js_includes.inc' %]

Point 6 in comment #133.

@@ -21,5 @@
> -
> -
> -<script type="text/javascript">
> -jQuery.validator.addMethod( "password_match", function(value,element){
> -        var MSG_PASSWORD_MISMATCH = (_("The passwords entered do not match"));

I like the simplification, but I think this shorter message is better, rather than the longer one above. But, not a Failed QA, in my mind.

@@ +174,4 @@
>                      <ol>
>                      <h3>OPAC/Staff Login</h3>
>                          <li>
> +                            <input type="hidden" name="BorrowerMandatoryField" value = "[% BorrowerMandatoryField %]" />

Not sure if the spacing will matter, but good HTML validation improvement by including the double quotes.

Comment on attachment 59830 [details] [review]
Bug 17855 - Follow up on copyright text, removal of summary info, removal of duplicate js and improved js indentation, removed useless use pragmas

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

Found more point 3 trimming needed.

::: installer/onboarding.pl
@@ +27,1 @@
>  use C4::Koha;

Digest::MD5, Encoding and C4::Koha don't need to be here. :(

@@ +30,2 @@
>  use Koha::Patrons;
>  use Koha::Items;

Koha::Items is not necessary either.

Created attachment 59840 [details] [review]
Bug 17855 - Follow up on copyright text, removal of summary info, removal of duplicate js and improved js indentation, removed useless use pragmas

Tets plan: Same as outlined in previous comments:
Go to and go through the web installer and onboarding tool:
https://koha-community.org/manual/installation/html/

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 59841 [details] [review]
Bug 17855: Follow up on more trimming

Still needed to remove some libraries to address point 3 in comment #133.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 59842 [details] [review]
Bug 17855: Followup silence a warning when there are no imported patrons.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Created attachment 59856 [details] [review]
Bug 17855: Follow up on more trimming

Still needed to remove some libraries to address point 3 in comment #133.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 59857 [details] [review]
Bug 17855: Followup silence a warning when there are no imported patrons.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

I am sorry but this patchset can go without bug 18039.

Created attachment 60092 [details] [review]
Bug 17855: Onboarding tool

(In reply to Jonathan Druart from comment #154)
> Created attachment 60092 [details] [review] [review]
> Bug 17855: Onboarding tool

So... I hope I did not break everything.
I extracted the installer files from this patchset and move them to bug 18039.
Then I have squashed all the patches together and here is the resulting squashed patch. Please apply it alone!

I have tried to move the installer files for the translated languages. Take a look at bug 18039.
That sounds a quite big work and I am not confident with the patchset, we will need to test it deeply.

So the plan would be to
1/ Apply patches from 18039
2/ Apply the squashed patch
3/ Test the different languages (how to make sure nothing has been missed??)
4/ Pushed everything altogether

Does that make sense?

Note sure what's is going on, I get 
  Template process failed: file error - installer-strings.inc: not found at /home/vagrant/kohaclone/C4/Templates.pm line 121.

on /installer/onboarding.pl?step=2

I do not manage to know where it comes from.

(With and without my changes)

(in reply to Jonathan Druart in comment 155)
At this stage yes that does all make sense thank you.

(in reply to Jonathan Druart in comment 156)
Interestingly I cannot seem to replicate the error you are getting. I am not getting that error displayed in the web browser or the intranet-error.log file. 

Is the error appearing when running the web installer/onboarding tool in a particular language?

Hi Jonathan,

I haven't been able to make mine error in your way either. Here's what I did:

Applied the patches, 17855, your squashed patch, and then 18039 - all went ok.

Installed fr-FR in my developer install: 
sudo env KOHA_CONF=/etc/koha/sites/library/koha-conf.xml PERL5LIB=/home/liz/koha-src/koha/ perl translate install fr-FR
(I also installed de-DE, for funsies, but I haven't run through the installer in de-DE)

Dropped my koha database
Created a new db in it's place

Ran through the installer in fr-FR, and made it all the way through to logging in as our newly created super librarian. 

As a note, there were lots of "C4::Carp redefined" warnings in the intranet log, but they didn't seem to cause any noticeable problem.

One thing I did note was that the fr-FR DB templates have some additional mandatory options re: UNIMARC, and the default MARC flavour is the 3rd one in the list. That might not be right but I think not a problem for this bug to fix.

(in reply to Jonathan Druart in comment 156)

Liz and I discovered the source of the problem after looking at the patches in bug 17855 and 18039 and we saw that installer-strings.inc does not exist in either. 

The installer-strings.inc file is created in bug 17942 and so there must be a bad conflict in the merge to make the most recent patch of 17855. So I would suggest trying the merge again.

Katrin is kindly providing a resolved-by-her patch on Owen's bug. She's awesome and stuff.

Liz

(In reply to Alex Buckley from comment #159)
> The installer-strings.inc file is created in bug 17942 and so there must be
> a bad conflict in the merge to make the most recent patch of 17855. So I
> would suggest trying the merge again.

Silly me, I think I understood.
I certainly apply patches from 17942, then generated the translated languages. Then back on this one but forgot to regenerate the templates.

To make thing clear: I did not want to obsolete the other patches because of the error, and I was a bit tired yesterday and the last patch does not contain any commit message.

If you agree with the plan, we can go with try to get 18039 first, then this one.
But I have no idea how we can test 18039 widely and make sure it will not introduce any regressions.

At the beginning of the onboarding tool, if I already have patron categories and libraries, the steps 1 and 2 are skipped.
But it's a bit weird, we do not understand what is going on.
IIRC when I tested earlier, these steps contain message like "data already exists, click on next".

With fr-FR, I do not see the "select all data" link on "Selecting Default Settings"

(in reply to Jonathan Druart in comment 161)

>To make thing clear: I did not want to obsolete the other patches because of >the error, and I was a bit tired yesterday and the last patch does not contain >any commit message.

Now that that error has kindly been resolved by Katrin and Liz's work on bug 17942 I will continue to test these three combined bug patches (17855, 18039 and 17942) and if successful I will obsolete the 10 previous patches for this bug and amend the final commit message of this bug.

>If you agree with the plan, we can go with try to get 18039 first, then this >one.
>But I have no idea how we can test 18039 widely and make sure it will not >introduce any regressions.

To clarify do you mean try to get 18039 through testing and QA before this bug?   

>At the beginning of the onboarding tool, if I already have patron categories and libraries, the steps 1 and 2 are skipped.
>But it's a bit weird, we do not understand what is going on.
>IIRC when I tested earlier, these steps contain message like "data already >exists, click on next".

Yes I changed the way the steps for creating libraries, patron categories and item types were displayed if the user had installed sample libraries, patron categories and item types in the web installer due to your request in comment 70) so now the user is redirected to the next step that they do need to complete.

(In reply to Alex Buckley from comment #164)
> >If you agree with the plan, we can go with try to get 18039 first, then this >one.
> >But I have no idea how we can test 18039 widely and make sure it will not >introduce any regressions.
> 
> To clarify do you mean try to get 18039 through testing and QA before this
> bug?   

Yes definitely. We have to push a feature that will work for any languages, not only for English.

> >At the beginning of the onboarding tool, if I already have patron categories and libraries, the steps 1 and 2 are skipped.
> >But it's a bit weird, we do not understand what is going on.
> >IIRC when I tested earlier, these steps contain message like "data already >exists, click on next".
> 
> Yes I changed the way the steps for creating libraries, patron categories
> and item types were displayed if the user had installed sample libraries,
> patron categories and item types in the web installer due to your request in
> comment 70) so now the user is redirected to the next step that they do need
> to complete.

Ok, we can improve that later.

(In reply to Jonathan Druart in comment 165)

The thing that concerns me regarding having to test the combination of this bug with bugs 18039 and 17942 for all languages in order to get the onboarding tool into Koha in one go is:

* The differences in the web installer when run in different languages (e.g. No UNIMARC marc flavour available when running web installer in de-DE), are going to significantly hold up the process of getting the onboarding tool functionality out to the users due to complex troubleshooting to determine the reason for why the web installer is displaying different options when run in different languages and then the associated signoff process.

Is it therefore possible for us to try to get the combination of these three bugs signed off by language singularly (starting with English) to split the problem into manageable chunks that will get through the signoff and QA process?

Created attachment 61667 [details] [review]
Bug 17855: Onboarding tool

Created attachment 61668 [details] [review]
Bug 17855 - Reverted back to original step 3 in the web installer so there is no basic and advanced setup mode and consequently no need for SQL files to be moved to a 'default' directory for all languages.

Onboarding tool and the rest of the web installer still exists in this
commit

I have removed the web installer improvements that require the SQL files to be moved to the default directory, meaning the two patches I have attached to this bug report do not contain a web installer with a basic and advanced setup mode. 

Therefore these patches simply contain minor web installer UI improvements (which do not require the movement of SQL files to a default directory) in addition to the onboarding tool, and so the onboarding tool can be tested and pushed without the need for bug 18039 to pass QA. 

The onboarding tool has been tested multiple times in this bug report in the past.

I have tested these two patches by applying them to a clean branch and then running through the installer and onboarding tool and everything works as outlined in (https://koha-community.org/manual/installation/html/index.html) except there is no basic and advanced setup modes in the web installer because like I said I removed that functionality deliberately to get the onboarding tool through.

Created attachment 61986 [details] [review]
Bug 17855 - Reverted back to original step 3 in the web installer so there is no basic and advanced setup mode and consequently no need for SQL files to be moved to a 'default' directory for all languages.

Onboarding tool and the rest of the web installer still exists in this
commit

How I tested in my VM (Current master):
- Dropped database
- Recreated database
- Went through installer
- Language en, Marc21
- Installed mandatory data only
- Went through Onboarding tool
- After finishing, logged in as superlibrarian
  that was created by onboarding tool
Everything worked fine.

Signed-off-by: Marc Véron <veron@veron.ch>

Note: 
Signed off after test on current master. I did not test on top of 18039; this dependency seems not to be necessary.
It would be fine to have some text in 'Text to go to the release notes'.

Hi Alex,

Just found a small problem while testing around with an installation that was configured using the onboarding tool. 

The problem was a test failing: t/dbdependent/Budgets.t
I figured out that while onboarding, step 'Create a new patron category', I had put a Categorycode that was not 'S' (something like 'STAFF'). The test assumes that a category 'S' exists and fails without it.
I can imagine that other tests and other scripts could fail as well.

I went again through installation / onboarding and set the category code to 'S'. Now the test passed.

Maybe it is a good idea to have in the Onboarding tool a fix value 'S' for the new patron category. What do you think about?

Hm, should we not fix the test instead? Not sure I understood correctly, but having a category of 'S' should not be mandatory for the tests to pass in a normal environment.

Agree with Cait, this is a problem with the test.

(In reply to Katrin Fischer from comment #173)
> Hm, should we not fix the test instead? Not sure I understood correctly, but
> having a category of 'S' should not be mandatory for the tests to pass in a
> normal environment.

No problem for me. I filed a bug:
Bug 18420 - t/dbdependent/Budgets.t fails without patron category 'S'

- On installer/step3.tt:
I'd remove the <button> tag, it does not display as other buttons. Let's style it later.
Same for the other ones (search for '<button>' in installer/*.tt)

Why did you change the wording "update your database" to "Update Database"?

Same for "Install Basic Configuration Settings", uppercases on each word is wrong.

- On onboarding/onboardingstep5.tt:
  * max="10" => Why? Unnecessary limitation
  * No default value for checkouts, loan, renewals and renewal period

- Step "Create a new patron category", I can choose pick negative value for "months" (cannot confirm the form but would be good to min="0")

- If I hit onboarding.pl or install.pl once the install is done, I login with DB user, click on "next" twice and end  on a "Koha web installer › Step 3" with a button "Back to Installation".
Before this patch it was a "Continue to log in to Koha".
This is wrong.

*
+        template_name => "/onboarding/onboardingstep"
+          . ( $step ? $step : 1 ) . ".tt",

Do a check on $step to make sure it is an integer, just in case...

*
+        #Adds a new patron category to the database
+        $category = Koha::Patron::Category->new(
+            {
[SNIP]
+                enrolmentperioddate   => $enrolmentperioddate,

$enrolmentperioddate is a date in iso format, I think you should be able to give a DateTime object (return by dt_from_string) directly.

Created attachment 62186 [details] [review]
Bug 17855 - Followup patch following requests in comments 176 and 177

In the 'Bug 17855 - Followup patch following requests in comments 176 and 177' patch I have addressed all issues brought up in comments 176 and 177, see below:

1. Removed the <button> tag from all installer/*.tt files

2. I cannot remember the rationale for the change of the wording from 'update your database' so I have changed it back to 'Update your database'. Note: I have made only the first letter capitalised thus meeting coding guidelines. 

3. I have changed 'Install Basic Configuration Settings' to 'Install basic configuration settings' so it meets the coding guidelines

4. In onboarding/onboardingstep5.tt I have removed max="10" limit on input boxes

5. I have written in default values for checkouts, loan, renewals and renewal period based on the values in the example circulation rule in the Koha 16.11 manual

6. Implemented min="0" attribute for months in "Create a new patron category" step

7. Implemented the "Continue to log in to Koha" link (which works successfully) if the install.pl is run after installation has been completed

8. I have checked and I can confirm that $step variable is an integer

9. I have set the $enrolementperioddate variable as a DateTime object, see below code snippet. I  tried removing the dateformat field altogether however that made the enrolementperioddate field empty. 

187         #Converts the string into a date format
188         if ($enrolmentperioddate) {
189             $enrolmentperioddate = output_pref(
190                 {
191                     dt         => dt_from_string($enrolmentperioddate),
192                     dateformat => 'DateTime',
193                     dateonly   => 1,
194                 }
195             );
196         }

Created attachment 62191 [details] [review]
Bug 17855: Onboarding tool

Tested 3 patches together on current master
- Dropped database
- Recreated database
- Went through installer
- Language en, Marc21
- Installed mandatory data only
- Went through Onboarding tool
- After finishing, logged in as superlibrarian
  that was created by onboarding tool
Everything worked fine.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 62192 [details] [review]
Bug 17855 - Reverted back to original step 3 in the web installer so there is no basic and advanced setup mode and consequently no need for SQL files to be moved to a 'default' directory for all languages.

Onboarding tool and the rest of the web installer still exists in this
commit

How I tested in my VM (Current master):
- Dropped database
- Recreated database
- Went through installer
- Language en, Marc21
- Installed mandatory data only
- Went through Onboarding tool
- After finishing, logged in as superlibrarian
  that was created by onboarding tool
Everything worked fine.

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 62193 [details] [review]
Bug 17855 - Followup patch following requests in comments 176 and 177

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 62220 [details] [review]
Bug 17855: perltidy onboarding.pl

Created attachment 62221 [details] [review]
Bug 17855: Do not create a new dbh, get it from C4::Context->dbh

Created attachment 62222 [details] [review]
Bug 17855: Redirect to the installer if installation process is not done yet

Created attachment 62223 [details] [review]
Bug 17855: No need to resend the library list to the template

This has been done at the beginning of the script

(In reply to Jonathan Druart from comment #176)
> - If I hit onboarding.pl or install.pl once the install is done, I login
> with DB user, click on "next" twice and end  on a "Koha web installer › Step
> 3" with a button "Back to Installation".
> Before this patch it was a "Continue to log in to Koha".
> This is wrong.

This was not only a wording issue, to recreate:
Go throw the install process, do not use the onboarding tool.
Hit /cgi-bin/koha/installer/install.pl, login.
You are redirected to http://pro.kohadev.vm/cgi-bin/koha/installer/install.pl?step=3, then click "update your database", then "Continue to log in to Koha"
But actually the current behaviour should be sufficient, do not worry about that.

However there are several other things wrong (sorry to always find something wrong...)
The errors are not correctly handled. For instance if the branchcode already exists when you create a new library, you do not get the correct error.
I do not know how we could achieve that easily with 1 template per step. Indeed we need to call get_template_and_user to know if the user is logged in, but we pass the template file we need at that point.

I will think a bit more about that.

I am on it.

Created attachment 62231 [details] [review]
Bug 17855: Simplify the onboarding tool

This patch should not modify a lot the behaviours of the onboarding
tool.
Its goal is mainly to remove duplicated as well as useless (because
copy/pasted from existing script files) code.

It assumes that the onboarding tool will be done on an empty database
and will skip steps that are not needed. For instance if a library
already exists, the first step will be skipped.

One of the main problem was the lack of feedback messages sent to the
user when something wrong/ok happened.

Explanation on main changes:
1. Use checkauth first, then get_template_and_user
=> As we do not know the template to use, it's better to use checkauth
first to know if the user is logged in, then retrieve the template we
need, depending on the success or the failure of the action
2. Create a @messages variables
Pushing messages to this variable and handling the messages via an
include files (onboarding_messages.inc) simplify error handling. Note
that we could remove this include file if we merge all the
onboardingstepX.tt files altogether
3. Simplify creation of the admnistrator user
This patch removes some unecessary checks done on the user's info
(passwd to short, mandatory fields

Todo (minor): Add style to feedback messages

Hi Alex and Marc,
Would you mind taking a look at this patch?
I have removed and simplified some code.
Hope it makes sense to you.

Sorry to ask for another signoff again...

Alex, use `git show -w` to see the modifications without the indentation changes (especially for .tt files).

Created attachment 62233 [details] [review]
Bug 17855: perltidy onboarding.pl

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62234 [details] [review]
Bug 17855: Do not create a new dbh, get it from C4::Context->dbh

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62235 [details] [review]
Bug 17855: Redirect to the installer if installation process is not done yet

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62236 [details] [review]
Bug 17855: No need to resend the library list to the template

This has been done at the beginning of the script

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62237 [details] [review]
Bug 17855: Simplify the onboarding tool

This patch should not modify a lot the behaviours of the onboarding
tool.
Its goal is mainly to remove duplicated as well as useless (because
copy/pasted from existing script files) code.

It assumes that the onboarding tool will be done on an empty database
and will skip steps that are not needed. For instance if a library
already exists, the first step will be skipped.

One of the main problem was the lack of feedback messages sent to the
user when something wrong/ok happened.

Explanation on main changes:
1. Use checkauth first, then get_template_and_user
=> As we do not know the template to use, it's better to use checkauth
first to know if the user is logged in, then retrieve the template we
need, depending on the success or the failure of the action
2. Create a @messages variables
Pushing messages to this variable and handling the messages via an
include files (onboarding_messages.inc) simplify error handling. Note
that we could remove this include file if we merge all the
onboardingstepX.tt files altogether
3. Simplify creation of the admnistrator user
This patch removes some unecessary checks done on the user's info
(passwd to short, mandatory fields

Todo (minor): Add style to feedback messages

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62238 [details] [review]
Bug 17855: perltidy onboarding.pl

Created attachment 62239 [details] [review]
Bug 17855: Do not create a new dbh, get it from C4::Context->dbh

Created attachment 62240 [details] [review]
Bug 17855: Redirect to the installer if installation process is not done yet

Created attachment 62241 [details] [review]
Bug 17855: No need to resend the library list to the template

This has been done at the beginning of the script

Created attachment 62242 [details] [review]
Bug 17855: Simplify the onboarding tool

This patch should not modify a lot the behaviours of the onboarding
tool.
Its goal is mainly to remove duplicated as well as useless (because
copy/pasted from existing script files) code.

It assumes that the onboarding tool will be done on an empty database
and will skip steps that are not needed. For instance if a library
already exists, the first step will be skipped.

One of the main problem was the lack of feedback messages sent to the
user when something wrong/ok happened.

Explanation on main changes:
1. Use checkauth first, then get_template_and_user
=> As we do not know the template to use, it's better to use checkauth
first to know if the user is logged in, then retrieve the template we
need, depending on the success or the failure of the action
2. Create a @messages variables
Pushing messages to this variable and handling the messages via an
include files (onboarding_messages.inc) simplify error handling. Note
that we could remove this include file if we merge all the
onboardingstepX.tt files altogether
3. Simplify creation of the admnistrator user
This patch removes some unecessary checks done on the user's info
(passwd to short, mandatory fields

Todo (minor): Add style to feedback messages

Created attachment 62243 [details] [review]
Bug 17855: perltidy onboarding.pl

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62251 [details] [review]
Bug 17855: Onboarding tool

Tested 3 patches together on current master
- Dropped database
- Recreated database
- Went through installer
- Language en, Marc21
- Installed mandatory data only
- Went through Onboarding tool
- After finishing, logged in as superlibrarian
  that was created by onboarding tool
Everything worked fine.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 62252 [details] [review]
Bug 17855 - Reverted back to original step 3 in the web installer so there is no basic and advanced setup mode and consequently no need for SQL files to be moved to a 'default' directory for all languages.

Onboarding tool and the rest of the web installer still exists in this
commit

How I tested in my VM (Current master):
- Dropped database
- Recreated database
- Went through installer
- Language en, Marc21
- Installed mandatory data only
- Went through Onboarding tool
- After finishing, logged in as superlibrarian
  that was created by onboarding tool
Everything worked fine.

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 62253 [details] [review]
Bug 17855 - Followup patch following requests in comments 176 and 177

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 62254 [details] [review]
Bug 17855: perltidy onboarding.pl

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62255 [details] [review]
Bug 17855: Do not create a new dbh, get it from C4::Context->dbh

Created attachment 62256 [details] [review]
Bug 17855: Redirect to the installer if installation process is not done yet

Created attachment 62257 [details] [review]
Bug 17855: No need to resend the library list to the template

This has been done at the beginning of the script

Created attachment 62258 [details] [review]
Bug 17855: Simplify the onboarding tool

This patch should not modify a lot the behaviours of the onboarding
tool.
Its goal is mainly to remove duplicated as well as useless (because
copy/pasted from existing script files) code.

It assumes that the onboarding tool will be done on an empty database
and will skip steps that are not needed. For instance if a library
already exists, the first step will be skipped.

One of the main problem was the lack of feedback messages sent to the
user when something wrong/ok happened.

Explanation on main changes:
1. Use checkauth first, then get_template_and_user
=> As we do not know the template to use, it's better to use checkauth
first to know if the user is logged in, then retrieve the template we
need, depending on the success or the failure of the action
2. Create a @messages variables
Pushing messages to this variable and handling the messages via an
include files (onboarding_messages.inc) simplify error handling. Note
that we could remove this include file if we merge all the
onboardingstepX.tt files altogether
3. Simplify creation of the admnistrator user
This patch removes some unecessary checks done on the user's info
(passwd to short, mandatory fields

Todo (minor): Add style to feedback messages

Created attachment 62259 [details] [review]
Bug 17855: perltidy onboarding.pl

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62260 [details] [review]
Bug 17855: Do not create a new dbh, get it from C4::Context->dbh

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62261 [details] [review]
Bug 17855: Redirect to the installer if installation process is not done yet

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62262 [details] [review]
Bug 17855: No need to resend the library list to the template

This has been done at the beginning of the script

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62263 [details] [review]
Bug 17855: Simplify the onboarding tool

This patch should not modify a lot the behaviours of the onboarding
tool.
Its goal is mainly to remove duplicated as well as useless (because
copy/pasted from existing script files) code.

It assumes that the onboarding tool will be done on an empty database
and will skip steps that are not needed. For instance if a library
already exists, the first step will be skipped.

One of the main problem was the lack of feedback messages sent to the
user when something wrong/ok happened.

Explanation on main changes:
1. Use checkauth first, then get_template_and_user
=> As we do not know the template to use, it's better to use checkauth
first to know if the user is logged in, then retrieve the template we
need, depending on the success or the failure of the action
2. Create a @messages variables
Pushing messages to this variable and handling the messages via an
include files (onboarding_messages.inc) simplify error handling. Note
that we could remove this include file if we merge all the
onboardingstepX.tt files altogether
3. Simplify creation of the admnistrator user
This patch removes some unecessary checks done on the user's info
(passwd to short, mandatory fields

Todo (minor): Add style to feedback messages

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Created attachment 62281 [details] [review]
Bug 17855: Onboarding tool

Tested 3 patches together on current master
- Dropped database
- Recreated database
- Went through installer
- Language en, Marc21
- Installed mandatory data only
- Went through Onboarding tool
- After finishing, logged in as superlibrarian
  that was created by onboarding tool
Everything worked fine.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62282 [details] [review]
Bug 17855 - Reverted back to original step 3 in the web installer so there is no basic and advanced setup mode and consequently no need for SQL files to be moved to a 'default' directory for all languages.

Onboarding tool and the rest of the web installer still exists in this
commit

How I tested in my VM (Current master):
- Dropped database
- Recreated database
- Went through installer
- Language en, Marc21
- Installed mandatory data only
- Went through Onboarding tool
- After finishing, logged in as superlibrarian
  that was created by onboarding tool
Everything worked fine.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62283 [details] [review]
Bug 17855 - Followup patch following requests in comments 176 and 177

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62284 [details] [review]
Bug 17855: perltidy onboarding.pl

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62285 [details] [review]
Bug 17855: Do not create a new dbh, get it from C4::Context->dbh

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62286 [details] [review]
Bug 17855: Redirect to the installer if installation process is not done yet

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62287 [details] [review]
Bug 17855: No need to resend the library list to the template

This has been done at the beginning of the script

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62288 [details] [review]
Bug 17855: Simplify the onboarding tool

This patch should not modify a lot the behaviours of the onboarding
tool.
Its goal is mainly to remove duplicated as well as useless (because
copy/pasted from existing script files) code.

It assumes that the onboarding tool will be done on an empty database
and will skip steps that are not needed. For instance if a library
already exists, the first step will be skipped.

One of the main problem was the lack of feedback messages sent to the
user when something wrong/ok happened.

Explanation on main changes:
1. Use checkauth first, then get_template_and_user
=> As we do not know the template to use, it's better to use checkauth
first to know if the user is logged in, then retrieve the template we
need, depending on the success or the failure of the action
2. Create a @messages variables
Pushing messages to this variable and handling the messages via an
include files (onboarding_messages.inc) simplify error handling. Note
that we could remove this include file if we merge all the
onboardingstepX.tt files altogether
3. Simplify creation of the admnistrator user
This patch removes some unecessary checks done on the user's info
(passwd to short, mandatory fields

Todo (minor): Add style to feedback messages

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62668 [details] [review]
Bug 17855: Onboarding tool

Tested 3 patches together on current master
- Dropped database
- Recreated database
- Went through installer
- Language en, Marc21
- Installed mandatory data only
- Went through Onboarding tool
- After finishing, logged in as superlibrarian
  that was created by onboarding tool
Everything worked fine.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62669 [details] [review]
Bug 17855 - Reverted back to original step 3 in the web installer so there is no basic and advanced setup mode and consequently no need for SQL files to be moved to a 'default' directory for all languages.

Onboarding tool and the rest of the web installer still exists in this
commit

How I tested in my VM (Current master):
- Dropped database
- Recreated database
- Went through installer
- Language en, Marc21
- Installed mandatory data only
- Went through Onboarding tool
- After finishing, logged in as superlibrarian
  that was created by onboarding tool
Everything worked fine.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62670 [details] [review]
Bug 17855 - Followup patch following requests in comments 176 and 177

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62671 [details] [review]
Bug 17855: perltidy onboarding.pl

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62672 [details] [review]
Bug 17855: Do not create a new dbh, get it from C4::Context->dbh

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62673 [details] [review]
Bug 17855: Redirect to the installer if installation process is not done yet

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62674 [details] [review]
Bug 17855: No need to resend the library list to the template

This has been done at the beginning of the script

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 62675 [details] [review]
Bug 17855: Simplify the onboarding tool

This patch should not modify a lot the behaviours of the onboarding
tool.
Its goal is mainly to remove duplicated as well as useless (because
copy/pasted from existing script files) code.

It assumes that the onboarding tool will be done on an empty database
and will skip steps that are not needed. For instance if a library
already exists, the first step will be skipped.

One of the main problem was the lack of feedback messages sent to the
user when something wrong/ok happened.

Explanation on main changes:
1. Use checkauth first, then get_template_and_user
=> As we do not know the template to use, it's better to use checkauth
first to know if the user is logged in, then retrieve the template we
need, depending on the success or the failure of the action
2. Create a @messages variables
Pushing messages to this variable and handling the messages via an
include files (onboarding_messages.inc) simplify error handling. Note
that we could remove this include file if we merge all the
onboardingstepX.tt files altogether
3. Simplify creation of the admnistrator user
This patch removes some unecessary checks done on the user's info
(passwd to short, mandatory fields

Todo (minor): Add style to feedback messages

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Pushed to master for 17.05, thanks Alex, Jonathan!

This is a new idea - I suggest marking it as 'new' instead of 'enhancement'

This won't get ported back to 16.11.x as it is a new feature.