Description
Andrew Isherwood
2019-09-26 13:59:33 UTC
It has become increasingly apparent that using authorised values for this is not the most appropriate way forward. The number of modifications and additions to AV was increasing the scope of the work too much, it was clear that AV was not the correct place for this. Therefore, I am intending creating a table specifically for Patron Restrictions and building the management UI for it, which will be placed probably under "Koha administration" > "Patrons and circulation" Created attachment 93769 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Created attachment 93770 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Created attachment 93771 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Created attachment 93772 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Created attachment 93773 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Created attachment 93774 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Created attachment 93775 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Created attachment 93776 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Created attachment 93777 [details] [review] Bug 23681: Add unit tests Test plan: - Apply bug - Rebuild CSS => TEST: Observe a new syspref exists "PatronRestrictionTypes" - Enable syspref "PatronRestrictionTypes" - Navigate to your currently logged in patron's "details" page - Choose More > Set permissions => TEST: Observe there is a new permission under "Manage Koha system settings" called "Manage patron restrictions (manage_patron_restrictions)" - Ensure you are logged in as either a superlibrarian, or assign yourself "manage_patron_restrictions" - Navigate to "Koha administration" => TEST: Observe a new menu option under "Patrons and circulation" called "Patron restrictions" - Navigate to "Patron restrictions" => TEST: Observe that 4 restrictions exist "DISCHARGE", "MANUAL", "OVERDUES", "SUSPENSION" - Click "New restriction" - Enter a code and label for the new restriction => TEST: Observe that the code can only be entered in uppercase - Click "Save" => TEST: Observe that the new restriction is listed in the table - On the row containing the new restriction, click "Edit" => TEST: Observe that the label can be edited, but the code cannot - Return to the table => TEST: Observe that none of the pre-existing restrictions can be deleted - Navigate to the details for an existing patron - Click on the "Restrictions" tab at the bottom - Click "Add manual restriction" => TEST: In the "Type" select box, observe that all 5 restriction types are available for selection - Select a restriction type - Enter a comment - Optionally, choose an expiration date - Click "Add restriction" - Return to the "Restrictions" tab => TEST: Observe that the newly added restriction is displayed - Navigate to the patron's "You summary" page in the OPAC => Observe that the newly added restriction's "comment" is displayed Created attachment 93778 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Created attachment 93779 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Created attachment 93780 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Created attachment 93781 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Created attachment 93782 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Created attachment 93783 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Created attachment 93784 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Created attachment 93785 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Created attachment 93786 [details] [review] Bug 23681: Add unit tests A couple things: * Can Koha::RestrictionType(s) be moved to Koha::Patron::RestrictionType(s) * Can the table debarment_types be renamed to patron_restriction_types so our internal and external terminology match? Thanks! Hi Kyle Thanks for taking a look at this. >Can Koha::RestrictionType(s) be moved to Koha::Patron::RestrictionType(s) That's actually how I had it originally. However I moved it since, as far as I could see, everything in Koha::Patron::* was related to a singular patron and represented properties of that patron, which was not the case of RestrictionTypes, which represents the canonical list of restriction types, or a restriction type, not at all related to a patron. I'm happy to move it back if you feel it would be better there, but wanted to explain the rationale. >Can the table debarment_types be renamed to patron_restriction_types so our internal and external terminology match? Again, there is a rationale behind this. I named the table "debarment_types" because it was related to a pre-existing table "borrower_debarments" and I felt it would be confusing to name it "restriction_types". I realise that there is a move towards "restriction" rather than "debarment", and I have used the new term in the parts of this bug that are truly new, but where I'm interacting with stuff that's referring to debarments, it felt less confusing to use the same terminology for my bits. Hopefully, at some point, a full sweep of "debarment" => "restriction" will be done, so I felt this was a reasonable interim balance. Again, if you'd rather it be changed, I can do so. Many thanks! Now that my brain has caught up with Andrews explanation I'm happy here. Yes, the Object <-> DB naming is inconsistent.. BUT by doing so the DB <-> DB and the Object <-> Object levels are both consistent at their level. That makes reasonable sense to me, though in reality I'd love to have seen 'debartment' changed to 'restriction' at the DB level consistently at the time we decided to switch terms.. we're now dealing with legacy baggage. As for RestrictionTypes I'm less sure.. but had a chat with Katrin and she agreed it did make sense as a top level object.. Patron::Restrictions makes sense as they're directly related but Patrons::RestrictionTypes doesn't as it's the restriction itself that has a type and is a level distinct.. Koha::Patron::Restriction::RestrictionTypes seems a little too nested.. so happy to stick to top level object here. Created attachment 94862 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Created attachment 94863 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Created attachment 94864 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Created attachment 94865 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Created attachment 94866 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Created attachment 94867 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Created attachment 94868 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Created attachment 94869 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Created attachment 94870 [details] [review] Bug 23681: Add unit tests Created attachment 94896 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Created attachment 94897 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Created attachment 94898 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Created attachment 94899 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Created attachment 94900 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Created attachment 94901 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Created attachment 94902 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Created attachment 94903 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Created attachment 94904 [details] [review] Bug 23681: Add unit tests Created attachment 95148 [details] [review] Bug 23681: (follow-up) Fix typo *** Bug 6775 has been marked as a duplicate of this bug. *** Created attachment 95255 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 95256 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 95257 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 95258 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 95259 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 95260 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 95261 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 95262 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 95263 [details] [review] Bug 23681: Add unit tests Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 95264 [details] [review] Bug 23681: (follow-up) Fix typo Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University (In reply to Andrew Isherwood from comment #1) > It has become increasingly apparent that using authorised values for this is > not the most appropriate way forward. The number of modifications and > additions to AV was increasing the scope of the work too much, it was clear > that AV was not the correct place for this. Why? Can you explain? That was my first reaction reading the patches. Using AV will make the feature translatable in the future (bug 20307). So you are adding a regression, the restrictions are no longer translatable. Is the "default" flag really useful? We could imagine a new "internal" flag (to replace ronly) for AVs. (In reply to Jonathan Druart from comment #54) > (In reply to Andrew Isherwood from comment #1) > > It has become increasingly apparent that using authorised values for this is > > not the most appropriate way forward. The number of modifications and > > additions to AV was increasing the scope of the work too much, it was clear > > that AV was not the correct place for this. > > Why? Can you explain? > That was my first reaction reading the patches. > Using AV will make the feature translatable in the future (bug 20307). Clearly AV would have been the preferable route, which is why I spent some considerable time trying to implement using AV before reluctantly abandoning it, it certainly wasn't abandoned lightly. There were too many requirements of this work that were not provided by AV and working around them was becoming increasingly cludgy and unmaintainable. This work was done a few months ago, so my memory of it isn't what it was, but examples would be: - the ability to specify a default restriction type, which any restrictions would fall back to if the restriction type they were using was deleted - the ability to prevent certain restriction types from being edited - the ability to prevent certain restriction types from being deleted > So you are adding a regression, the restrictions are no longer translatable. They were previously held in borrower_debarments.type, they are now in debarment_types, so I'm guessing adding translation support for the default values is not an insurmountable problem, I am happy to add whatever work is required to make that happen to this bug. Any additional values are added by the user anyway, so are by their nature translatable. >Is the "default" flag really useful? We could imagine a new "internal" flag (to replace ronly) for AVs. I believe so. Restrictions need a type, even if one is not explicitly chosen. Also, we need an ability to say "if a restriction type is deleted, any restrictions that use this type should fall back to type X", the default flag indicates which type is X. (In reply to Andrew Isherwood from comment #57) > (In reply to Jonathan Druart from comment #54) > >Is the "default" flag really useful? We could imagine a new "internal" flag (to replace ronly) for AVs. > > I believe so. Restrictions need a type, even if one is not explicitly > chosen. Also, we need an ability to say "if a restriction type is deleted, > any restrictions that use this type should fall back to type X", the default > flag indicates which type is X. I do not remember I saw a way to modify it, it's hardcoded in DB (and so could be in the code). The default (In reply to Jonathan Druart from comment #58) > > t > > I believe so. Restrictions need a type, even if one is not explicitly > > chosen. Also, we need an ability to say "if a restriction type is deleted, > > any restrictions that use this type should fall back to type X", the default > > flag indicates which type is X. > > I do not remember I saw a way to modify it, it's hardcoded in DB (and so > could be in the code). You're absolutely right, the default must always be "MANUAL" due to the way the restrictions were originally implemented, however the name can be changed so as far as the user is concerned, it can be anything they wish. Created attachment 99893 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 99894 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 99895 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 99896 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 99897 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 99898 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 99899 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 99900 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 99901 [details] [review] Bug 23681: Add unit tests Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 99902 [details] [review] Bug 23681: (follow-up) Fix typo Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University (In reply to Andrew Isherwood from comment #57) > (In reply to Jonathan Druart from comment #54) > > So you are adding a regression, the restrictions are no longer translatable. > > They were previously held in borrower_debarments.type, they are now in > debarment_types, so I'm guessing adding translation support for the default > values is not an insurmountable problem, I am happy to add whatever work is > required to make that happen to this bug. Any additional values are added by > the user anyway, so are by their nature translatable. This is a blocker. Please provide a fix. To resume, We have 4 defaults 'SUSPENSION','OVERDUES','MANUAL','DISCHARGE'. Prior to this patch, they were translatable from the template (see SWITCH in borrower_debarments.inc). Now you can edit their description in the new admin view, but they cannot be correctly translated (depending on the language of the interface). I made one suggestion to Andrew: remove the ability to edit the lib for those 4 defaults. So we could still use the SWITCH and default to the description for the others. But it seems that it won't fit the need of the sponsor. I do not have another solution however. We need Ben to varify that they really want to change the description of any of the following types: 'Manual', 'Overdues', 'Suspension', 'Discharge' And, to be honest, those should be easily altered via JS if required.. I don't think this should block the rest of the work. So.. to me it feels like we just need to take the same approach as we now have with the accounts system, where the original system types are all coded in the template and thus translations are not lost and then we fallback to descriptions from the database for any new types. Comment on attachment 99898 [details] [review] Bug 23681: Allow for changes to debarments Review of attachment 99898 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/includes/borrower_debarments.inc @@ -26,5 @@ > - Overdues > - [% CASE 'SUSPENSION' %] > - Suspension > - [% CASE 'DISCHARGE' %] > - Discharge Simply adding [% CASE %] [% restriction_types.$dtype.display_text | html %] To the end of this SWITCH block instead of replacing the block entirely should get this passed QA in my opinion. The customer requires being able to change the hard coded values.. as such this bug is blocked until we have translatable authorized values Could we do something like an overwrite option? We use the default descriptions when the lib/lib_opac are empty and we use the description of those, if they are set? The other things is me wondering if using authorized value would not only be a temporary solution, making it necessary soon to move debarment types to their own table to be more flexible with configuration. In this case we could have a column for the description too - it would be even a little easier. And we could then develop the table further to have more columns that make the behavior of a debarment more granular (block renewals, block issuing, block holds, which category types can it be applied to, etc.). (In reply to Katrin Fischer from comment #75) > Could we do something like an overwrite option? > > We use the default descriptions when the lib/lib_opac are empty and we use > the description of those, if they are set? > Nope, that would still break translations as Jonathan suggests. The issue is that you cant define that default for each language.. only a single language. > The other things is me wondering if using authorized value would not only be > a temporary solution, making it necessary soon to move debarment types to > their own table to be more flexible with configuration. In this case we > could have a column for the description too - it would be even a little > easier. This development doesn't use AV's specifically for the reasons you suggest. > > And we could then develop the table further to have more columns that make > the behavior of a debarment more granular (block renewals, block issuing, > block holds, which category types can it be applied to, etc.). I've been waiting years for the AV translations proof work to work their way through.. their holding up a whole plethora of bugs including this one. I thought we could do it similar to account types, just with the addition of allowing to overwrite the standard description. If the overwrite is empty, we use what is in the templates. So we could keep the standard ones translatable. For the table vs AV - I was didn't read far enough - sorry. But... if we are no longer translating an AV... shoudl this not be a little easier in terms of 'hugeness' of development? Maybe it would be easier to start here doing something similar than we have for itemtypes? I see this bug is a bit stuck, but commenting to say I like the idea! Created attachment 130373 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 130374 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 130375 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 130376 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 130377 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 130378 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 130379 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 130380 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 130381 [details] [review] Bug 23681: Add unit tests Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 130382 [details] [review] Bug 23681: (follow-up) Fix typo Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 130383 [details] [review] Bug 23681: (follow-up) Fix rebase issues Created attachment 130384 [details] [review] Bug 23681: (QA follow-up) Allow for translations This patch allows for existing translations to follow through if debarment type descriptions are left unaltered for the default types. Rebased and added a follow-up to do as Katrin suggested.. we now use hardcoded translations in the template if the defaults are left unaltered in the existing debarment types. Starting here: 1) QA test tools Processing files before patches |========================>| 18 / 18 (100.00%) Processing files after patches No schema file found for table debarment_types at /kohadevbox/qa-test-tools/QohA/File/Specific/Kohastructure.pm line 118. No schema file found for table debarment_types at /kohadevbox/qa-test-tools/QohA/File/Specific/Kohastructure.pm line 118. |========================>| 18 / 18 (100.00%) FAIL Koha/RestrictionType.pm FAIL forbidden patterns forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 16) FAIL Koha/RestrictionTypes.pm FAIL forbidden patterns forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 16) FAIL admin/restrictions.pl FAIL critic Subroutine "new" called using indirect syntax at line 27, column 13. See page 349 of PBP. FAIL forbidden patterns forbidden pattern: Script permissions is authnotrequired => 0, it could be correct for an OPAC script if it is was you really want error (bug 24663) (line 38) OK circ/circulation.pl OK installer/data/mysql/kohastructure.sql OK installer/data/mysql/mandatory/sysprefs.sql FAIL koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss FAIL css_and_scss_in_sync Found CSS file not in sync with its SCSS file, use yarn build. This is not blocker, testers can generate the .css file if they need it, see https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client OK koha-tmpl/intranet-tmpl/prog/en/includes/borrower_debarments.inc OK koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/restrictions.tt FAIL forbidden patterns forbidden pattern: Title elements must start with the unique information (bug 26703) (line 6) OK koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt OK koha-tmpl/intranet-tmpl/prog/js/restrictiontypes.js OK members/memberentry.pl OK members/mod_debarment.pl OK members/moremember.pl FAIL t/db_dependent/RestrictionType.t FAIL file permissions File must have the exec flag FAIL t/db_dependent/RestrictionTypes.t FAIL file permissions File must have the exec flag 2) Sample data for installer The sample data added here is .sql not a .yml, but they might contain strings to translate. Can you please verify? But we still need the .sql files to add to the remaining 'old style' installers. 3) Database structure ronly and dflt could be improved as column names. ronly = read only, but dlft? Otherwise as far as I can tell it all seems to match up between new and updated installations. 4) Preference So the preference is only about the pull down when adding a patron restriction, correct? If you want to make me really happy, please and with a . :) But I believe options need to be changed to 1 and 0? + - pref: PatronRestrictionTypes + choices: + yes: Allow + no: Don't allow + - "the type of patron restriction to be specified when applying manually" 5) Page title It needs to be updated to reflect the accessibility work starting with the most specific bits, but I think the QA tool already caught that: +<title>Koha › Administration › Patron restrictions › [% IF op == 'add_form' %][% IF ( restriction ) %]Modify restriction '[% restriction.display_text | html %]'[% ELSE %]New restriction[% END %][% END %] 6) New permission - OK I wondered if the new permission should be assigned to someone... but I guess it works ok. If the user has module permission already, they will have permission for the new page. Otherwise it will have to be assigned, but existing functionality keeps working meanwhile. So all good. Created attachment 132302 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 132303 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 132304 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 132305 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 132306 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 132307 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 132308 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 132309 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 132310 [details] [review] Bug 23681: Add unit tests Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 132311 [details] [review] Bug 23681: (follow-up) Fix typo Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 132312 [details] [review] Bug 23681: (follow-up) Fix rebase issues Created attachment 132313 [details] [review] Bug 23681: (QA follow-up) Allow for translations This patch allows for existing translations to follow through if debarment type descriptions are left unaltered for the default types. Created attachment 132314 [details] [review] Bug 23681: Updates for bug 26703 This patch fixes the title ordering and also replaces the breadcrumbs with the modern nav block equivilent Created attachment 132315 [details] [review] Bug 23681: (QA follow-up) Fix license, perlcritic & executable This patch fixes the simple issues mentioned by the QA script. Created attachment 132316 [details] [review] Bug 23681: CSS Build QA Fixes Done More to do... I missed Katrins extra comments beyond the QA script failure.. oops Created attachment 132318 [details] [review] Bug 23681: (QA follow-up) Rename columns Renames ronly to readonly and dflt to is_system. Created attachment 132319 [details] [review] Bug 23681: (QA follow-up) Rename columns Renames ronly to readonly and dflt to is_system. Created attachment 132320 [details] [review] Bug 23681: (QA follow-up) Fix prefs Add the missing `.` and switch from `yes/no` to `1/0`. (In reply to Katrin Fischer from comment #92) > 2) Sample data for installer > I think this is dealt with by having it in the templates instead > > 3) Database structure > > ronly and dflt could be improved as column names. ronly = read only, but > dlft? dflt = default, but this is a reserved word so I updated it to is_system (which other places already use) > 4) Preference Done > 5) Page title Done > 6) New permission - OK I think this is OK. I am sorry, this got rather long again. It might not look like it, but I think we are really close and it WOULD make a great starting point for making the restriction system more granular. 1) Installer data again (In reply to Martin Renvoize from comment #113) > (In reply to Katrin Fischer from comment #92) > > 2) Sample data for installer > > > I think this is dealt with by having it in the templates instead I had a closer look here and the installer file needs updating one way around or the other: I we don't want to translate it, it should be in https://git.koha-community.org/Koha-community/Koha/src/branch/master/installer/data/mysql/mandatory. If we don't have it wrong, we should transform to .yml and also add it to fr-FR, the remaining SQL installer. The .yml file structure looks easy enough in how to mark the translatable bits and how to add a description for the installer: https://git.koha-community.org/Koha-community/Koha/src/branch/master/installer/data/mysql/en/mandatory I either case the .txt needs to be removed. I think the latter would be the nicer solution, as the display text translated would still be nice for reports and such. (I'd also be in favor of moving account_credit_types and debit_types for that reason, but that's another story :) ) +INSERT INTO debarment_types (code, display_text, readonly, system) VALUES + ('MANUAL', 'Manual', 1, 1), + ('OVERDUES', 'Overdues', 1, 0), + ('SUSPENSION', 'Suspension', 1, 0), + ('DISCHARGE', 'Discharge', 1, 0); 2) CSS update I believe Bug 23681: CSS Build should be dropped. It's expected that you generate the CSS files locally. It might create conflicts, but I can just drop/skip it then. 3) Typo in database update / column names a) + system tinyint(1) NOT NULL DEFAULT 0 Should be: is_system. I've fixed it locally for testing further: alter table debarment_types rename column system to is_system; b) I have been thinking more about the different columns and I think there is an error there: +------------+--------------+----------+-----------+ | code | display_text | readonly | is_system | +------------+--------------+----------+-----------+ | DISCHARGE | Discharge | 1 | 0 | | MANUAL | Manual | 1 | 1 | | OVERDUES | Overdues | 1 | 0 | | SUSPENSION | Suspension | 1 | 0 | +------------+--------------+----------+-----------+ read_only should be is_system = It's internal, don't change it. Applies to all currently added ones. is_system, was dflt = default = means it's used when the pref is turned off. So it should be default actually. AND: I think we need another column: can_be_added_manually (see account_credit_types for comparison) Because right now the list of restrictions on the patron account will show ALL restrictions defined in debarment_types and I feel like we are asking for trouble if we allow to set restrictions manually that are supposed to be added automatically. Like a DISCHARGE restriction should be linked to an entry in the discharges table somehow, an OVERDUES restriction can be lifted automatically if all overdues are returned, SUSPENSION is calculated on late returns... I'd feel we might want to leave them out of the pulldown really to keep internal behaviors untouched. 4) System preference (sorry, only saw this in the GUI) Currently the pref appears under "Patrons > Patron relationships", but this is the section for guarantor and family account stuff. I suggest moving it into the "Accounting > Features" tab, that maybe didn't exist yet when this was first written. 5) Page title There is a > left at the beginning that should not be there if you look at the tab in the browser. 6) Deleting a restriction When I try to delete a newly created restriction, I see: Can't locate object method "_new_from_dbic" via package "Koha::DebarmentType" (perhaps you forgot to load "Koha::DebarmentType"?) at /kohadevbox/koha/Koha/Object.pm line 237 7) Tests are failing :( prove t/db_dependent/RestrictionType.t t/db_dependent/RestrictionTypes.t t/db_dependent/RestrictionType.t ... 1/3 # Failed test 'keyed_on_code returns correctly' # at t/db_dependent/RestrictionType.t line 58. # Structures begin differing at: # $got->{MANUAL} = HASH(0x5618db704c68) # $expected->{MANUAL} = Does not exist # Looks like you failed 1 test of 3. t/db_dependent/RestrictionType.t ... Dubious, test returned 1 (wstat 256, 0x100) Failed 1/3 subtests t/db_dependent/RestrictionTypes.t .. 1/6 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row. SQL that returns multiple rows is DEPRECATED for ->find and ->single at /kohadevbox/koha/Koha/Objects.pm line 96 Can't locate object method "_new_from_dbic" via package "Koha::DebarmentType" (perhaps you forgot to load "Koha::DebarmentType"?) at /kohadevbox/koha/Koha/Object.pm line 237. # Looks like your test exited with 11 just after 3. t/db_dependent/RestrictionTypes.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 3/6 subtests Test Summary Report ------------------- t/db_dependent/RestrictionType.t (Wstat: 256 Tests: 3 Failed: 1) Failed test: 3 Non-zero exit status: 1 t/db_dependent/RestrictionTypes.t (Wstat: 2816 Tests: 3 Failed: 0) Non-zer Created attachment 134512 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134513 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134514 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134515 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134516 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134517 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134518 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134519 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134520 [details] [review] Bug 23681: Add unit tests Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134521 [details] [review] Bug 23681: (follow-up) Fix typo Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134522 [details] [review] Bug 23681: (follow-up) Fix rebase issues Created attachment 134523 [details] [review] Bug 23681: (QA follow-up) Allow for translations This patch allows for existing translations to follow through if debarment type descriptions are left unaltered for the default types. Created attachment 134524 [details] [review] Bug 23681: Updates for bug 26703 This patch fixes the title ordering and also replaces the breadcrumbs with the modern nav block equivilent Created attachment 134525 [details] [review] Bug 23681: (QA follow-up) Fix license, perlcritic & executable This patch fixes the simple issues mentioned by the QA script. Created attachment 134526 [details] [review] Bug 23681: (QA follow-up) Rename columns Renames ronly to readonly and dflt to is_system. Created attachment 134527 [details] [review] Bug 23681: (QA follow-up) Fix prefs Add the missing `.` and switch from `yes/no` to `1/0`. Created attachment 134528 [details] [review] Bug 23681: Fix QA issues This patch fixes following QA issues: - convert intaller files as .yml - change column name readonly as is_system - change column name is_system as default_value - add column can_be_added_manually (testplan for this below) - move syspref "PatronRestrictionTypes" to "Accounting > Features" tab - tweak page title - tweak tests to apply these changes Also atomicupdate files have been updated. Issues with delete and tests is fixed with adding additional schema change file. To test: 1) Add new restriction type and make it manual. 2) Navigate to patron details page. 3) Add new restriction to patron. => Only selectable restriction should be the one you just created. Also prove t/db_dependent/RestrictionTypes.t. Sponsored-by: Koha-Suomi Oy Created attachment 134529 [details] [review] Bug 23681: DO NOT PUSH! Schema changes To be able to delete and pass tests we need to declare koha_object[s]_class to DebarmentType.pm. Otherwise following error is raised: "Can't locate object method "_new_from_dbic" via package "Koha::DebarmentType" (perhaps you forgot to load "Koha::DebarmentType"?) at /kohadevbox/koha/Koha/Object.pm line 237" Rebased patches and added couple new ones where I hopefully catched up all QA issues from comment 114. Also I dropped and obsoleted CSS patch since it broke the UI in several pages and as Katrin stated: >It's expected that you generate the CSS files locally. Thanks for jumping on this rebase and clean up Emmi, teamwork for the win! Testing follow-ups now.. We're back to failing here.. whilst I understand the want to translate the system options, this follow-up breaks the ability to customise those same options for display... which was a core requirement for the original Sponsor. I'm torn.. I feel you should be able to customise those if you really want to. I also think that the 'Manual' option should be listed as one you can add manually ;) I also disagree with the syspref move.. this isn't an accounting feature.. it has nothing to do with money. I think the pref should live under circulation or patron tabs somewhere.. we already have some Debartment/Restriction preferences it could perhaps go with. AutoRemoveOverduesRestrictions, RestrictionBlockRenewing, CumulativeRestrictionPeriods all appear under circulation.. so I reckon that's where if fits. I can happily move the pref.. but the translation vs customisation is a wider issue.. it needs some cleaning up either way, but I need input. See coment 75, 76, 77 and 91 for details about the way translations were handled here.. it was a very deliberate choice to leave them as translations in the Templates and not move them to the Database.. but moving them to the database here we loose the ability to define custom display texts locally. Working on follow-ups (In reply to Martin Renvoize from comment #137) > I also disagree with the syspref move.. this isn't an accounting feature.. > it has nothing to do with money. Oof, I misunderstood "Accounting" part here. Accounting => account => everything that has something to do with handling patrons account :') But I agree, the syspref should probably be put either under Patron->General or maybe Circulation->Checkout, since most of the other similar sysprefs are there (personally I think we could have section for restrictions or even general section for patron account handling but that's a whole different thing). As for not being able to modify default restrictions, that's a mistake in code. This line > $restriction->display_text($display_text); should be above unless statement. After moving it system restrictions can at least be modified from UI. And I forgot to add to my first comment, I didn't include fr-FR translated files in these patches as Katrin suggested, since I'm not a native speaker/don't even speak french :D So they are still missing from here. Hi Emmi, I do really appreciate your work here, hope I didn't come across mean in my comments.. it turned out a little more verbose than intended as I worked out how it all fits together again. Do you happen to still have a branch around for this? I've got a couple of follow-up patches to add but I've discovered that when trying to apply the patches from here I get some weird unexpected conflicts and the result is that we somehow drop a patch when applying at the moment. If you still have the branch, I could use that as a base to try and work on the fixes. (In reply to Martin Renvoize from comment #144) > Hi Emmi, > > I do really appreciate your work here, hope I didn't come across mean in my > comments.. it turned out a little more verbose than intended as I worked out > how it all fits together again. > You didn't sound mean at all, so don't worry :o And I understand perfectly that these thinks tend to turn out more complex than they first look :D > Do you happen to still have a branch around for this? I've got a couple of > follow-up patches to add but I've discovered that when trying to apply the > patches from here I get some weird unexpected conflicts and the result is > that we somehow drop a patch when applying at the moment. > > If you still have the branch, I could use that as a base to try and work on > the fixes. I do, here: https://github.com/emta001/Koha/tree/bug-23681 Can you get it from there? Awesome, that worked perfectly.. no idea why the patches on BZ apply weirdly. Right.. I'll add my follow-ups shortly.. may squash a few patches too, but keep enough that we get all the attributions after so many people have worked on this. Created attachment 134580 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134581 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134582 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134583 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134584 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134585 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134586 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134587 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134588 [details] [review] Bug 23681: Add unit tests Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134589 [details] [review] Bug 23681: (follow-up) Fix typo Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134590 [details] [review] Bug 23681: (follow-up) Fix rebase issues Created attachment 134591 [details] [review] Bug 23681: (QA follow-up) Allow for translations This patch allows for existing translations to follow through if debarment type descriptions are left unaltered for the default types. Created attachment 134592 [details] [review] Bug 23681: Updates for bug 26703 This patch fixes the title ordering and also replaces the breadcrumbs with the modern nav block equivilent Created attachment 134593 [details] [review] Bug 23681: (QA follow-up) Fix license, perlcritic & executable This patch fixes the simple issues mentioned by the QA script. Created attachment 134594 [details] [review] Bug 23681: (QA follow-up) Rename columns Renames ronly to readonly and dflt to is_system. Created attachment 134595 [details] [review] Bug 23681: (QA follow-up) Fix prefs Add the missing `.` and switch from `yes/no` to `1/0`. Created attachment 134596 [details] [review] Bug 23681: Fix QA issues This patch fixes following QA issues: - convert intaller files as .yml - change column name readonly as is_system - change column name is_system as default_value - add column can_be_added_manually (testplan for this below) - move syspref "PatronRestrictionTypes" to "Accounting > Features" tab - tweak page title - tweak tests to apply these changes Also atomicupdate files have been updated. Issues with delete and tests is fixed with adding additional schema change file. To test: 1) Add new restriction type and make it manual. 2) Navigate to patron details page. 3) Add new restriction to patron. => Only selectable restriction should be the one you just created. Also prove t/db_dependent/RestrictionTypes.t. Sponsored-by: Koha-Suomi Oy Created attachment 134597 [details] [review] Bug 23681: DO NOT PUSH! Schema changes To be able to delete and pass tests we need to declare koha_object[s]_class to DebarmentType.pm. Otherwise following error is raised: "Can't locate object method "_new_from_dbic" via package "Koha::DebarmentType" (perhaps you forgot to load "Koha::DebarmentType"?) at /kohadevbox/koha/Koha/Object.pm line 237" Created attachment 134598 [details] [review] Bug 23681: (QA follow-up) Move preference again This patch removes the new PatronRestrictionTypes preference from the accounting area in preference to creating a new group for 'Patron restriction' related preferences in the circulation preferences tab. Created attachment 134599 [details] [review] Bug 23681: (QA follow-up) Restore customisablity of description This patch restores the customisability of description for system restriction types. Translatability is not affected as you can translate the defaults via template translation and assuming the end users leave their descriptions at the default, the translations will be picked. Created attachment 134600 [details] [review] Bug 23681: (QA follow-up) Merge update files This patch merges the three atomic update files into one and also adds a check for foreing key existance to make the update idempotent. Created attachment 134601 [details] [review] Bug 23681: (QA follow-up) Proper handling of default option This patch removes the 'can_be_added_manually' flag. Only non-system restriction types can be added manually, so we exclude is_system instead of having two flags. (And we set the 'Manual' that's added at install time to default but not system). We then add proper handling for setting the default manual restriction type in the management page and set the dropdown list to use that value by default. Created attachment 134602 [details] [review] Bug 23681: DBIC Rebuild That was "fun".. The patchset now all applies cleanly again and I believe the default handling now works as requested along with only displaying non system types as manually addable. The translation options are also fixed. Back to the QA queue There is one fail in the QA test tools here: FAIL installer/data/mysql/kohastructure.sql FAIL tinyint_has_boolean_flag The new column debarment_types.is_system is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/DebarmentType.pm), see the SQL12 coding guideline The new column debarment_types.is_default is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/DebarmentType.pm), see the SQL12 coding guideline Also tests are failing: prove t/db_dependent/RestrictionType.t t/db_dependent/RestrictionTypes.t t/db_dependent/RestrictionType.t ... 1/3 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'me.default_value' in 'where clause' at /kohadevbox/koha/Koha/Objects.pm line 96 # Looks like your test exited with 11 just after 2. t/db_dependent/RestrictionType.t ... Dubious, test returned 11 (wstat 2816, 0xb00) Failed 1/3 subtests t/db_dependent/RestrictionTypes.t .. 1/6 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'me.default_value' in 'where clause' at /kohadevbox/koha/Koha/Objects.pm line 96 # Looks like your test exited with 11 just after 3. t/db_dependent/RestrictionTypes.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 3/6 subtests Test Summary Report ------------------- t/db_dependent/RestrictionType.t (Wstat: 2816 Tests: 2 Failed: 0) Non-zero exit status: 11 Parse errors: Bad plan. You planned 3 tests but ran 2. t/db_dependent/RestrictionTypes.t (Wstat: 2816 Tests: 3 Failed: 0) Non-zero exit status: 11 Parse errors: Bad plan. You planned 6 tests but ran 3. Files=2, Tests=5, 2 wallclock secs ( 0.01 usr 0.01 sys + 1.88 cusr 0.15 csys = 2.05 CPU) Result: FAIL I made sure to apply the database update before running the tests. Can you please check? Created attachment 134684 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134685 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134686 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134687 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134688 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134689 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134690 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134691 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134692 [details] [review] Bug 23681: Add unit tests Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134693 [details] [review] Bug 23681: (follow-up) Fix typo Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 134694 [details] [review] Bug 23681: (follow-up) Fix rebase issues Created attachment 134696 [details] [review] Bug 23681: (QA follow-up) Allow for translations This patch allows for existing translations to follow through if debarment type descriptions are left unaltered for the default types. Created attachment 134697 [details] [review] Bug 23681: Updates for bug 26703 This patch fixes the title ordering and also replaces the breadcrumbs with the modern nav block equivilent Created attachment 134698 [details] [review] Bug 23681: (QA follow-up) Fix license, perlcritic & executable This patch fixes the simple issues mentioned by the QA script. Created attachment 134699 [details] [review] Bug 23681: (QA follow-up) Rename columns Renames ronly to readonly and dflt to is_system. Created attachment 134700 [details] [review] Bug 23681: (QA follow-up) Fix prefs Add the missing `.` and switch from `yes/no` to `1/0`. Created attachment 134701 [details] [review] Bug 23681: Fix QA issues This patch fixes following QA issues: - convert intaller files as .yml - change column name readonly as is_system - change column name is_system as default_value - add column can_be_added_manually (testplan for this below) - move syspref "PatronRestrictionTypes" to "Accounting > Features" tab - tweak page title - tweak tests to apply these changes Also atomicupdate files have been updated. Issues with delete and tests is fixed with adding additional schema change file. To test: 1) Add new restriction type and make it manual. 2) Navigate to patron details page. 3) Add new restriction to patron. => Only selectable restriction should be the one you just created. Also prove t/db_dependent/RestrictionTypes.t. Sponsored-by: Koha-Suomi Oy Created attachment 134702 [details] [review] Bug 23681: DO NOT PUSH! Schema changes To be able to delete and pass tests we need to declare koha_object[s]_class to DebarmentType.pm. Otherwise following error is raised: "Can't locate object method "_new_from_dbic" via package "Koha::DebarmentType" (perhaps you forgot to load "Koha::DebarmentType"?) at /kohadevbox/koha/Koha/Object.pm line 237" Created attachment 134703 [details] [review] Bug 23681: (QA follow-up) Move preference again This patch removes the new PatronRestrictionTypes preference from the accounting area in preference to creating a new group for 'Patron restriction' related preferences in the circulation preferences tab. Created attachment 134704 [details] [review] Bug 23681: (QA follow-up) Restore customisablity of description This patch restores the customisability of description for system restriction types. Translatability is not affected as you can translate the defaults via template translation and assuming the end users leave their descriptions at the default, the translations will be picked. Created attachment 134705 [details] [review] Bug 23681: (QA follow-up) Merge update files This patch merges the three atomic update files into one and also adds a check for foreing key existance to make the update idempotent. Created attachment 134706 [details] [review] Bug 23681: (QA follow-up) Proper handling of default option This patch removes the 'can_be_added_manually' flag. Only non-system restriction types can be added manually, so we exclude is_system instead of having two flags. (And we set the 'Manual' that's added at install time to default but not system). We then add proper handling for setting the default manual restriction type in the management page and set the dropdown list to use that value by default. Created attachment 134707 [details] [review] Bug 23681: DBIC Rebuild Created attachment 134708 [details] [review] Bug 23681: (follow-up) Mark fields at booleans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> One more little follow-up for the booleans issue and a rebase to fix the other problems. Hopefully it should all be passing now Katrin :) Could you please rebase? Created attachment 136716 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 136717 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 136718 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 136719 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 136720 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 136721 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 136722 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 136723 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 136724 [details] [review] Bug 23681: Add unit tests Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 136725 [details] [review] Bug 23681: (follow-up) Fix typo Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 136726 [details] [review] Bug 23681: (follow-up) Fix rebase issues Created attachment 136727 [details] [review] Bug 23681: (QA follow-up) Allow for translations This patch allows for existing translations to follow through if debarment type descriptions are left unaltered for the default types. Created attachment 136728 [details] [review] Bug 23681: Updates for bug 26703 This patch fixes the title ordering and also replaces the breadcrumbs with the modern nav block equivilent Created attachment 136729 [details] [review] Bug 23681: (QA follow-up) Fix license, perlcritic & executable This patch fixes the simple issues mentioned by the QA script. Created attachment 136730 [details] [review] Bug 23681: (QA follow-up) Rename columns Renames ronly to readonly and dflt to is_system. Created attachment 136731 [details] [review] Bug 23681: (QA follow-up) Fix prefs Add the missing `.` and switch from `yes/no` to `1/0`. Created attachment 136732 [details] [review] Bug 23681: Fix QA issues This patch fixes following QA issues: - convert intaller files as .yml - change column name readonly as is_system - change column name is_system as default_value - add column can_be_added_manually (testplan for this below) - move syspref "PatronRestrictionTypes" to "Accounting > Features" tab - tweak page title - tweak tests to apply these changes Also atomicupdate files have been updated. Issues with delete and tests is fixed with adding additional schema change file. To test: 1) Add new restriction type and make it manual. 2) Navigate to patron details page. 3) Add new restriction to patron. => Only selectable restriction should be the one you just created. Also prove t/db_dependent/RestrictionTypes.t. Sponsored-by: Koha-Suomi Oy Created attachment 136733 [details] [review] Bug 23681: DO NOT PUSH! Schema changes To be able to delete and pass tests we need to declare koha_object[s]_class to DebarmentType.pm. Otherwise following error is raised: "Can't locate object method "_new_from_dbic" via package "Koha::DebarmentType" (perhaps you forgot to load "Koha::DebarmentType"?) at /kohadevbox/koha/Koha/Object.pm line 237" Created attachment 136734 [details] [review] Bug 23681: (QA follow-up) Move preference again This patch removes the new PatronRestrictionTypes preference from the accounting area in preference to creating a new group for 'Patron restriction' related preferences in the circulation preferences tab. Created attachment 136735 [details] [review] Bug 23681: (QA follow-up) Restore customisablity of description This patch restores the customisability of description for system restriction types. Translatability is not affected as you can translate the defaults via template translation and assuming the end users leave their descriptions at the default, the translations will be picked. Created attachment 136736 [details] [review] Bug 23681: (QA follow-up) Merge update files This patch merges the three atomic update files into one and also adds a check for foreing key existance to make the update idempotent. Created attachment 136737 [details] [review] Bug 23681: (QA follow-up) Proper handling of default option This patch removes the 'can_be_added_manually' flag. Only non-system restriction types can be added manually, so we exclude is_system instead of having two flags. (And we set the 'Manual' that's added at install time to default but not system). We then add proper handling for setting the default manual restriction type in the management page and set the dropdown list to use that value by default. Created attachment 136738 [details] [review] Bug 23681: DBIC Rebuild Created attachment 136739 [details] [review] Bug 23681: (follow-up) Mark fields at booleans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Rebased Starting QA here: 0) Tests pass. QA script complains a lot about "added and removed in the same patch set", we could consider some squashing. Database update is missing execute permission. 1) The new patron restrictions configuration page is missing from the Administration side menu, visible when you are on any of the admin pages. It's ok on the start page of the module. 2) The delete confirmation question is missing a question mark :) (also might want to remove the additional spaces) Are you sure you want to delete " Test " I'd also make the button just "Delete" or "Yes, delete", but that's a personal preference and I see that this was 'copied' from how deleting a patron category looks like. 3) After installing/updating with this patch, MANUAL is marked es the default. You cannot change it, until you have added a new restriction, which makes sense, as the system internal ones should not be default. But: you cannot ever change the default back to MANUAL. I think this is because system internal values are excluded, but it might raise complaints for people experimenting with the feature that want to restore previous behavior. 4) Just a comment: Love the new Patron restrictions sub heading in patron preferences :) 5) When adding a manual restriction to the patron account, the default seems to be only used when PatronRestrictionTypes is off, when it's on and the pull down shows, the default is not preselected, but the first entry is. 6) Something super odd: In the patron account in the staff interface, on the checkout tab, the Restrictions shows as (0), while on the details tab the restrictions show the correct number (2) I have added. Only the tab heading is wrong, otherwise the restrictions show ok. 7) When editing the patron record, the pull down for restriction type is empty. 8) I notice that the type itself doesn't show up in the OPAC or the note on the patron account in staff, just the comments do. This is the case with and without the patches, maybe something so consider later. Or we could imagine adding a default comment? (no blocker, just loud thinking) (In reply to Katrin Fischer from comment #224) > Starting QA here: > > 0) Tests pass. QA script complains a lot about "added and removed in the > same patch set", we could consider some squashing. Database update is > missing execute permission. I think this is a QA script mis-identification.. the file it mentions is also mentioned in the git manip.. i.e. it no longer exists and the remaining renamed file does have the exec bit set. (In reply to Martin Renvoize from comment #225) > (In reply to Katrin Fischer from comment #224) > > Starting QA here: > > > > 0) Tests pass. QA script complains a lot about "added and removed in the > > same patch set", we could consider some squashing. Database update is > > missing execute permission. > > I think this is a QA script mis-identification.. the file it mentions is > also mentioned in the git manip.. i.e. it no longer exists and the remaining > renamed file does have the exec bit set. I think you are right about this one. Created attachment 137066 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 137067 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 137068 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 137069 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 137070 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 137071 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 137072 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 137073 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 137074 [details] [review] Bug 23681: Add unit tests Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 137075 [details] [review] Bug 23681: (follow-up) Fix typo Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Created attachment 137076 [details] [review] Bug 23681: (follow-up) Fix rebase issues Created attachment 137077 [details] [review] Bug 23681: (QA follow-up) Allow for translations This patch allows for existing translations to follow through if debarment type descriptions are left unaltered for the default types. Created attachment 137078 [details] [review] Bug 23681: Updates for bug 26703 This patch fixes the title ordering and also replaces the breadcrumbs with the modern nav block equivilent Created attachment 137079 [details] [review] Bug 23681: (QA follow-up) Fix license, perlcritic & executable This patch fixes the simple issues mentioned by the QA script. Created attachment 137080 [details] [review] Bug 23681: (QA follow-up) Rename columns Renames ronly to readonly and dflt to is_system. Created attachment 137081 [details] [review] Bug 23681: (QA follow-up) Fix prefs Add the missing `.` and switch from `yes/no` to `1/0`. Created attachment 137082 [details] [review] Bug 23681: Fix QA issues This patch fixes following QA issues: - convert intaller files as .yml - change column name readonly as is_system - change column name is_system as default_value - add column can_be_added_manually (testplan for this below) - move syspref "PatronRestrictionTypes" to "Accounting > Features" tab - tweak page title - tweak tests to apply these changes Also atomicupdate files have been updated. Issues with delete and tests is fixed with adding additional schema change file. To test: 1) Add new restriction type and make it manual. 2) Navigate to patron details page. 3) Add new restriction to patron. => Only selectable restriction should be the one you just created. Also prove t/db_dependent/RestrictionTypes.t. Sponsored-by: Koha-Suomi Oy Created attachment 137083 [details] [review] Bug 23681: DO NOT PUSH! Schema changes To be able to delete and pass tests we need to declare koha_object[s]_class to DebarmentType.pm. Otherwise following error is raised: "Can't locate object method "_new_from_dbic" via package "Koha::DebarmentType" (perhaps you forgot to load "Koha::DebarmentType"?) at /kohadevbox/koha/Koha/Object.pm line 237" Created attachment 137084 [details] [review] Bug 23681: (QA follow-up) Move preference again This patch removes the new PatronRestrictionTypes preference from the accounting area in preference to creating a new group for 'Patron restriction' related preferences in the circulation preferences tab. Created attachment 137085 [details] [review] Bug 23681: (QA follow-up) Restore customisablity of description This patch restores the customisability of description for system restriction types. Translatability is not affected as you can translate the defaults via template translation and assuming the end users leave their descriptions at the default, the translations will be picked. Created attachment 137086 [details] [review] Bug 23681: (QA follow-up) Merge update files This patch merges the three atomic update files into one and also adds a check for foreing key existance to make the update idempotent. Created attachment 137087 [details] [review] Bug 23681: (QA follow-up) Proper handling of default option This patch removes the 'can_be_added_manually' flag. Only non-system restriction types can be added manually, so we exclude is_system instead of having two flags. (And we set the 'Manual' that's added at install time to default but not system). We then add proper handling for setting the default manual restriction type in the management page and set the dropdown list to use that value by default. Created attachment 137088 [details] [review] Bug 23681: DBIC Rebuild Created attachment 137089 [details] [review] Bug 23681: (follow-up) Mark fields at booleans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 137090 [details] [review] Bug 23681: (QA follow-up) Add patron restrictions to admin menu This patch adds the patron restrictions page to the admin menu that appears to the left on all administration pages. Created attachment 137091 [details] [review] Bug 23681: (QA follow-up) Remove spacing and add question mark This patch removes the superflous whitespace around the restriction descriptions and also adds the missing question mark into the delete question. Created attachment 137092 [details] [review] Bug 23681: (QA follow-up) Match default data to update The default data at install time didn't match the data added at update. This patch corrects that and by doing so it also adds back the ability to make MANUAL restriction types the default after any other type has been made default. Created attachment 137093 [details] [review] Bug 23681: (QA follow-up) Fix debarment count This patch fixes the debarment count displayed on the circulation page tabs Created attachment 137094 [details] [review] Bug 23681: (QA follow-up) Fix memberentry page display We were missing some changes to the restriction types dropdown handling in the memberentry template. (In reply to Katrin Fischer from comment #224) > 1) The new patron restrictions configuration page is missing from the > Administration side menu, visible when you are on any of the admin pages. > It's ok on the start page of the module. Fixed in follow-up > 2) The delete confirmation question is missing a question mark :) (also > might want to remove the additional spaces) > Are you sure you want to delete " Test " Fixed in follow-up > I'd also make the button just "Delete" or "Yes, delete", but that's a > personal preference and I see that this was 'copied' from how deleting a > patron category looks like. Hmm, I'd love to see us standardise on something and create a guideline about it.. at that time I'd be keen to go through and get everything consistent. > 3) After installing/updating with this patch, MANUAL is marked es the > default. You cannot change it, until you have added a new restriction, which > makes sense, as the system internal ones should not be default. But: you > cannot ever change the default back to MANUAL. I think this is because > system internal values are excluded, but it might raise complaints for > people experimenting with the feature that want to restore previous behavior. I discovered the update and the install data were out of sync.. fixed in a follow-up. This resolves the defaulting issue. > 4) Just a comment: Love the new Patron restrictions sub heading in patron > preferences :) Glad you like it :) > 5) When adding a manual restriction to the patron account, the default seems > to be only used when PatronRestrictionTypes is off, when it's on and the > pull down shows, the default is not preselected, but the first entry is. Hmm, I couldn't replicate this. > 6) Something super odd: In the patron account in the staff interface, on the > checkout tab, the Restrictions shows as (0), while on the details tab the > restrictions show the correct number (2) I have added. Only the tab heading > is wrong, otherwise the restrictions show ok. Unrelated to this patchset I believe.. bit it is fixed in a follow-up now. > 7) When editing the patron record, the pull down for restriction type is > empty. Fixed in follow-up. > 8) I notice that the type itself doesn't show up in the OPAC or the note on > the patron account in staff, just the comments do. This is the case with and > without the patches, maybe something so consider later. Or we could imagine > adding a default comment? (no blocker, just loud thinking) Wow.. wierd.. I'd never noticed that.. can we submit a new bug for that one? (In reply to Martin Renvoize from comment #256) > (In reply to Katrin Fischer from comment #224) > > > I'd also make the button just "Delete" or "Yes, delete", but that's a > > personal preference and I see that this was 'copied' from how deleting a > > patron category looks like. > > Hmm, I'd love to see us standardise on something and create a guideline > about it.. at that time I'd be keen to go through and get everything > consistent. Agreed, I was just wondering, because in German such buttons tend to get huge. > > > 3) After installing/updating with this patch, MANUAL is marked es the > > default. You cannot change it, until you have added a new restriction, which > > makes sense, as the system internal ones should not be default. But: you > > cannot ever change the default back to MANUAL. I think this is because > > system internal values are excluded, but it might raise complaints for > > people experimenting with the feature that want to restore previous behavior. > > I discovered the update and the install data were out of sync.. fixed in a > follow-up. This resolves the defaulting issue. Good catch! Will retest. > > 5) When adding a manual restriction to the patron account, the default seems > > to be only used when PatronRestrictionTypes is off, when it's on and the > > pull down shows, the default is not preselected, but the first entry is. > > Hmm, I couldn't replicate this. I'll retest. > > 8) I notice that the type itself doesn't show up in the OPAC or the note on > > the patron account in staff, just the comments do. This is the case with and > > without the patches, maybe something so consider later. Or we could imagine > > adding a default comment? (no blocker, just loud thinking) > > Wow.. wierd.. I'd never noticed that.. can we submit a new bug for that one? Yes, loud thinking definitely warrants a separate bug. I'd also not noticed that before, but it's unchanged by this patch set. 0) QA script + unit tests RESOLVED 1) Side menu RESOLVED 2) Delete confirmation RESOLVED 3) Default behavior / MANUAL RESOLVED 4) :) 5) Pre-selection in pull down Retested and RESOLVED. 6) Number in restrictions tab RESOLVED 7) When editing the patron record, the pull down for restriction type is empty. RESOLVED 8) Show restriction type in places MOVE to separate bug later :) Created attachment 137102 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137103 [details] [review] Bug 23681: Add debarment_types This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137104 [details] [review] Bug 23681: Add modify_patron_restrictions perm This patch adds a new "modify_patron_restrictions" permission Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137105 [details] [review] Bug 23681: Add Koha::RestrictionType(s) This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137106 [details] [review] Bug 23681: Add management UI This patch adds UI to allow CRUD operations on restriction types Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137107 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137108 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137109 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137110 [details] [review] Bug 23681: Add unit tests Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137111 [details] [review] Bug 23681: (follow-up) Fix typo Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Sponsored-by: Loughborough University Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137112 [details] [review] Bug 23681: (follow-up) Fix rebase issues Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137113 [details] [review] Bug 23681: (QA follow-up) Allow for translations This patch allows for existing translations to follow through if debarment type descriptions are left unaltered for the default types. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137114 [details] [review] Bug 23681: Updates for bug 26703 This patch fixes the title ordering and also replaces the breadcrumbs with the modern nav block equivilent Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137115 [details] [review] Bug 23681: (QA follow-up) Fix license, perlcritic & executable This patch fixes the simple issues mentioned by the QA script. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137116 [details] [review] Bug 23681: (QA follow-up) Rename columns Renames ronly to readonly and dflt to is_system. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137117 [details] [review] Bug 23681: (QA follow-up) Fix prefs Add the missing `.` and switch from `yes/no` to `1/0`. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137118 [details] [review] Bug 23681: Fix QA issues This patch fixes following QA issues: - convert intaller files as .yml - change column name readonly as is_system - change column name is_system as default_value - add column can_be_added_manually (testplan for this below) - move syspref "PatronRestrictionTypes" to "Accounting > Features" tab - tweak page title - tweak tests to apply these changes Also atomicupdate files have been updated. Issues with delete and tests is fixed with adding additional schema change file. To test: 1) Add new restriction type and make it manual. 2) Navigate to patron details page. 3) Add new restriction to patron. => Only selectable restriction should be the one you just created. Also prove t/db_dependent/RestrictionTypes.t. Sponsored-by: Koha-Suomi Oy Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137119 [details] [review] Bug 23681: DO NOT PUSH! Schema changes To be able to delete and pass tests we need to declare koha_object[s]_class to DebarmentType.pm. Otherwise following error is raised: "Can't locate object method "_new_from_dbic" via package "Koha::DebarmentType" (perhaps you forgot to load "Koha::DebarmentType"?) at /kohadevbox/koha/Koha/Object.pm line 237" Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137120 [details] [review] Bug 23681: (QA follow-up) Move preference again This patch removes the new PatronRestrictionTypes preference from the accounting area in preference to creating a new group for 'Patron restriction' related preferences in the circulation preferences tab. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137121 [details] [review] Bug 23681: (QA follow-up) Restore customisablity of description This patch restores the customisability of description for system restriction types. Translatability is not affected as you can translate the defaults via template translation and assuming the end users leave their descriptions at the default, the translations will be picked. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137122 [details] [review] Bug 23681: (QA follow-up) Merge update files This patch merges the three atomic update files into one and also adds a check for foreing key existance to make the update idempotent. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137123 [details] [review] Bug 23681: (QA follow-up) Proper handling of default option This patch removes the 'can_be_added_manually' flag. Only non-system restriction types can be added manually, so we exclude is_system instead of having two flags. (And we set the 'Manual' that's added at install time to default but not system). We then add proper handling for setting the default manual restriction type in the management page and set the dropdown list to use that value by default. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137124 [details] [review] Bug 23681: DBIC Rebuild Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137125 [details] [review] Bug 23681: (follow-up) Mark fields at booleans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137126 [details] [review] Bug 23681: (QA follow-up) Add patron restrictions to admin menu This patch adds the patron restrictions page to the admin menu that appears to the left on all administration pages. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137127 [details] [review] Bug 23681: (QA follow-up) Remove spacing and add question mark This patch removes the superflous whitespace around the restriction descriptions and also adds the missing question mark into the delete question. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137128 [details] [review] Bug 23681: (QA follow-up) Match default data to update The default data at install time didn't match the data added at update. This patch corrects that and by doing so it also adds back the ability to make MANUAL restriction types the default after any other type has been made default. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137129 [details] [review] Bug 23681: (QA follow-up) Fix debarment count This patch fixes the debarment count displayed on the circulation page tabs Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 137130 [details] [review] Bug 23681: (QA follow-up) Fix memberentry page display We were missing some changes to the restriction types dropdown handling in the memberentry template. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Can you please rebase? I'll make sure I take care of it once you rebase to avoid further delays. I would like to see some squashing as well. But not trying to complicate things too much. Created attachment 139250 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139251 [details] [review] Bug 23681: Add debarment_types table This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139252 [details] [review] Bug 23681: Add modify_patron_restrictions permission This patch adds a new "modify_patron_restrictions" permission Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139253 [details] [review] Bug 23681: Add Koha Classes This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139254 [details] [review] Bug 23681: Add Management UI This patch adds UI to allow CRUD operations on restriction types Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139255 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139256 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139257 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139258 [details] [review] Bug 23681: Add unit tests Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139259 [details] [review] Bug 23681: (QA follow-up) Allow for translations This patch allows for existing translations to follow through if debarment type descriptions are left unaltered for the default types. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139260 [details] [review] Bug 23681: Updates for bug 26703 This patch fixes the title ordering and also replaces the breadcrumbs with the modern nav block equivilent Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139261 [details] [review] Bug 23681: (QA follow-up) Fix license, perlcritic & executable This patch fixes the simple issues mentioned by the QA script. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139262 [details] [review] Bug 23681: (QA follow-up) Fix preferences Add the missing `.` and switch from `yes/no` to `1/0`. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139263 [details] [review] Bug 23681: (QA follow-up) Rename columns Renames ronly to readonly and dflt to is_system. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139264 [details] [review] Bug 23681: Fix QA issues This patch fixes following QA issues: - convert intaller files as .yml - change column name readonly as is_system - change column name is_system as default_value - add column can_be_added_manually (testplan for this below) - move syspref "PatronRestrictionTypes" to "Accounting > Features" tab - tweak page title - tweak tests to apply these changes Also atomicupdate files have been updated. Issues with delete and tests is fixed with adding additional schema change file. To test: 1) Add new restriction type and make it manual. 2) Navigate to patron details page. 3) Add new restriction to patron. => Only selectable restriction should be the one you just created. Also prove t/db_dependent/RestrictionTypes.t. Sponsored-by: Koha-Suomi Oy Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139265 [details] [review] Bug 23681: DBIC Schema To be able to delete and pass tests we need to declare koha_object[s]_class to DebarmentType.pm. Otherwise following error is raised: "Can't locate object method "_new_from_dbic" via package "Koha::DebarmentType" (perhaps you forgot to load "Koha::DebarmentType"?) at /kohadevbox/koha/Koha/Object.pm line 237" Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139266 [details] [review] Bug 23681: (QA follow-up) Move preference again This patch removes the new PatronRestrictionTypes preference from the accounting area in preference to creating a new group for 'Patron restriction' related preferences in the circulation preferences tab. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139267 [details] [review] Bug 23681: (QA follow-up) Restore customisablity of description This patch restores the customisability of description for system restriction types. Translatability is not affected as you can translate the defaults via template translation and assuming the end users leave their descriptions at the default, the translations will be picked. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139268 [details] [review] Bug 23681: (QA follow-up) Merge update files This patch merges the three atomic update files into one and also adds a check for foreing key existance to make the update idempotent. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139269 [details] [review] Bug 23681: (QA follow-up) Proper handling of default option This patch removes the 'can_be_added_manually' flag. Only non-system restriction types can be added manually, so we exclude is_system instead of having two flags. (And we set the 'Manual' that's added at install time to default but not system). We then add proper handling for setting the default manual restriction type in the management page and set the dropdown list to use that value by default. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139270 [details] [review] Bug 23681: (follow-up) Mark fields at booleans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139271 [details] [review] Bug 23681: (QA follow-up) Add patron restrictions to admin menu This patch adds the patron restrictions page to the admin menu that appears to the left on all administration pages. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139272 [details] [review] Bug 23681: (QA follow-up) Remove spacing and add question mark This patch removes the superflous whitespace around the restriction descriptions and also adds the missing question mark into the delete question. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139273 [details] [review] Bug 23681: (QA follow-up) Fix memberentry page display We were missing some changes to the restriction types dropdown handling in the memberentry template. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Rebased and squashed as much as I can easily squash.. hopefully that's OK. Created attachment 139509 [details] [review] Bug 23681: (QA follow-up) debarment_types => restriction_types The code moved from *debarments* to *restrictions* but the table didn't. This patch just renames things accordingly. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 139512 [details] [review] Bug 23681: (QA follow-up) New schema files Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Ok, this is almost done. I added some required follow-ups. But the remaining things are too much for me to take care right now, so failing it and adding my (not so big) list: - The test files should be moved to Koha/RestrictionType*.t [MINOR] - The tests do not cover all the things. They are also mixed. The RestrictionType.t file tests Koha::RestrictionTypes->keyed_on_code - Koha::RestrictionTypes->keyed_on_code is not really required... and is very different than our normal return values... The fact we need to delete everything in order to test it points to some design issue (i.e. it is not a method that acts on $self as a resultset, but works in the whole table. - keyed_on_code should be replaced by something along the lines of: { map { $_->{code} => $_ } $self->as_list } - Koha::RestrictionType->make_default looks great :-D - Koha::RestrictionType->delete has problems. We *could* accept it using plain SQL, but the logic is wrong. If the type is a system one, and exception should be thrown. The same if it is the default ;-) (i.e. it is setting itself as default (again) and then deleting itself). As you can see, we are close to pushing this. Please hurry so no rebases are needed. Created attachment 139649 [details] [review] Bug 23681: Move to ::Patron::Restriction::Type(s) This patch moves the new classes under ::Patron::Restriction:: and enhances the Unit tests for those classes. NOTE: We should drop keyed_on_code as part of bug 31095 OK, that last patch handles most issues outlined above with the exception of touching keyed_on_code. I found that I'd need to write a fair bit of code to work around that implimentation and I felt it more pertinant to help quickly follow this bug by working on a cleanup in bug 31095 where I intend to drop GetDebarmentTypes and therefore the need for keyed_on_code entirely.. it'll end up much more like the Accounts code using relations properly. Created attachment 139717 [details] [review] Bug 23681: Move to ::Patron::Restriction::Type(s) This patch moves the new classes under ::Patron::Restriction:: and enhances the Unit tests for those classes. NOTE: We should drop keyed_on_code as part of bug 31095 Created attachment 139768 [details] [review] Bug 23681: Add PatronRestrictionTypes syspref This patch adds the new syspref "PatronRestrictionTypes" Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139769 [details] [review] Bug 23681: Add debarment_types table This patch adds the table debarment_types and changes borrower_debarments.type to be a foreign key to it Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139770 [details] [review] Bug 23681: Add modify_patron_restrictions permission This patch adds a new "modify_patron_restrictions" permission Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139771 [details] [review] Bug 23681: Add Koha Classes This patch adds the following objects: Koha::RestrictionType Koha::RestrictionTypes Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139772 [details] [review] Bug 23681: Add Management UI This patch adds UI to allow CRUD operations on restriction types Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139773 [details] [review] Bug 23681: Allow for changes to debarments The structure of debarments has changes slightly in that the displayed text is now a product of a call to Koha::RestrictionTypes rather than just the debarment's code. This patch allows for that Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139774 [details] [review] Bug 23681: Allow for selection of restriction type This patch displays a restriction type select box (when appropriate) when adding manual patron restrictions Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139775 [details] [review] Bug 23681: Clean up on delete When a patron restriction type is deleted, any debarments that use that type need to revert to the default type, this patch implements this behaviour Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139776 [details] [review] Bug 23681: Add unit tests Sponsored-by: Loughborough University Signed-off-by: Benjamin Veasey <B.T.Veasey@lboro.ac.uk> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139777 [details] [review] Bug 23681: (QA follow-up) Allow for translations This patch allows for existing translations to follow through if debarment type descriptions are left unaltered for the default types. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139778 [details] [review] Bug 23681: Updates for bug 26703 This patch fixes the title ordering and also replaces the breadcrumbs with the modern nav block equivilent Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139779 [details] [review] Bug 23681: (QA follow-up) Fix license, perlcritic & executable This patch fixes the simple issues mentioned by the QA script. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139780 [details] [review] Bug 23681: (QA follow-up) Fix preferences Add the missing `.` and switch from `yes/no` to `1/0`. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139781 [details] [review] Bug 23681: (QA follow-up) Rename columns Renames ronly to readonly and dflt to is_system. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139782 [details] [review] Bug 23681: Fix QA issues This patch fixes following QA issues: - convert intaller files as .yml - change column name readonly as is_system - change column name is_system as default_value - add column can_be_added_manually (testplan for this below) - move syspref "PatronRestrictionTypes" to "Accounting > Features" tab - tweak page title - tweak tests to apply these changes Also atomicupdate files have been updated. Issues with delete and tests is fixed with adding additional schema change file. To test: 1) Add new restriction type and make it manual. 2) Navigate to patron details page. 3) Add new restriction to patron. => Only selectable restriction should be the one you just created. Also prove t/db_dependent/RestrictionTypes.t. Sponsored-by: Koha-Suomi Oy Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139783 [details] [review] Bug 23681: DBIC Schema To be able to delete and pass tests we need to declare koha_object[s]_class to DebarmentType.pm. Otherwise following error is raised: "Can't locate object method "_new_from_dbic" via package "Koha::DebarmentType" (perhaps you forgot to load "Koha::DebarmentType"?) at /kohadevbox/koha/Koha/Object.pm line 237" Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139784 [details] [review] Bug 23681: (QA follow-up) Move preference again This patch removes the new PatronRestrictionTypes preference from the accounting area in preference to creating a new group for 'Patron restriction' related preferences in the circulation preferences tab. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139785 [details] [review] Bug 23681: (QA follow-up) Restore customisablity of description This patch restores the customisability of description for system restriction types. Translatability is not affected as you can translate the defaults via template translation and assuming the end users leave their descriptions at the default, the translations will be picked. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139786 [details] [review] Bug 23681: (QA follow-up) Merge update files This patch merges the three atomic update files into one and also adds a check for foreing key existance to make the update idempotent. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139787 [details] [review] Bug 23681: (QA follow-up) Proper handling of default option This patch removes the 'can_be_added_manually' flag. Only non-system restriction types can be added manually, so we exclude is_system instead of having two flags. (And we set the 'Manual' that's added at install time to default but not system). We then add proper handling for setting the default manual restriction type in the management page and set the dropdown list to use that value by default. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139788 [details] [review] Bug 23681: (follow-up) Mark fields at booleans Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139789 [details] [review] Bug 23681: (QA follow-up) Add patron restrictions to admin menu This patch adds the patron restrictions page to the admin menu that appears to the left on all administration pages. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139790 [details] [review] Bug 23681: (QA follow-up) Remove spacing and add question mark This patch removes the superflous whitespace around the restriction descriptions and also adds the missing question mark into the delete question. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139791 [details] [review] Bug 23681: (QA follow-up) Fix memberentry page display We were missing some changes to the restriction types dropdown handling in the memberentry template. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 139792 [details] [review] Bug 23681: (QA follow-up) debarment_types => restriction_types The code moved from *debarments* to *restrictions* but the table didn't. This patch just renames things accordingly. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 139793 [details] [review] Bug 23681: (QA follow-up) New schema files Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 139794 [details] [review] Bug 23681: Move to ::Patron::Restriction::Type(s) This patch moves the new classes under ::Patron::Restriction:: and enhances the Unit tests for those classes. NOTE: We should drop keyed_on_code as part of bug 31095 Created attachment 139795 [details] [review] Bug 23681: (follow-up) Re-fix ::Patron::Restriction::Type(s) Pushed to master for 22.11. Nice work everyone, thanks! |