Other parts of Koha that need to render this should not be repeating code but instead make use of established DRY logic.
Created attachment 182739 [details] [review] Bug 39971: Move form rendering to reusable tt block
Created attachment 182740 [details] [review] Bug 39971: Move patron_attributes_form method to a class This makes it reusable, documentable, and testable. This is changing code without adding a new feature or fixing a bug. The first test plan should be to just make sure that the existing functionality is kept intact, i.e. rendering and saving the patron attribute types in the form. As to why we need this, we're maintaining the ILL Actions plugin where we have a feature to quickly add a patron. This is designed to be as minimal as possible, but if there are any mandatory patron attribute types, those need to be considered. To test this: 1) Apply patches and enable ILLModule syspref. 2) Install the plugin: https://github.com/openfifth/koha-plugin-ill-actions/releases/tag/v2.4.0 3) Add a mandatory patron attribute type, you can edit the existing one in k-t-d, visit: <staff_url>/cgi-bin/koha/admin/patron-attr-types.pl?op=edit_attribute_type&code=SHOW_BCODE 5) Tick the 'Staff interface mandatory' checkbox 6) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 7) Notice there's a '+Add new user' link next to the cardnumber input at the bottom of the form. Click that. 8) Notice there's a 'Additional attributes and identifiers' section. This is only possible (without writing it again) because of the patches here.
Created attachment 182741 [details] [review] Bug 39971: Add POD
Created attachment 182742 [details] [review] Bug 39971: Add query param
Created attachment 183087 [details] [review] Bug 39971: Move form rendering to reusable tt block Signed-off-by: David Nind <david@davidnind.com>
Created attachment 183088 [details] [review] Bug 39971: Move patron_attributes_form method to a class This makes it reusable, documentable, and testable. This is changing code without adding a new feature or fixing a bug. The first test plan should be to just make sure that the existing functionality is kept intact, i.e. rendering and saving the patron attribute types in the form. As to why we need this, we're maintaining the ILL Actions plugin where we have a feature to quickly add a patron. This is designed to be as minimal as possible, but if there are any mandatory patron attribute types, those need to be considered. To test this: 1) Apply patches and enable ILLModule syspref. 2) Install the plugin: https://github.com/openfifth/koha-plugin-ill-actions/releases/tag/v2.4.0 3) Add a mandatory patron attribute type, you can edit the existing one in k-t-d, visit: <staff_url>/cgi-bin/koha/admin/patron-attr-types.pl?op=edit_attribute_type&code=SHOW_BCODE 5) Tick the 'Staff interface mandatory' checkbox 6) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 7) Notice there's a '+Add new user' link next to the cardnumber input at the bottom of the form. Click that. 8) Notice there's a 'Additional attributes and identifiers' section. This is only possible (without writing it again) because of the patches here. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 183089 [details] [review] Bug 39971: Add POD Signed-off-by: David Nind <david@davidnind.com>
Created attachment 183090 [details] [review] Bug 39971: Add query param Signed-off-by: David Nind <david@davidnind.com>
Things worked as per the test plan. In this case, a new patron wasn't created. However, I assume that is because the plugin has to implement that part? (It also let you press submit even if you didn't add a mandatory field.)
Created attachment 189077 [details] [review] Bug 39971: Move form rendering to reusable tt block Signed-off-by: David Nind <david@davidnind.com>
Created attachment 189078 [details] [review] Bug 39971: Move patron_attributes_form method to a class This makes it reusable, documentable, and testable. This is changing code without adding a new feature or fixing a bug. The first test plan should be to just make sure that the existing functionality is kept intact, i.e. rendering and saving the patron attribute types in the form. As to why we need this, we're maintaining the ILL Actions plugin where we have a feature to quickly add a patron. This is designed to be as minimal as possible, but if there are any mandatory patron attribute types, those need to be considered. To test this: 1) Apply patches and enable ILLModule syspref. 2) Install the plugin: https://github.com/openfifth/koha-plugin-ill-actions/releases/tag/v2.4.0 3) Add a mandatory patron attribute type, you can edit the existing one in k-t-d, visit: <staff_url>/cgi-bin/koha/admin/patron-attr-types.pl?op=edit_attribute_type&code=SHOW_BCODE 5) Tick the 'Staff interface mandatory' checkbox 6) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 7) Notice there's a '+Add new user' link next to the cardnumber input at the bottom of the form. Click that. 8) Notice there's a 'Additional attributes and identifiers' section. This is only possible (without writing it again) because of the patches here. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 189079 [details] [review] Bug 39971: Add POD Signed-off-by: David Nind <david@davidnind.com>
Created attachment 189080 [details] [review] Bug 39971: Add query param Signed-off-by: David Nind <david@davidnind.com>
Rebased on bug 40917
Created attachment 189125 [details] [review] Bug 39971: Move form rendering to reusable tt block Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 189126 [details] [review] Bug 39971: Move patron_attributes_form method to a class This makes it reusable, documentable, and testable. This is changing code without adding a new feature or fixing a bug. The first test plan should be to just make sure that the existing functionality is kept intact, i.e. rendering and saving the patron attribute types in the form. As to why we need this, we're maintaining the ILL Actions plugin where we have a feature to quickly add a patron. This is designed to be as minimal as possible, but if there are any mandatory patron attribute types, those need to be considered. To test this: 1) Apply patches and enable ILLModule syspref. 2) Install the plugin: https://github.com/openfifth/koha-plugin-ill-actions/releases/tag/v2.4.0 3) Add a mandatory patron attribute type, you can edit the existing one in k-t-d, visit: <staff_url>/cgi-bin/koha/admin/patron-attr-types.pl?op=edit_attribute_type&code=SHOW_BCODE 5) Tick the 'Staff interface mandatory' checkbox 6) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 7) Notice there's a '+Add new user' link next to the cardnumber input at the bottom of the form. Click that. 8) Notice there's a 'Additional attributes and identifiers' section. This is only possible (without writing it again) because of the patches here. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 189127 [details] [review] Bug 39971: Add POD Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 189128 [details] [review] Bug 39971: Add query param Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 189129 [details] [review] Bug 39971: (QA follow-up) Unit tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Thank you, Nick. I'm sorry, I realize now I wrongfully set this to PQA after rebase when I should've set it to 'Signed-off' as it was originally.
Created attachment 189130 [details] [review] Bug 39971: Move form rendering to reusable tt block Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 189131 [details] [review] Bug 39971: Move patron_attributes_form method to a class This makes it reusable, documentable, and testable. This is changing code without adding a new feature or fixing a bug. The first test plan should be to just make sure that the existing functionality is kept intact, i.e. rendering and saving the patron attribute types in the form. As to why we need this, we're maintaining the ILL Actions plugin where we have a feature to quickly add a patron. This is designed to be as minimal as possible, but if there are any mandatory patron attribute types, those need to be considered. To test this: 1) Apply patches and enable ILLModule syspref. 2) Install the plugin: https://github.com/openfifth/koha-plugin-ill-actions/releases/tag/v2.4.0 3) Add a mandatory patron attribute type, you can edit the existing one in k-t-d, visit: <staff_url>/cgi-bin/koha/admin/patron-attr-types.pl?op=edit_attribute_type&code=SHOW_BCODE 5) Tick the 'Staff interface mandatory' checkbox 6) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 7) Notice there's a '+Add new user' link next to the cardnumber input at the bottom of the form. Click that. 8) Notice there's a 'Additional attributes and identifiers' section. This is only possible (without writing it again) because of the patches here. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 189132 [details] [review] Bug 39971: Add POD Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 189133 [details] [review] Bug 39971: Add query param Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 189134 [details] [review] Bug 39971: (QA follow-up) Unit tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>