25070 – Include files to display address and contact must be refactored

Bug 25070 - Include files to display address and contact must be refactored

Summary: Include files to display address and contact must be refactored

Status:	CLOSED FIXED

Alias:	None

Product:	Koha
Classification:	Unclassified
Component:	Architecture, internals, and plumbing (show other bugs)
Version:	unspecified
Hardware:	All All

Importance:	P5 - low enhancement
Assignee:	Jonathan Druart
QA Contact:	Katrin Fischer

URL:
Keywords:

Depends on:	4041 18110 25069
Blocks:	22087 25463 25839 25842 26556 29213
	Show dependency tree / graph

Reported:	2020-04-06 12:54 UTC by Jonathan Druart
Modified:	2021-10-14 10:06 UTC (History)
CC List:	4 users (show)

See Also:	25452
Change sponsored?:	---
Patch complexity:	Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:	This internal change simplifies the code for editing and displaying patron address and contact information. It removes duplicated code, reducing potential problems when the code is changed in the future. For example, there are currently 5 include files for each value of the address format (us, de, fr) with the code duplicated for each language. The change reduces the need for 5*3 files to 5 files.
Version(s) released in:	20.11.00, 20.05.01
Circulation function:

Attachments
Bug 25070: Centralize member-alt-address-style (35.10 KB, patch) 2020-04-06 13:16 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-contact-style (30.86 KB, patch) 2020-04-06 13:16 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-address-style (14.28 KB, patch) 2020-04-06 13:16 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-alt-address-style (10.18 KB, patch) 2020-04-06 13:16 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-main-address-style (25.39 KB, patch) 2020-04-06 13:16 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-address-style (35.10 KB, patch) 2020-04-07 08:52 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-contact-style (30.86 KB, patch) 2020-04-07 08:52 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-address-style (14.28 KB, patch) 2020-04-07 08:52 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-alt-address-style (10.18 KB, patch) 2020-04-07 08:52 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-main-address-style (25.39 KB, patch) 2020-04-07 08:52 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-address-style (35.10 KB, patch) 2020-04-07 10:03 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-contact-style (30.86 KB, patch) 2020-04-07 10:03 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-address-style (14.28 KB, patch) 2020-04-07 10:03 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-alt-address-style (10.18 KB, patch) 2020-04-07 10:03 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-main-address-style (25.39 KB, patch) 2020-04-07 10:03 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-address-style (35.10 KB, patch) 2020-05-08 09:33 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-contact-style (30.86 KB, patch) 2020-05-08 09:33 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-address-style (14.28 KB, patch) 2020-05-08 09:33 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-alt-address-style (10.18 KB, patch) 2020-05-08 09:33 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-main-address-style (25.45 KB, patch) 2020-05-08 09:33 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-address-style (35.10 KB, patch) 2020-05-08 11:06 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-contact-style (30.86 KB, patch) 2020-05-08 11:06 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-address-style (14.28 KB, patch) 2020-05-08 11:06 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-alt-address-style (10.18 KB, patch) 2020-05-08 11:06 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-main-address-style (26.35 KB, patch) 2020-05-08 11:06 UTC, Jonathan Druart	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-address-style (35.18 KB, patch) 2020-05-11 09:25 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-contact-style (30.94 KB, patch) 2020-05-11 09:25 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-address-style (14.38 KB, patch) 2020-05-11 09:25 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-alt-address-style (10.26 KB, patch) 2020-05-11 09:25 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-main-address-style (26.43 KB, patch) 2020-05-11 09:25 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-address-style (35.24 KB, patch) 2020-06-11 12:42 UTC, Katrin Fischer	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-alt-contact-style (30.99 KB, patch) 2020-06-11 12:42 UTC, Katrin Fischer	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-address-style (14.43 KB, patch) 2020-06-11 12:42 UTC, Katrin Fischer	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-display-alt-address-style (10.31 KB, patch) 2020-06-11 12:42 UTC, Katrin Fischer	Details \| Diff \| Splinter Review
Bug 25070: Centralize member-main-address-style (26.49 KB, patch) 2020-06-11 12:42 UTC, Katrin Fischer	Details \| Diff \| Splinter Review
Bug 25070: (QA follow-up) Fix capitalization: Street Number (2.03 KB, patch) 2020-06-11 12:43 UTC, Katrin Fischer	Details \| Diff \| Splinter Review
Show Obsolete (30) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Jonathan Druart 2020-04-06 12:54:15 UTC

