Created attachment 175355 [details] [review] Bug 38663: [DONT PUSH] Preparation: Change record_id to varchar: DBIC
Created attachment 175356 [details] [review] Bug 38663: Preparation: Change record_id to varchar: database record_if in the additional_field_values is an int(11) This works for most cases, but in the branches case, the id column is actually a varchar (branchcode). This patch updates the column to accomodate for this.
Created attachment 175357 [details] [review] Bug 38663: Add libraries table to additional fields admin screen
Created attachment 175358 [details] [review] Bug 38663: DBIC: Add AdditionalFields to Library
Created attachment 175359 [details] [review] Bug 38663: Add AdditionalFields to Library Test plan, k-t-d: 1) Add a new additional field to 'branches': <staff_url>/cgi-bin/koha/admin/additional-fields.pl?tablename=branches 2) Click 'New field'. Add a name. Click Save. 3) Repeat step 2 but repeatable. Repeat again for 2 more of AV category (one repeatable, one not repeatable) 4) Edit a library: <staff_url>/cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=CPL 5) Input various values in the different additional fields. Click 'Submit' 6) Visit the detail page of the library and verify the additional fields data is displayed correctly: <staff_url>/cgi-bin/koha/admin/branches.pl?op=view&branchcode=CPL
Created attachment 175370 [details] [review] Bug 38663: Add AdditionalFields to Library Test plan, k-t-d: 1) Add a new additional field to 'branches': <staff_url>/cgi-bin/koha/admin/additional-fields.pl?tablename=branches 2) Click 'New field'. Add a name. Click Save. 3) Repeat step 2 but repeatable. Repeat again for 2 more of AV category (one repeatable, one not repeatable) 4) Edit a library: <staff_url>/cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=CPL 5) Input various values in the different additional fields. Click 'Submit' 6) Visit the detail page of the library and verify the additional fields data is displayed correctly: <staff_url>/cgi-bin/koha/admin/branches.pl?op=view&branchcode=CPL
It's a good question. Historically, I think we'd say branches should be converted to have a branch_id. Once upon a time, reserve_id didn't exist (see Bug 7065), but that was a long time ago and a different set of circumstances... (e.g. it didn't have a primary key at all) Technically, I suppose we could keep branchcode as the PK, and maybe just add an autoincrementing branch_id int(11) with a UNIQUE constraint. I don't know enough about additional_field_values to know if that would be workable or not though
One day, it would be cool to switch from autoincrementing integers to UUIDs, which are less prone to enumeration attacks, but that's a whole other story...
Created attachment 175757 [details] [review] Bug 38663: [DONT PUSH] Preparation: Change record_id to varchar: DBIC Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 175758 [details] [review] Bug 38663: Preparation: Change record_id to varchar: database record_if in the additional_field_values is an int(11) This works for most cases, but in the branches case, the id column is actually a varchar (branchcode). This patch updates the column to accomodate for this. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 175759 [details] [review] Bug 38663: Add libraries table to additional fields admin screen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 175760 [details] [review] Bug 38663: DBIC: Add AdditionalFields to Library Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 175761 [details] [review] Bug 38663: Add AdditionalFields to Library Test plan, k-t-d: 1) Add a new additional field to 'branches': <staff_url>/cgi-bin/koha/admin/additional-fields.pl?tablename=branches 2) Click 'New field'. Add a name. Click Save. 3) Repeat step 2 but repeatable. Repeat again for 2 more of AV category (one repeatable, one not repeatable) 4) Edit a library: <staff_url>/cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=CPL 5) Input various values in the different additional fields. Click 'Submit' 6) Visit the detail page of the library and verify the additional fields data is displayed correctly: <staff_url>/cgi-bin/koha/admin/branches.pl?op=view&branchcode=CPL Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 175762 [details] [review] Bug 38663: (follow-up) Hide additional fields input appropriately This patch hides the additional fields input on the library add/edit page when there are no additional fields defined for libraries. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
The patches no longer apply - sha1/fake ancestor error: git bz apply 38663 Bug 38663 - Add additional fields to libraries 175757 - Bug 38663: [DONT PUSH] Preparation: Change record_id to varchar: DBIC 175758 - Bug 38663: Preparation: Change record_id to varchar: database 175759 - Bug 38663: Add libraries table to additional fields admin screen 175760 - Bug 38663: DBIC: Add AdditionalFields to Library 175761 - Bug 38663: Add AdditionalFields to Library 175762 - Bug 38663: (follow-up) Hide additional fields input appropriately Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 38663: [DONT PUSH] Preparation: Change record_id to varchar: DBIC Applying: Bug 38663: Preparation: Change record_id to varchar: database Applying: Bug 38663: Add libraries table to additional fields admin screen error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/admin/additional-fields.tt). error: could not build fake ancestor Patch failed at 0001 Bug 38663: Add libraries table to additional fields admin screen
(In reply to David Nind from comment #15) > The patches no longer apply - sha1/fake ancestor error: Hi! Can't reproduce, sorry! Just tested twice on up to date with main branch, it applies. Can you please double-check? Thanks!
(In reply to Pedro Amorim from comment #16) > (In reply to David Nind from comment #15) > > The patches no longer apply - sha1/fake ancestor error: > > Hi! Can't reproduce, sorry! Just tested twice on up to date with main > branch, it applies. Can you please double-check? Thanks! Yeah, I have the same problem as David N. You probably won't have that problem, Pedro, as you'll have the git ancestor in your repo while we don't. (In this case, it looks like 62e85cc3eab is the ancestor that you'll have which isn't in the upstream main branch. It might be interesting to see what that hash resolves to in your repo using "git show".) If you can fetch the latest upstream and then rebase against that main branch, that should fix the problem for us.
(In reply to David Cook from comment #17) > If you can fetch the latest upstream and then rebase against that main > branch, that should fix the problem for us. The rebase is necessary, because if you did "git checkout -b bug_38663 origin/main" and then did "git bz apply 38663" it probably would work for you, because you have that ancestor elsewhere in your repo referenced by some other branch, tag, or just a dangling reference that hasn't been garbage collected yet.
Created attachment 176214 [details] [review] Bug 38663: [DONT PUSH] Preparation: Change record_id to varchar: DBIC Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176215 [details] [review] Bug 38663: Preparation: Change record_id to varchar: database record_if in the additional_field_values is an int(11) This works for most cases, but in the branches case, the id column is actually a varchar (branchcode). This patch updates the column to accomodate for this. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176216 [details] [review] Bug 38663: Add libraries table to additional fields admin screen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176217 [details] [review] Bug 38663: DBIC: Add AdditionalFields to Library Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176218 [details] [review] Bug 38663: Add AdditionalFields to Library Test plan, k-t-d: 1) Add a new additional field to 'branches': <staff_url>/cgi-bin/koha/admin/additional-fields.pl?tablename=branches 2) Click 'New field'. Add a name. Click Save. 3) Repeat step 2 but repeatable. Repeat again for 2 more of AV category (one repeatable, one not repeatable) 4) Edit a library: <staff_url>/cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=CPL 5) Input various values in the different additional fields. Click 'Submit' 6) Visit the detail page of the library and verify the additional fields data is displayed correctly: <staff_url>/cgi-bin/koha/admin/branches.pl?op=view&branchcode=CPL Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176219 [details] [review] Bug 38663: (follow-up) Hide additional fields input appropriately This patch hides the additional fields input on the library add/edit page when there are no additional fields defined for libraries. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176220 [details] [review] Bug 38663: [DONT PUSH] Preparation: Change record_id to varchar: DBIC Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 176221 [details] [review] Bug 38663: Preparation: Change record_id to varchar: database record_if in the additional_field_values is an int(11) This works for most cases, but in the branches case, the id column is actually a varchar (branchcode). This patch updates the column to accomodate for this. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 176222 [details] [review] Bug 38663: Add libraries table to additional fields admin screen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 176223 [details] [review] Bug 38663: DBIC: Add AdditionalFields to Library Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 176224 [details] [review] Bug 38663: Add AdditionalFields to Library Test plan, k-t-d: 1) Add a new additional field to 'branches': <staff_url>/cgi-bin/koha/admin/additional-fields.pl?tablename=branches 2) Click 'New field'. Add a name. Click Save. 3) Repeat step 2 but repeatable. Repeat again for 2 more of AV category (one repeatable, one not repeatable) 4) Edit a library: <staff_url>/cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=CPL 5) Input various values in the different additional fields. Click 'Submit' 6) Visit the detail page of the library and verify the additional fields data is displayed correctly: <staff_url>/cgi-bin/koha/admin/branches.pl?op=view&branchcode=CPL Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 176225 [details] [review] Bug 38663: (follow-up) Hide additional fields input appropriately This patch hides the additional fields input on the library add/edit page when there are no additional fields defined for libraries. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Added my signoff. Patches apply, works as per the test plan. Testing notes (using KTD): 1. Apply the patches. 2. Update the database: updatedatabase 3. Run DBIC: dbic 4. Restart everything: restart_all
Everything works find and QA tool passes. Implementation is the same as for other additional fields so it makes sense to me. Concerning the code I'm wondering why there is a double if here ? > [% IF CAN_user_parameters_manage_libraries %] > <h3>Admin</h3> > <ul> > [% IF CAN_user_parameters_manage_libraries %] > [% WRAPPER table_option value="branches" %]<span>Libraries</span>[% END %] > [% END %] > </ul> > [% END %] I also tested other values to verify nothing is broken by record_id type changes, looks fine.
(In reply to Baptiste Wojtkowski (bwoj) from comment #32) > Everything works find and QA tool passes. Implementation is the same as for > other additional fields so it makes sense to me. > > Concerning the code I'm wondering why there is a double if here ? > > > [% IF CAN_user_parameters_manage_libraries %] > > <h3>Admin</h3> > > <ul> > > [% IF CAN_user_parameters_manage_libraries %] > > [% WRAPPER table_option value="branches" %]<span>Libraries</span>[% END %] > > [% END %] > > </ul> > > [% END %] > > I also tested other values to verify nothing is broken by record_id type > changes, looks fine. Hey, that's a good observation. I opted for this pattern approach as the initial IF should be an OR of all child IFs. It's just so happens that in this case there is only 1 child IF, but more entries may be added under 'Admin' in the future. To better understand what I mean here see the 'Acquisitions' section of the code right above it. Having said that, this same pattern is not present on the other subsequent IFs below so, yeah, up for discussion. I agree with either way and don't feel strongly about it, but I believe that was the reason.
Created attachment 176296 [details] [review] Bug 38663: (follow-up) Heading - Admin to Administration Signed-off-by: David Nind <david@davidnind.com>
I understand the reason when you explain it, but I feel like double check will be weird in the code as long as we don't add multiple options, so I'd rather say let's not double check as long as we don't need.
(In reply to Baptiste Wojtkowski (bwoj) from comment #35) > I understand the reason when you explain it, but I feel like double check > will be weird in the code as long as we don't add multiple options, so I'd > rather say let's not double check as long as we don't need. Makes sense. I agree.
Created attachment 176781 [details] [review] Bug 38663: [DONT PUSH] Preparation: Change record_id to varchar: DBIC Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176782 [details] [review] Bug 38663: Preparation: Change record_id to varchar: database record_if in the additional_field_values is an int(11) This works for most cases, but in the branches case, the id column is actually a varchar (branchcode). This patch updates the column to accomodate for this. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176783 [details] [review] Bug 38663: Add libraries table to additional fields admin screen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176784 [details] [review] Bug 38663: DBIC: Add AdditionalFields to Library Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176785 [details] [review] Bug 38663: Add AdditionalFields to Library Test plan, k-t-d: 1) Add a new additional field to 'branches': <staff_url>/cgi-bin/koha/admin/additional-fields.pl?tablename=branches 2) Click 'New field'. Add a name. Click Save. 3) Repeat step 2 but repeatable. Repeat again for 2 more of AV category (one repeatable, one not repeatable) 4) Edit a library: <staff_url>/cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=CPL 5) Input various values in the different additional fields. Click 'Submit' 6) Visit the detail page of the library and verify the additional fields data is displayed correctly: <staff_url>/cgi-bin/koha/admin/branches.pl?op=view&branchcode=CPL Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176786 [details] [review] Bug 38663: (follow-up) Hide additional fields input appropriately This patch hides the additional fields input on the library add/edit page when there are no additional fields defined for libraries. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176787 [details] [review] Bug 38663: (follow-up) Heading - Admin to Administration Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176788 [details] [review] Bug 38663: (QA follow-up) Removed superflous IF As requested, I've removed the superflous IF statement in the template.. I'll likely be adding it back in as part of the next patch series, but it makes the history clear this way. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Fancy finishing up the QA here and adding your stamp Baptiste :)
Created attachment 176797 [details] [review] Bug 38663: [DONT PUSH] Preparation: Change record_id to varchar: DBIC Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176799 [details] [review] Bug 38663: Preparation: Change record_id to varchar: database record_if in the additional_field_values is an int(11) This works for most cases, but in the branches case, the id column is actually a varchar (branchcode). This patch updates the column to accomodate for this. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176800 [details] [review] Bug 38663: Add libraries table to additional fields admin screen Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176801 [details] [review] Bug 38663: DBIC: Add AdditionalFields to Library Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176802 [details] [review] Bug 38663: Add AdditionalFields to Library Test plan, k-t-d: 1) Add a new additional field to 'branches': <staff_url>/cgi-bin/koha/admin/additional-fields.pl?tablename=branches 2) Click 'New field'. Add a name. Click Save. 3) Repeat step 2 but repeatable. Repeat again for 2 more of AV category (one repeatable, one not repeatable) 4) Edit a library: <staff_url>/cgi-bin/koha/admin/branches.pl?op=add_form&branchcode=CPL 5) Input various values in the different additional fields. Click 'Submit' 6) Visit the detail page of the library and verify the additional fields data is displayed correctly: <staff_url>/cgi-bin/koha/admin/branches.pl?op=view&branchcode=CPL Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176803 [details] [review] Bug 38663: (follow-up) Hide additional fields input appropriately This patch hides the additional fields input on the library add/edit page when there are no additional fields defined for libraries. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176804 [details] [review] Bug 38663: (follow-up) Heading - Admin to Administration Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Created attachment 176805 [details] [review] Bug 38663: (QA follow-up) Removed superflous IF As requested, I've removed the superflous IF statement in the template.. I'll likely be adding it back in as part of the next patch series, but it makes the history clear this way. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>