Bug 18110 - Adds FR to the syspref AddressFormat
Summary: Adds FR to the syspref AddressFormat
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Baptiste
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-14 09:56 UTC by Baptiste
Modified: 2017-05-13 12:31 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Add a field FR to the syspref AddressFormat (3.38 KB, patch)
2017-02-14 10:01 UTC, Baptiste
Details | Diff | Splinter Review
Add a field FR to the syspref AddressFormat (20.76 KB, patch)
2017-02-14 15:29 UTC, Baptiste
Details | Diff | Splinter Review
Add a field FR to the syspref AddressFormat (20.91 KB, patch)
2017-02-14 15:38 UTC, Baptiste
Details | Diff | Splinter Review
BUG 18110 : Removed a wrong syspref from code (1.43 KB, patch)
2017-02-17 15:05 UTC, Baptiste
Details | Diff | Splinter Review
Screenshot with patch (65.24 KB, image/png)
2017-02-24 14:44 UTC, Marc Véron
Details
Bug 16034 - DBRev 16.12.00.010 (2.60 KB, patch)
2017-02-27 08:54 UTC, Baptiste
Details | Diff | Splinter Review
BUG 18110 : Modified the adresses in order to fit the french standards (5.29 KB, patch)
2017-02-27 09:00 UTC, Baptiste
Details | Diff | Splinter Review
BUG 18110 : Correct display and syspref AddFormat (20.04 KB, patch)
2017-02-27 09:36 UTC, Baptiste
Details | Diff | Splinter Review
BUG 18110 : Correct display and syspref AddFormat (20.07 KB, patch)
2017-02-27 09:41 UTC, Baptiste
Details | Diff | Splinter Review
BUG 18110: Corrected display address format (3.59 KB, patch)
2017-03-24 12:50 UTC, Baptiste
Details | Diff | Splinter Review
BUG 18810: Removed unwanted spaces (1.65 KB, patch)
2017-03-27 07:33 UTC, Baptiste
Details | Diff | Splinter Review
BUG 18110 : Correct display and syspref AddFormat (20.12 KB, patch)
2017-03-28 03:39 UTC, Aleisha Amohia
Details | Diff | Splinter Review
BUG 18110: Corrected display address format (3.65 KB, patch)
2017-03-28 03:39 UTC, Aleisha Amohia
Details | Diff | Splinter Review
BUG 18810: Removed unwanted spaces (1.71 KB, patch)
2017-03-28 03:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18110: Folllowup to fix alternative address and add missing class (3.25 KB, patch)
2017-03-28 07:33 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18110: Update addressFormat in atomicupdate (932 bytes, patch)
2017-04-03 13:42 UTC, Baptiste
Details | Diff | Splinter Review
Bug 18110: Folllowup to fix alternative address and add missing class (3.32 KB, patch)
2017-04-25 08:23 UTC, Baptiste
Details | Diff | Splinter Review
Bug 18110: Update addressFormat in atomicupdate (848 bytes, patch)
2017-04-25 08:24 UTC, Baptiste
Details | Diff | Splinter Review
Bug 18110: Update addressFormat in atomicupdate (982 bytes, patch)
2017-04-25 12:41 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18110: Add a field FR to the syspref AddressFormat (20.20 KB, patch)
2017-04-26 18:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18110: Corrected display address format (3.72 KB, patch)
2017-04-26 18:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18110: Removed unwanted spaces (1.72 KB, patch)
2017-04-26 18:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18110: Folllowup to fix alternative address and add missing class (3.40 KB, patch)
2017-04-26 18:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18110: Update addressFormat in atomicupdate (1.03 KB, patch)
2017-04-26 18:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18110: Do not update value for existing installs (2.25 KB, patch)
2017-04-26 18:51 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Baptiste 2017-02-14 09:56:31 UTC

    
Comment 1 Baptiste 2017-02-14 10:01:46 UTC
Created attachment 60176 [details] [review]
Add a field FR to the syspref AddressFormat

