Bug 17718

Created attachment 57948 [details] [review]
Bug 17718: Smartform API allowed in patron registration

Test plan:
1) Apply the patch
2) Setup preferences "SmartformEnable", "SmartformClientID"
3) Edit a patron
4) Start entering street in an address
5) Select a street from autocomplete
6) Continue entering street number and select the right address
7) ZIP and city should get values from autocomplete
8) Disable SmartformEnable and repeat steps above
9) No autocompletion should occur

Side note: only DE version of address template scripts have changed, because Czech intallations do not need US address version at all.

Radek, why did you not use template plugin Koha.Preference for getting SmartformEnable syspref?

Is there any demo account on Smartform.cz? Or should I register to pay for this to start testing?

> Is there any demo account on Smartform.cz? Or should I register to pay for
> this to start testing?

Forgot this question, I sorted it out: It's enough to register here to start using this service https://secure.smartform.cz/admin/user/create

No registration is required to start testing, just use "smartform" as client ID when working on localhost.

Created attachment 57969 [details] [review]
Bug 17718: Used Koha.preference instead of passing params to templates from module memberentry.pl

Test plan:
1) Apply the patch
2) Setup preferences "SmartformEnable", "SmartformClientID"
   (register at smartform.cz or use "smartform" as the client ID)
3) Show/hide streetnumber as desired in BorrowerUnwantedField pref
   (eg. fill in "streetnumber|B_streetnumber")
4) Edit a patron
5) Start entering street in an address field
6) Select a street from autocomplete
7) Continue entering street number and select the right address
8) ZIP and city should get values from autocomplete
9) Disable SmartformEnable and repeat steps above
10) No autocompletion should occur

Address templates allow to use street number merged into one field with the street name or separately as two fields. Autocomplete should work correctly without problems regardless the settings in BorrowerUnwantedField system preference.

Sorry, QA tool complains:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/member-alt-address-style-de.inc
   FAIL	  tt_valid
		lines 12, 47, 72, 84

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/member-alt-contact-style-de.inc
   FAIL	  tt_valid
		lines 36, 60, 72

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-de.inc
   FAIL	  tt_valid
		lines 35, 47, 71, 83

It's related to HTML1 point of coding guidelines I think.

But the functionality is OK so, when you fix this, I am going to sign-off ;)

Created attachment 57994 [details] [review]
Bug 17718: Fixed QA tool complains

I don't fully understand the reason, but I had run the tool without any
errors. Now I did my best according to coding guidelines, I hope.

Test plan:
1) Apply the patch
2) Setup preferences "SmartformEnable", "SmartformClientID"
   (register at smartform.cz or use "smartform" as the client ID)
3) Show/hide streetnumber as desired in BorrowerUnwantedField pref
   (eg. fill in "streetnumber|B_streetnumber")
4) Edit a patron
5) Start entering street in an address field
6) Select a street from autocomplete
7) Continue entering street number and select the right address
8) ZIP and city should get values from autocomplete
9) Disable SmartformEnable and repeat steps above
10) No autocompletion should occur

Created attachment 57995 [details] [review]
[SIGNED-OFF] Bug 17718: Smartform API allowed in patron edit form

Test plan:
1) Apply the patch
2) Setup preferences "SmartformEnable", "SmartformClientID"
   (register at smartform.cz or use "smartform" as the client ID)
3) Show/hide streetnumber as desired in BorrowerUnwantedField pref
   (eg. fill in "streetnumber|B_streetnumber")
4) Edit a patron
5) Start entering street in an address field
6) Select a street from autocomplete
7) Continue entering street number and select the right address
8) ZIP and city should get values from autocomplete
9) Disable SmartformEnable and repeat steps above
10) No autocompletion should occur

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

I squashed the patches to simplify history

I have enabled SmartformEnable, set SmartformClientID to "smartform", I create a new patron, fill address input with some random chars, but no autocompletion (and no request to the ws).
Can you double-check or tell me what I missed?

Jonathan, were you running it from localhost and using HTTP URL from a local IP? If not, it may not run. The service has some security limitations, because it's paid for business, but any Czech library can use it for free after registration.

Few questions:
Why not only have one preference? If the client id is empty, the pref is obviously not enabled. We already have so much preferences ;)

From a security perspective, I do not really like to include code like this:
<script type="text/javascript" src="http://smartform.cz/api/v1/smartform.js"></script>
Can we do it some other way? Add it to a local lib directory, and have more control?

In general: Can you convince me why we should push it in this current form? I could imagine that we could have a more general 'plugin' that operates on address fields and allows access to selected web services, or so?

Anyway, thanks for your work ! Your answers or adjustments may bring this further.

Changing status to reflect need for feedback.

One preference is a good idea, I will change it.

I understand the security reasons about cross-site inclusion of javascripts. The problem is, this a 3rd party service we really like and we trust it, because it's widely used in our country including a leader of IT market. Not a good reason, I know. Currently they offer direct linkage only. I am not sure if they will ever allow to download the sources as a package. It is connected to their database structure and so they have a good control over compatibility issues, I think. However, I'll try to ask them for the sources. For your information, please look at
http://www.smartform.cz/index.html
and you will see how the service grows on the graph in the right bottom corner of the page.

The only reason to ask for the current form is a growing community of this service users in the Czech Republic. Originally we had an idea similar to the Norwegian patron database and we planned this feature for our country only. On the other hand, you are right that kind of general plugin or driver would be useful, but it highly exceeds the original limits.

> 
> In general: Can you convince me why we should push it in this current form?
> I could imagine that we could have a more general 'plugin' that operates on
> address fields and allows access to selected web services, or so?
> 

As this is quite a specific API that might change, this seems like an ideal use case for a plugin. And the plugin system has been improved quite a bit since this bug was originally filed in 2016.