Bug 15272 - aqbookseller addresses are combined into "physical address" field
Summary: aqbookseller addresses are combined into "physical address" field
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 21126 (view as bug list)
Depends on: 18177
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-01 21:18 UTC by Rocio Dressler
Modified: 2020-04-20 09:29 UTC (History)
5 users (show)

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


Attachments
Bug 15272: DBIC changes (1.82 KB, patch)
2020-03-17 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15272: DB changes (3.06 KB, patch)
2020-03-17 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15272: Merge aqbooksellers addressX columns (11.21 KB, patch)
2020-03-17 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15272: Update notice templates (1.60 KB, patch)
2020-03-17 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15272: Update notice templates for all languages (40.01 KB, patch)
2020-03-17 09:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15272: DBIC changes (1.88 KB, patch)
2020-04-03 12:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15272: DB changes (3.13 KB, patch)
2020-04-03 12:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15272: Merge aqbooksellers addressX columns (11.27 KB, patch)
2020-04-03 12:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15272: Update notice templates (1.67 KB, patch)
2020-04-03 12:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 15272: Update notice templates for all languages (40.07 KB, patch)
2020-04-03 12:52 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Rocio Dressler 2015-12-01 21:18:36 UTC
In the database, vendors can have text in up to 4 address fields.

In the staff client, these 4 addresses appear in the same area next to a label of "Physical address".

The addresses appear to belong to one field here, and are not separated by a space so the data looks bad.

Clicking on Edit allows you to edit the "Physical address" box (containing the combined data from these 4 fields).

If you modify the text in this box, and click save, you've modified the location of the data in the database.

From that point forward, the vendor only appears to have data in address1, and address2-address4 are set to NULL.
Comment 1 Katrin Fischer 2016-02-28 19:09:16 UTC
Hi Rocio,

I am sorry, but I have tried a few things and I am not sure what this bug is about. When I enter the addresses (postal and physical) on multiple lines they get saved to the database correctly afaikt. The postal address is split into multiple database fields while the physical address is stored in postal.
Editing works ok too - it displays as I entered it.

Can you try and describe the problem a bit more?
Comment 2 Owen Leonard 2019-11-21 18:31:35 UTC
Perhaps the problem is that the system parses the data entered into the textarea and splits it based on line breaks. If you put in more than 4 lines of data the last line will get discarded.

We should at the very least add a note to the field about the 4-line limit.
Comment 3 Jonathan Druart 2020-03-17 09:04:53 UTC
*** Bug 21126 has been marked as a duplicate of this bug. ***
Comment 4 Jonathan Druart 2020-03-17 09:57:49 UTC
Created attachment 100875 [details] [review]
Bug 15272: DBIC changes
Comment 5 Jonathan Druart 2020-03-17 09:57:52 UTC
Created attachment 100876 [details] [review]
Bug 15272: DB changes
Comment 6 Jonathan Druart 2020-03-17 09:57:55 UTC
Created attachment 100877 [details] [review]
Bug 15272: Merge aqbooksellers addressX columns

The physical address of a vendor is stored in 4 different DB columns:
address1, address2, address3 and address4
The are concatenated and split for storage/display purpose.

If you enter an address on more than 4 lines, the other lines are
truncated: data loss

To avoid such weird behavior, we could simplify the code and get more
flexibility (like address as long as you wish) merging those 4 columns
into 1, address

Test plan:
0. Do not apply the patches
1. Create a vendor and enter more than 4 lines in the physical address,
save
=> Notice that the lines has been stored in the 4 columns, and that you
lost the end of the address
2. Apply the patches and execute the updatedatabase script
3. Edit the vendor's address
=> Notice that the address is displayed correctly
4. Add more lines, save
=> Notice that the extra lines are kept
5. Generate a ACQCLAIM or ACQORDER notice for this vendor
=> Confirm that the address is displayed correctly in the generated
notice
Comment 7 Jonathan Druart 2020-03-17 09:57:59 UTC
Created attachment 100878 [details] [review]
Bug 15272: Update notice templates
Comment 8 Jonathan Druart 2020-03-17 09:58:02 UTC
Created attachment 100879 [details] [review]
Bug 15272: Update notice templates for all languages
Comment 9 Martin Renvoize 2020-04-03 12:52:27 UTC
Created attachment 102349 [details] [review]
Bug 15272: DBIC changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-04-03 12:52:30 UTC
Created attachment 102350 [details] [review]
Bug 15272: DB changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-04-03 12:52:33 UTC
Created attachment 102351 [details] [review]
Bug 15272: Merge aqbooksellers addressX columns