Testplan
1 - Apply
2 - Check there is a field FR in the syspref AddressFormat
Comment 2 Baptiste 2017-02-14 15:29:38 UTC
Created attachment 60214 [details] [review]
Add a field FR to the syspref AddressFormat

Testplan
1 - Apply
2 - Check there is a field FR in the syspref AddressFormat
Comment 3 Baptiste 2017-02-14 15:38:48 UTC
Created attachment 60215 [details] [review]
Add a field FR to the syspref AddressFormat

Testplan
1 - Apply patch
2 - Check if there is a field FR in the syspref AddressFormat, enable it
3 - Create a new borrower and check the order of the fields for the address
4 - Check if it's address appears in a propper manner
Comment 4 Marc Véron 2017-02-17 14:31:44 UTC
Hi Babtiste, 
the patch adds a line 
('LoadSearchHistoryToTheFirstLoggedUser',.... 
to installer/data/mysql/sysprefs.sql (Line 237)

It seems to me that it is not related to this Bug?
Comment 5 Baptiste 2017-02-17 15:05:18 UTC
Created attachment 60426 [details] [review]
BUG 18110 : Removed a wrong syspref from code
Comment 6 Marc Véron 2017-02-17 15:24:36 UTC
On the patron's detail page, street number goes on a separate line, something like
11
rue de Bâle

instead of
11 rue de Bâle

I think the code should go inside the li element in member-display-address-style-fr.inc (maybe at other places as well?)

You have something like:

[% IF streetnumber %] [% streetnumber %][% END %]
[%IF roadtype_desc %] [% roadtype_desc %][% END %]
<li class="patronaddress1">[% address %]
</li>

I think it should be something like:

<li class="patronaddress1">
    [% IF streetnumber %] [% streetnumber %][% END %]
    [%IF roadtype_desc %] [% roadtype_desc %][% END %]
    [% address %]
</li>
Comment 7 Baptiste 2017-02-24 13:20:05 UTC
Hi,
Are you sure you tested the newest version of the patch ?
I remember I corrected at last a part of the problem

Could you try again and send me the exact test plan you applied to get this bug ?

I tried to make a new branch from master and apply the patch on a new branch : I can't reproduce it.
Comment 8 Marc Véron 2017-02-24 14:44:33 UTC
Created attachment 60657 [details]
Screenshot with patch

The streetnumber displays above the street (see screenshots at the left), I would expect them on the same line.
4554 Library Rd. 
instead of
4554
Library Rd. 

Editing the main address (Link below "Gender") displays some stray HTML (screenshots right).
Comment 9 Baptiste 2017-02-24 14:55:25 UTC
Hi !

Did you reapply the patch ?
I mean I can't reproduce your problem again, and I think I have corrected it, and by reapplying it on the master I can't reproduce it again.
Comment 10 Marc Véron 2017-02-24 15:45:01 UTC
Yes, I applied the patch.

I think it is in member-display-address-style-fr.inc:

The patch is as follows:
(...)
    [% IF ( address ) %]
        [% SET roadtype_desc = '' %]
        [% IF streettype %]
            [% SET roadtype_desc = AuthorisedValues.GetByCode('ROADTYPE', streettype) %]
        [% END %]
        [% IF streetnumber %] [% streetnumber %][% END %]
        [%IF roadtype_desc %] [% roadtype_desc %][% END %]
        <li class="patronaddress1">[% address %]
       </li>
    [% END %]
(...)

If I change it to the following, the address displays OK:

(...)

    [% IF ( address ) %]
        [% SET roadtype_desc = '' %]
        [% IF streettype %]
            [% SET roadtype_desc = AuthorisedValues.GetByCode('ROADTYPE', streettype) %]
        [% END %]

        <li class="patronaddress1">
            [% IF streetnumber %] [% streetnumber %][% END %]
            [%IF roadtype_desc %] [% roadtype_desc %][% END %]
            [% address %]
       </li>
    [% END %]


(...)
Comment 11 Marc Véron 2017-02-24 15:53:26 UTC
Additionally, member-main-address-style-fr.inc line 1 misses an opening < :

fieldset class="rows" id="memberentry_mainaddress">

instead of:
 	
<fieldset class="rows" id="memberentry_mainaddress">
Comment 12 Baptiste 2017-02-27 08:54:04 UTC
Created attachment 60689 [details] [review]
Bug 16034 - DBRev 16.12.00.010

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Baptiste 2017-02-27 09:00:50 UTC
Created attachment 60690 [details] [review]
BUG 18110 : Modified the adresses in order to fit the french standards
Comment 14 Baptiste 2017-02-27 09:36:09 UTC
Created attachment 60694 [details] [review]
BUG 18110 : Correct display and syspref AddFormat

Add a field FR to the syspref AddressFormat

Testplan
1 - Apply patch
2 - Check if there is a field FR in the syspref AddressFormat, enable it
3 - Create a new borrower and check the order of the fields for the address
4 - Check if it's address appears in a propper manner

27/02/17 : Fixed the display
Comment 15 Baptiste 2017-02-27 09:41:46 UTC
Created attachment 60695 [details] [review]
BUG 18110 : Correct display and syspref AddFormat

Add a field FR to the syspref AddressFormat

Testplan
1 - Apply patch
2 - Check if there is a field FR in the syspref AddressFormat, enable it
3 - Create a new borrower and check the order of the fields for the address
4 - Check if it's address appears in a propper manner

27/02/17 : Fixed the display
Comment 16 Marc Véron 2017-02-27 16:16:49 UTC
I'm really sorry, but there is still a problem with 
koha-tmpl/intranet-tmpl/prog/en/includes/member-display-address-style-fr.inc

The streetnumber still does not display on the same line as the address.

Your code reads (line 7 and following):

        <li>
        [% IF streetnumber %] [% streetnumber %][% END %]
        [%IF roadtype_desc %] [% roadtype_desc %][% END %]
        </li>
            <li class="patronaddress1">[% address %]
            </li>

It should be:

            <li class="patronaddress1">
                [% IF streetnumber %] [% streetnumber %][% END %]
                [%IF roadtype_desc %] [% roadtype_desc %][% END %]
                [% address %]
            </li>
Comment 17 Baptiste 2017-02-28 08:47:50 UTC
Hi !
Do you think we should have everything printed on the same line ? I wasn't sure it would we best way to display the address, but is ok, I'll change it again ;)
Comment 18 Marc Véron 2017-02-28 10:40:22 UTC
Hi Baptiste,