There are 5 includes files per value of AddressFormat (us, de, fr). The code is duplicated for each language, the only thing that changes is the order.

A refactoring is needed to avoid further regressions.

Comment 1 Jonathan Druart 2020-04-06 13:16:27 UTC

Created attachment 102439 [details] [review]
Bug 25070: Centralize member-alt-address-style

There are 5 includes files per value of AddressFormat (us, de, fr).
The code is duplicated for each language, the only thing that changes is the order.

Wwith this patchset we are going to refactore them to avoid further regressions.

The idea is to remove all code that is duplicated and remove the need of
having 5*3 files. We are going to limit them to 5 files, 1 per use.

Note that we could go even further, for instance
member-display-alt-address-style and member-display-address-style are
quite identical.

Test plan (for the whole patchset):
- Create a patron with all the address, contact and alternate info filled
- Edit them and save
- Look at the patron's info (top left) in the patron module, and the
patron detail view.
- Play with the different values of AddressFormat
The order and the info must be displayed identically before and after
this patchset. No change is expected.

Comment 2 Jonathan Druart 2020-04-06 13:16:30 UTC

Created attachment 102440 [details] [review]
Bug 25070: Centralize member-alt-contact-style

Comment 3 Jonathan Druart 2020-04-06 13:16:33 UTC

Created attachment 102441 [details] [review]
Bug 25070: Centralize member-display-address-style

Comment 4 Jonathan Druart 2020-04-06 13:16:36 UTC

Created attachment 102442 [details] [review]
Bug 25070: Centralize member-display-alt-address-style

Comment 5 Jonathan Druart 2020-04-06 13:16:39 UTC

Created attachment 102443 [details] [review]
Bug 25070: Centralize member-main-address-style

Comment 6 Jonathan Druart 2020-04-07 08:52:09 UTC

Created attachment 102477 [details] [review]
Bug 25070: Centralize member-alt-address-style

There are 5 includes files per value of AddressFormat (us, de, fr).
The code is duplicated for each language, the only thing that changes is the order.

Wwith this patchset we are going to refactore them to avoid further regressions.

The idea is to remove all code that is duplicated and remove the need of
having 5*3 files. We are going to limit them to 5 files, 1 per use.

Note that we could go even further, for instance
member-display-alt-address-style and member-display-address-style are
quite identical.

Test plan (for the whole patchset):
- Create a patron with all the address, contact and alternate info filled
- Edit them and save
- Look at the patron's info (top left) in the patron module, and the
patron detail view.
- Play with the different values of AddressFormat
The order and the info must be displayed identically before and after
this patchset. No change is expected.

Comment 7 Jonathan Druart 2020-04-07 08:52:12 UTC

Created attachment 102478 [details] [review]
Bug 25070: Centralize member-alt-contact-style

Comment 8 Jonathan Druart 2020-04-07 08:52:15 UTC

Created attachment 102479 [details] [review]
Bug 25070: Centralize member-display-address-style

Comment 9 Jonathan Druart 2020-04-07 08:52:18 UTC

Created attachment 102480 [details] [review]
Bug 25070: Centralize member-display-alt-address-style

Comment 10 Jonathan Druart 2020-04-07 08:52:22 UTC

Created attachment 102481 [details] [review]
Bug 25070: Centralize member-main-address-style

Comment 11 Jonathan Druart 2020-04-07 10:03:28 UTC

Created attachment 102487 [details] [review]
Bug 25070: Centralize member-alt-address-style

There are 5 includes files per value of AddressFormat (us, de, fr).
The code is duplicated for each language, the only thing that changes is the order.