The physical address of a vendor is stored in 4 different DB columns:
address1, address2, address3 and address4
The are concatenated and split for storage/display purpose.

If you enter an address on more than 4 lines, the other lines are
truncated: data loss

To avoid such weird behavior, we could simplify the code and get more
flexibility (like address as long as you wish) merging those 4 columns
into 1, address

Test plan:
0. Do not apply the patches
1. Create a vendor and enter more than 4 lines in the physical address,
save
=> Notice that the lines has been stored in the 4 columns, and that you
lost the end of the address
2. Apply the patches and execute the updatedatabase script
3. Edit the vendor's address
=> Notice that the address is displayed correctly
4. Add more lines, save
=> Notice that the extra lines are kept
5. Generate a ACQCLAIM or ACQORDER notice for this vendor
=> Confirm that the address is displayed correctly in the generated
notice

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-04-03 12:52:36 UTC
Created attachment 102352 [details] [review]
Bug 15272: Update notice templates

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-04-03 12:52:40 UTC
Created attachment 102353 [details] [review]
Bug 15272: Update notice templates for all languages

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-04-03 12:52:58 UTC
Looks good and works well for me.. Signing off
Comment 15 Katrin Fischer 2020-04-18 22:24:01 UTC
I am not sure about this approach goes in the right direction.

I wonder if it wouldn't be better to split the information into multiple fields in the GUI instead, making it match how we handle addresses for patrons and libraries.

Pros:
- Allows more than 4 lines in addresses
- Doesn't lose data without notifying the user
- Makes physical and postal address consistent

Cons:
- Special handling is needed in HTML notices to keep line breaks
- Notices: Harder to parse the address if you want different formatting. Probably possible with TT, but much more complex. Existing notices will have be updated after update or break. 
- Reports: Existing reports will be broken and it will be much harder to report on specific information. Example: a report on delivery time of vendors in and outside the country.
- CSV export: People using serial letters or other ways to create order letters will have to adapt (this would directly affect us as well)
- Creates inconsistency in address handling with patrons and libraries

Usually we try to avoid creating a need for changes to be made manually after an update, but I don't think we could avoid that here (reports, notices, CSV profiles for basket exports etc.)

Some notes for further testing beyond the test plan:
- Export reports on vendor data
- CSV basket and basket group exports
- PDF basket group printing
- Vendors API

Found some more occurrences of address3 in the PDF templates:
Koha/pdfformat/layout2pages.pm:    $text->text($bookseller->address3);
Koha/pdfformat/layout2pagesde.pm:    $text->text($bookseller->address3);
Koha/pdfformat/layout3pagesfr.pm:    $text->text($bookseller->address3);
...

Failing for the last - but maybe we should discuss this a bit more? I hope, I didn't miss something important.
Comment 16 Jonathan Druart 2020-04-20 09:29:17 UTC
(In reply to Katrin Fischer from comment #15)
> Found some more occurrences of address3 in the PDF templates:
> Koha/pdfformat/layout2pages.pm:    $text->text($bookseller->address3);
> Koha/pdfformat/layout2pagesde.pm:    $text->text($bookseller->address3);
> Koha/pdfformat/layout3pagesfr.pm:    $text->text($bookseller->address3);

Will fix if we agree on the move.

> Failing for the last - but maybe we should discuss this a bit more? I hope,
> I didn't miss something important.

Switching to In Discussion then...