Yes, I think that number, street and address should display on the same line.
See:

https://fr.wikipedia.org/wiki/Adresse_postale#Recommandations_de_La_Poste

(...) numéro dans la voie (éventuellement suivi d’un complément de numéro tel bis, ter, quater, etc.), immédiatement suivi du type de voie (rue, avenue, etc.) et du nom de celle-ci (...)
Comment 19 jmbroust 2017-03-20 14:24:17 UTC
There is a problem when b.streettype is used, address 1 :

1 -  is displayed on separate line at :
http://pro.user05-koha.sandbox.biblibre.eu/cgi-bin/koha/circ/circulation.pl?borrowernumber=7020

http://pro.user05-koha.sandbox.biblibre.eu/cgi-bin/koha/members/moremember.pl?borrowernumber=7020

Furthermore, streettype is not displayed at all on some pages : 
For example : 
http://pro.user05-koha.sandbox.biblibre.eu/cgi-bin/koha/members/boraccount.pl?borrowernumber=7020

Other point : streetnumber nor streettype are not displayed here : 
http://pro.user05-koha.sandbox.biblibre.eu/cgi-bin/koha/members/member-password.pl?member=7020

but  I guess these should be filled in another bugzilla ticket).
Comment 20 Baptiste 2017-03-24 12:50:18 UTC
Created attachment 61610 [details] [review]
BUG 18110: Corrected display address format