Wwith this patchset we are going to refactore them to avoid further regressions.

The idea is to remove all code that is duplicated and remove the need of
having 5*3 files. We are going to limit them to 5 files, 1 per use.

Note that we could go even further, for instance
member-display-alt-address-style and member-display-address-style are
quite identical.

Test plan (for the whole patchset):
- Create a patron with all the address, contact and alternate info filled
- Edit them and save
- Look at the patron's info (top left) in the patron module, and the
patron detail view.
- Play with the different values of AddressFormat
The order and the info must be displayed identically before and after
this patchset. No change is expected.

Comment 12 Jonathan Druart 2020-04-07 10:03:31 UTC

Created attachment 102488 [details] [review]
Bug 25070: Centralize member-alt-contact-style

Comment 13 Jonathan Druart 2020-04-07 10:03:35 UTC

Created attachment 102489 [details] [review]
Bug 25070: Centralize member-display-address-style

Comment 14 Jonathan Druart 2020-04-07 10:03:38 UTC

Created attachment 102490 [details] [review]
Bug 25070: Centralize member-display-alt-address-style

Comment 15 Jonathan Druart 2020-04-07 10:03:41 UTC

Created attachment 102491 [details] [review]
Bug 25070: Centralize member-main-address-style

Comment 16 David Nind 2020-05-08 06:22:43 UTC

Patch doesn't apply 8-(

root@34d7e7da5f3f:koha(bz25070)$ git bz apply 25070
Bug 25070 - Include files to display address and contact must be refactored

102487 - Bug 25070: Centralize member-alt-address-style
102488 - Bug 25070: Centralize member-alt-contact-style
102489 - Bug 25070: Centralize member-display-address-style
102490 - Bug 25070: Centralize member-display-alt-address-style
102491 - Bug 25070: Centralize member-main-address-style

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 25070: Centralize member-alt-address-style
Applying: Bug 25070: Centralize member-alt-contact-style
Applying: Bug 25070: Centralize member-display-address-style
Applying: Bug 25070: Centralize member-display-alt-address-style
Applying: Bug 25070: Centralize member-main-address-style
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-de.inc
M	koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-fr.inc
M	koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-us.inc
M	koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
CONFLICT (modify/delete): koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-us.inc deleted in Bug 25070: Centralize member-main-address-style and modified in HEAD. Version HEAD of koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-us.inc left in tree.
CONFLICT (modify/delete): koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-fr.inc deleted in Bug 25070: Centralize member-main-address-style and modified in HEAD. Version HEAD of koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-fr.inc left in tree.
CONFLICT (modify/delete): koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-de.inc deleted in Bug 25070: Centralize member-main-address-style and modified in HEAD. Version HEAD of koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-de.inc left in tree.
error: Failed to merge in the changes.
Patch failed at 0001 Bug 25070: Centralize member-main-address-style

Comment 17 Jonathan Druart 2020-05-08 09:33:23 UTC

Created attachment 104562 [details] [review]
Bug 25070: Centralize member-alt-address-style

There are 5 includes files per value of AddressFormat (us, de, fr).
The code is duplicated for each language, the only thing that changes is the order.

Wwith this patchset we are going to refactore them to avoid further regressions.

The idea is to remove all code that is duplicated and remove the need of
having 5*3 files. We are going to limit them to 5 files, 1 per use.

Note that we could go even further, for instance
member-display-alt-address-style and member-display-address-style are
quite identical.

Test plan (for the whole patchset):
- Create a patron with all the address, contact and alternate info filled
- Edit them and save
- Look at the patron's info (top left) in the patron module, and the
patron detail view.
- Play with the different values of AddressFormat
The order and the info must be displayed identically before and after
this patchset. No change is expected.

Comment 18 Jonathan Druart 2020-05-08 09:33:26 UTC

Created attachment 104563 [details] [review]
Bug 25070: Centralize member-alt-contact-style

Comment 19 Jonathan Druart 2020-05-08 09:33:29 UTC

Created attachment 104564 [details] [review]
Bug 25070: Centralize member-display-address-style

Comment 20 Jonathan Druart 2020-05-08 09:33:32 UTC

Created attachment 104565 [details] [review]
Bug 25070: Centralize member-display-alt-address-style

Comment 21 Jonathan Druart 2020-05-08 09:33:36 UTC

Created attachment 104566 [details] [review]
Bug 25070: Centralize member-main-address-style

Comment 22 David Nind 2020-05-08 10:40:10 UTC

I'm getting an error when I go to add a new patron - hope this enough of the error trace:

Template process failed: file error - member-alt-contact-style-us: not found at /kohadevbox/koha/C4/Templates.pm line 122
in C4::Templates::output at /kohadevbox/koha/C4/Templates.pm line 122
  119: 
  120:     my $data;
  121:     binmode( STDOUT, ":utf8" );
  122:     $template->process( $self->filename, $vars, \$data )
  123:       || die "Template process failed: ", $template->error();
  124:     return $data;
  125: }
Show function arguments

in (eval) at /kohadevbox/koha/members/memberentry.pl line 86

Comment 23 Jonathan Druart 2020-05-08 11:01:07 UTC

Hum, I reused patches from a local branch, I should have not apparently :-/

Comment 24 Jonathan Druart 2020-05-08 11:06:20 UTC

Created attachment 104581 [details] [review]
Bug 25070: Centralize member-alt-address-style

There are 5 includes files per value of AddressFormat (us, de, fr).
The code is duplicated for each language, the only thing that changes is the order.

Wwith this patchset we are going to refactore them to avoid further regressions.

The idea is to remove all code that is duplicated and remove the need of
having 5*3 files. We are going to limit them to 5 files, 1 per use.

Note that we could go even further, for instance
member-display-alt-address-style and member-display-address-style are
quite identical.

Test plan (for the whole patchset):
- Create a patron with all the address, contact and alternate info filled
- Edit them and save
- Look at the patron's info (top left) in the patron module, and the
patron detail view.
- Play with the different values of AddressFormat
The order and the info must be displayed identically before and after
this patchset. No change is expected.

Comment 25 Jonathan Druart 2020-05-08 11:06:24 UTC

Created attachment 104582 [details] [review]
Bug 25070: Centralize member-alt-contact-style

Comment 26 Jonathan Druart 2020-05-08 11:06:27 UTC

Created attachment 104583 [details] [review]
Bug 25070: Centralize member-display-address-style

Comment 27 Jonathan Druart 2020-05-08 11:06:31 UTC

Created attachment 104584 [details] [review]
Bug 25070: Centralize member-display-alt-address-style

Comment 28 Jonathan Druart 2020-05-08 11:06:34 UTC

Created attachment 104585 [details] [review]
Bug 25070: Centralize member-main-address-style

Comment 29 Jonathan Druart 2020-05-08 11:08:42 UTC

Sorry David, it should be ok now.

Comment 30 David Nind 2020-05-08 21:39:39 UTC

(In reply to Jonathan Druart from comment #29)
> Sorry David, it should be ok now.

Thanks Jonathan - it all works now!

Two things I noted:

1) Alternative address section: the email address displays as empty on the patron details page, even though it has a value.

2) Alternative contact section: the address displays with each field listed, rather than as an address block like the contact information and alternative address sections.

As both of these reflect the current behaviour, I'm not sure whether you want to fix these in this bug or do separately.

Apart from those two points, everything seems to work and I'm happy to sign off.

Comment 31 Jonathan Druart 2020-05-11 08:54:03 UTC

We should fix them on a separate bug report, to make the bug fixes easily backportable.

Comment 32 Jonathan Druart 2020-05-11 09:03:12 UTC

(In reply to David Nind from comment #30)

I have fixed:

> 1) Alternative address section: the email address displays as empty on the
> patron details page, even though it has a value.

on bug 25452.

I let you open a bug report for the second remark ;)

Comment 33 David Nind 2020-05-11 09:25:35 UTC

Created attachment 104659 [details] [review]
Bug 25070: Centralize member-alt-address-style