- minor changes on address format
- corrected member-password so that steetnumber is given to the template
Comment 21 Baptiste 2017-03-24 12:52:51 UTC
I corrected the display of streetnumber in this ticket, I don't think it's necessary to open a new ticket just for one code line.
Comment 22 Marc Véron 2017-03-25 09:28:51 UTC
Hi Babtiste,

Changes work fine, but please have a look at Alternate address as well, streetnumber is on a separate line and street type is after address.

See: member-display-alt-address-style-fr.inc

Marc
Comment 23 Marc Véron 2017-03-26 14:09:12 UTC
Sorry, I was wrong in my comment #16. I just found out that the addres line starts with an unwanted space character in member-display-address-style-fr.inc lines 8/9:

        [% IF streetnumber %] [% streetnumber %][% END %]
        [%IF roadtype_desc %] [% roadtype_desc %][% END %]

The space characters after the IFs should move before the ENDs, otherwise the line will begin wit a blank.

This is what I mean:

        [% IF streetnumber %][% streetnumber %] [% END %]
        [%IF roadtype_desc %][% roadtype_desc %] [% END %]

BTW: Will be the same for member-display-alt-address-style-fr.inc
Comment 24 Marc Véron 2017-03-26 14:18:39 UTC
Sorry, one more unwanted space at the beginning of a line:

[% IF ( city ) %]<li class="patroncity">
    [%IF (zipcode ) %][% zipcode %] [% END %][% city %][% IF ( state )

...should be:

[% IF ( city ) %]
    <li class="patroncity">[%IF (zipcode ) %][% zipcode %] [% END %][% city %][% IF ( state )

Otherwise you get a space before the line with zipcode, city etc.
Comment 25 Baptiste 2017-03-27 07:33:27 UTC
Created attachment 61639 [details] [review]
BUG 18810: Removed unwanted spaces
Comment 26 Marc Véron 2017-03-27 12:02:45 UTC
(In reply to Marc Véron from comment #22)
> Hi Babtiste,
> 
> Changes work fine, but please have a look at Alternate address as well,
> streetnumber is on a separate line and street type is after address.
> 
> See: member-display-alt-address-style-fr.inc
> 
> Marc

Hi Babtiste,
With Alternate address, I get:

Alternate address

777
Royale rue
75002 Paris
France

I would expect 

777 rue Royale
Comment 27 Aleisha Amohia 2017-03-28 03:39:36 UTC
Created attachment 61650 [details] [review]
BUG 18110 : Correct display and syspref AddFormat

Add a field FR to the syspref AddressFormat

Testplan
1 - Apply patch
2 - Check if there is a field FR in the syspref AddressFormat, enable it
3 - Create a new borrower and check the order of the fields for the address
4 - Check if it's address appears in a propper manner

27/02/17 : Fixed the display

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 28 Aleisha Amohia 2017-03-28 03:39:41 UTC
Created attachment 61651 [details] [review]
BUG 18110: Corrected display address format

- minor changes on address format
- corrected member-password so that steetnumber is given to the template

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 29 Aleisha Amohia 2017-03-28 03:40:49 UTC
Created attachment 61652 [details] [review]
BUG 18810: Removed unwanted spaces

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

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 30 Marc Véron 2017-03-28 07:32:45 UTC
Problem described in Comment #26 is not yet fixed.
Followup patch follows
Comment 31 Marc Véron 2017-03-28 07:33:04 UTC
Created attachment 61656 [details] [review]
Bug 18110: Folllowup to fix alternative address and add missing class

This patch fixes the display of alternative address (streetnumber, streettype
and address on same line, see comment #22, and it adds missing class
patronaddress1 to main address display.

To test:
- Create / edit a customer to have streetnumber, streettype and address in
  both main address and alternate address
- Switch syspref addressformat ot French style
- Verify that both main address and alternate address display as expected
  on patron's detail page (street number, street type and address on same
  line)
Comment 32 Marc Véron 2017-03-28 07:33:56 UTC
Switching back to NSO for followup patch
Comment 33 Jonathan Druart 2017-03-28 12:28:52 UTC
Do not forget the update DB entry. And commit messages has to start with 'Bug xxxxx', not 'BUG xxxxx'
Comment 34 Baptiste 2017-04-03 13:42:24 UTC
Created attachment 61796 [details] [review]
Bug 18110: Update addressFormat in atomicupdate
Comment 35 Marc Véron 2017-04-03 15:27:29 UTC
Hi Babtiste

- My followup patch would need a sign-off (See comment #32)
- Atomic update produces an sql error, UPDATE sytax seems to be wrong. Did you test it?

Marc
Comment 36 Baptiste 2017-04-25 08:23:51 UTC
Created attachment 62651 [details] [review]
Bug 18110: Folllowup to fix alternative address and add missing class

This patch fixes the display of alternative address (streetnumber, streettype
and address on same line, see comment #22, and it adds missing class
patronaddress1 to main address display.

To test:
- Create / edit a customer to have streetnumber, streettype and address in
  both main address and alternate address
- Switch syspref addressformat ot French style
- Verify that both main address and alternate address display as expected
  on patron's detail page (street number, street type and address on same
  line)

Works as expected.

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 37 Baptiste 2017-04-25 08:24:15 UTC
Created attachment 62652 [details] [review]
Bug 18110: Update addressFormat in atomicupdate
Comment 38 Baptiste 2017-04-25 08:27:06 UTC
Sorry, syntax was wrong, I understood the error message as: your base is already full but everything is ok.

I tested and signed off you patch, I just need the atomic update to be tested and signed off and I think we are done.
Comment 39 Marc Véron 2017-04-25 12:41:18 UTC
Created attachment 62664 [details] [review]
Bug 18110: Update addressFormat in atomicupdate

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 40 Jonathan Druart 2017-04-26 18:50:39 UTC
Created attachment 62740 [details] [review]
Bug 18110: Add a field FR to the syspref AddressFormat

Add a field FR to the syspref AddressFormat

Testplan
1 - Apply patch
2 - Check if there is a field FR in the syspref AddressFormat, enable it
3 - Create a new borrower and check the order of the fields for the address
4 - Check if it's address appears in a propper manner

27/02/17 : Fixed the display

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 41 Jonathan Druart 2017-04-26 18:50:44 UTC
Created attachment 62741 [details] [review]
Bug 18110: Corrected display address format

- minor changes on address format
- corrected member-password so that steetnumber is given to the template

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 42 Jonathan Druart 2017-04-26 18:50:49 UTC
Created attachment 62742 [details] [review]
Bug 18110: Removed unwanted spaces

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 43 Jonathan Druart 2017-04-26 18:50:56 UTC
Created attachment 62743 [details] [review]
Bug 18110: Folllowup to fix alternative address and add missing class

This patch fixes the display of alternative address (streetnumber, streettype
and address on same line, see comment #22, and it adds missing class
patronaddress1 to main address display.

To test:
- Create / edit a customer to have streetnumber, streettype and address in
  both main address and alternate address
- Switch syspref addressformat ot French style
- Verify that both main address and alternate address display as expected
  on patron's detail page (street number, street type and address on same
  line)

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Jonathan Druart 2017-04-26 18:51:01 UTC
Created attachment 62744 [details] [review]
Bug 18110: Update addressFormat in atomicupdate

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 45 Jonathan Druart 2017-04-26 18:51:06 UTC
Created attachment 62745 [details] [review]
Bug 18110: Do not update value for existing installs

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 46 Kyle M Hall 2017-04-28 12:54:23 UTC
Pushed to master for 17.05, thanks Baptiste, Marc, Jonathan!
Comment 47 Katrin Fischer 2017-05-13 12:31:34 UTC
This won't get ported back to 16.11.x as it is an enhancement.