There are 5 includes files per value of AddressFormat (us, de, fr).
The code is duplicated for each language, the only thing that changes is the order.

Wwith this patchset we are going to refactore them to avoid further regressions.

The idea is to remove all code that is duplicated and remove the need of
having 5*3 files. We are going to limit them to 5 files, 1 per use.

Note that we could go even further, for instance
member-display-alt-address-style and member-display-address-style are
quite identical.

Test plan (for the whole patchset):
- Create a patron with all the address, contact and alternate info filled
- Edit them and save
- Look at the patron's info (top left) in the patron module, and the
patron detail view.
- Play with the different values of AddressFormat
The order and the info must be displayed identically before and after
this patchset. No change is expected.

Signed-off-by: David Nind <david@davidnind.com>

Comment 34 David Nind 2020-05-11 09:25:39 UTC

Created attachment 104660 [details] [review]
Bug 25070: Centralize member-alt-contact-style

Signed-off-by: David Nind <david@davidnind.com>

Comment 35 David Nind 2020-05-11 09:25:43 UTC

Created attachment 104661 [details] [review]
Bug 25070: Centralize member-display-address-style

Signed-off-by: David Nind <david@davidnind.com>

Comment 36 David Nind 2020-05-11 09:25:46 UTC

Created attachment 104662 [details] [review]
Bug 25070: Centralize member-display-alt-address-style

Signed-off-by: David Nind <david@davidnind.com>

Comment 37 David Nind 2020-05-11 09:25:51 UTC

Created attachment 104663 [details] [review]
Bug 25070: Centralize member-main-address-style

Signed-off-by: David Nind <david@davidnind.com>

Comment 38 David Nind 2020-05-12 11:13:41 UTC

(In reply to Jonathan Druart from comment #32)
> I let you open a bug report for the second remark ;)

Bug 25463 opened.

Comment 39 Katrin Fischer 2020-06-11 12:42:39 UTC

Created attachment 105745 [details] [review]
Bug 25070: Centralize member-alt-address-style

There are 5 includes files per value of AddressFormat (us, de, fr).
The code is duplicated for each language, the only thing that changes is the order.

Wwith this patchset we are going to refactore them to avoid further regressions.

The idea is to remove all code that is duplicated and remove the need of
having 5*3 files. We are going to limit them to 5 files, 1 per use.

Note that we could go even further, for instance
member-display-alt-address-style and member-display-address-style are
quite identical.

Test plan (for the whole patchset):
- Create a patron with all the address, contact and alternate info filled
- Edit them and save
- Look at the patron's info (top left) in the patron module, and the
patron detail view.
- Play with the different values of AddressFormat
The order and the info must be displayed identically before and after
this patchset. No change is expected.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Comment 40 Katrin Fischer 2020-06-11 12:42:44 UTC

Created attachment 105746 [details] [review]
Bug 25070: Centralize member-alt-contact-style

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Comment 41 Katrin Fischer 2020-06-11 12:42:48 UTC

Created attachment 105747 [details] [review]
Bug 25070: Centralize member-display-address-style

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Comment 42 Katrin Fischer 2020-06-11 12:42:53 UTC

Created attachment 105748 [details] [review]
Bug 25070: Centralize member-display-alt-address-style

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Comment 43 Katrin Fischer 2020-06-11 12:42:58 UTC

Created attachment 105749 [details] [review]
Bug 25070: Centralize member-main-address-style

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Comment 44 Katrin Fischer 2020-06-11 12:43:02 UTC

Created attachment 105750 [details] [review]
Bug 25070: (QA follow-up) Fix capitalization: Street Number

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Comment 45 Jonathan Druart 2020-06-15 09:59:28 UTC

Pushed to master for 20.11, thanks to everybody involved!

Comment 46 Lucas Gass (lukeg) 2020-06-17 23:37:46 UTC

backported to 20.05.x for 20.05.01

Comment 47 Aleisha Amohia 2020-06-22 04:40:07 UTC

enhancement, not backported to 19.11.x