Bug 23681 - Patron restrictions should be user definable
Summary: Patron restrictions should be user definable
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Andrew Isherwood
QA Contact: Jonathan Druart
URL:
Keywords: rel_20_05_target
: 6775 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-09-26 13:59 UTC by Andrew Isherwood
Modified: 2020-04-01 13:27 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23681: Add PatronRestrictionTypes syspref (3.33 KB, patch)
2019-10-07 09:42 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add debarment_types (5.39 KB, patch)
2019-10-07 09:42 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add modify_patron_restrictions perm (5.26 KB, patch)
2019-10-07 09:42 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add Koha::RestrictionType(s) (3.32 KB, patch)
2019-10-07 09:42 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add management UI (14.42 KB, patch)
2019-10-07 09:42 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for changes to debarments (5.59 KB, patch)
2019-10-07 09:42 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for selection of restriction type (5.88 KB, patch)
2019-10-07 09:42 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Clean up on delete (1.88 KB, patch)
2019-10-07 09:42 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add unit tests (4.98 KB, patch)
2019-10-07 09:42 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add PatronRestrictionTypes syspref (3.33 KB, patch)
2019-10-07 10:03 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add debarment_types (5.39 KB, patch)
2019-10-07 10:03 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add modify_patron_restrictions perm (5.26 KB, patch)
2019-10-07 10:03 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add Koha::RestrictionType(s) (3.41 KB, patch)
2019-10-07 10:03 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add management UI (14.44 KB, patch)
2019-10-07 10:03 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for changes to debarments (5.59 KB, patch)
2019-10-07 10:03 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for selection of restriction type (5.91 KB, patch)
2019-10-07 10:03 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Clean up on delete (2.50 KB, patch)
2019-10-07 10:03 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add unit tests (4.98 KB, patch)
2019-10-07 10:03 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add PatronRestrictionTypes syspref (3.44 KB, patch)
2019-10-30 11:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add debarment_types (5.39 KB, patch)
2019-10-30 11:52 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add modify_patron_restrictions perm (5.23 KB, patch)
2019-10-30 11:53 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add Koha::RestrictionType(s) (3.41 KB, patch)
2019-10-30 11:53 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add management UI (14.44 KB, patch)
2019-10-30 11:53 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for changes to debarments (5.56 KB, patch)
2019-10-30 11:53 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for selection of restriction type (5.91 KB, patch)
2019-10-30 11:53 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Clean up on delete (2.50 KB, patch)
2019-10-30 11:53 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add unit tests (4.98 KB, patch)
2019-10-30 11:53 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add PatronRestrictionTypes syspref (3.44 KB, patch)
2019-10-31 10:15 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add debarment_types (5.39 KB, patch)
2019-10-31 10:15 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add modify_patron_restrictions perm (5.23 KB, patch)
2019-10-31 10:15 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add Koha::RestrictionType(s) (3.41 KB, patch)
2019-10-31 10:16 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add management UI (14.44 KB, patch)
2019-10-31 10:16 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for changes to debarments (5.56 KB, patch)
2019-10-31 10:16 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for selection of restriction type (5.91 KB, patch)
2019-10-31 10:16 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Clean up on delete (2.50 KB, patch)
2019-10-31 10:16 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add unit tests (4.98 KB, patch)
2019-10-31 10:16 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: (follow-up) Fix typo (691 bytes, patch)
2019-11-07 11:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add PatronRestrictionTypes syspref (3.53 KB, patch)
2019-11-11 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add debarment_types (5.48 KB, patch)
2019-11-11 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add modify_patron_restrictions perm (5.33 KB, patch)
2019-11-11 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add Koha::RestrictionType(s) (3.51 KB, patch)
2019-11-11 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add management UI (14.53 KB, patch)
2019-11-11 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for changes to debarments (5.65 KB, patch)
2019-11-11 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for selection of restriction type (6.00 KB, patch)
2019-11-11 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Clean up on delete (2.59 KB, patch)
2019-11-11 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add unit tests (5.08 KB, patch)
2019-11-11 12:46 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: (follow-up) Fix typo (785 bytes, patch)
2019-11-11 12:46 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add PatronRestrictionTypes syspref (3.53 KB, patch)
2020-03-02 11:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add debarment_types (5.48 KB, patch)
2020-03-02 11:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add modify_patron_restrictions perm (5.31 KB, patch)
2020-03-02 11:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add Koha::RestrictionType(s) (3.51 KB, patch)
2020-03-02 11:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add management UI (14.53 KB, patch)
2020-03-02 11:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for changes to debarments (5.93 KB, patch)
2020-03-02 11:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Allow for selection of restriction type (6.08 KB, patch)
2020-03-02 11:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Clean up on delete (2.59 KB, patch)
2020-03-02 11:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: Add unit tests (5.08 KB, patch)
2020-03-02 11:28 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23681: (follow-up) Fix typo (785 bytes, patch)
2020-03-02 11:28 UTC, Andrew Isherwood
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2019-09-26 13:59:33 UTC
Currently the patron restrictions are hard coded as enums in borrower_debarments.type, the available values are 'SUSPENSION','OVERDUES','MANUAL','DISCHARGE'.

We (PTFS Europe) have a customer requirement to:

- Provide a select box when adding a patron restriction containing user defined values for restriction types, the user defined values should be defined as authorised values
- Ensure a default value if no restriction type is selected
- Allow the name of the default value to be modifiable
- Ensure that if a restriction type authorised value is removed, any restrictions that use it should revert to the default value
- Make the selection of restriction type optional via a syspref

Currently, at the DB level, the default value is 'MANUAL'.

The plan is to:

DB MIGRATION
------------
- Add authorised_value_categories row for 'PATRON_RESTRICTIONS'
- Add authorised_values.core, default value 0
  - Implies AV cannot be deleted and authorised_values.authorised_value cannot be changed
- Add new borrower_debarments.debarment_type column
- Select distinct values from borrower_debarments.type
- For each $value:
  - Create AV rows:
    - authorised_values.authorised_value = $value
    - authorised_values.lib = $value
    - authorised_values.core = 1
  - ** Note id of each new row, associated with $value
  - update borrower_debarments.debarment_type to be appropriate AV id
  - Remove borrower_debarments.type

OTHER STUFF
-----------

- AV UI (and anything else that can bring about AV modification), should respect "core"
- Modify Patron::Debarments appropriate functions to abstract underlying changes i.e. we maintain backward compatibility with the way they currently work, we just obfuscate the fact that the "type" comes from a different table
- Overload AuthorisedValue::delete to allow us to call optional functions after delete - in a sane world this would be a trigger. This will allow us to set appropriate patron_debarments.debarment_type rows to default value upon AV deletion
Comment 1 Andrew Isherwood 2019-10-01 09:49:44 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"
Comment 2 Andrew Isherwood 2019-10-07 09:42:00 UTC
Created attachment 93769 [details] [review]
Bug 23681: Add PatronRestrictionTypes syspref

This patch adds the new syspref "PatronRestrictionTypes"
Comment 3 Andrew Isherwood 2019-10-07 09:42:03 UTC
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
Comment 4 Andrew Isherwood 2019-10-07 09:42:06 UTC
Created attachment 93771 [details] [review]
Bug 23681: Add modify_patron_restrictions perm

This patch adds a new "modify_patron_restrictions" permission
Comment 5 Andrew Isherwood 2019-10-07 09:42:08 UTC
Created attachment 93772 [details] [review]
Bug 23681: Add Koha::RestrictionType(s)

This patch adds the following objects:

Koha::RestrictionType
Koha::RestrictionTypes
Comment 6 Andrew Isherwood 2019-10-07 09:42:11 UTC
Created attachment 93773 [details] [review]
Bug 23681: Add management UI

This patch adds UI to allow CRUD operations on restriction types
Comment 7 Andrew Isherwood 2019-10-07 09:42:14 UTC
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
Comment 8 Andrew Isherwood 2019-10-07 09:42:17 UTC
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
Comment 9 Andrew Isherwood 2019-10-07 09:42:20 UTC
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
Comment 10 Andrew Isherwood 2019-10-07 09:42:23 UTC
Created attachment 93777 [details] [review]
Bug 23681: Add unit tests
Comment 11 Andrew Isherwood 2019-10-07 09:42:44 UTC
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
Comment 12 Andrew Isherwood 2019-10-07 10:03:19 UTC
Created attachment 93778 [details] [review]
Bug 23681: Add PatronRestrictionTypes syspref

This patch adds the new syspref "PatronRestrictionTypes"
Comment 13 Andrew Isherwood 2019-10-07 10:03:22 UTC
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
Comment 14 Andrew Isherwood 2019-10-07 10:03:25 UTC
Created attachment 93780 [details] [review]
Bug 23681: Add modify_patron_restrictions perm

This patch adds a new "modify_patron_restrictions" permission
Comment 15 Andrew Isherwood 2019-10-07 10:03:28 UTC
Created attachment 93781 [details] [review]
Bug 23681: Add Koha::RestrictionType(s)

This patch adds the following objects:

Koha::RestrictionType
Koha::RestrictionTypes
Comment 16 Andrew Isherwood 2019-10-07 10:03:31 UTC
Created attachment 93782 [details] [review]
Bug 23681: Add management UI

This patch adds UI to allow CRUD operations on restriction types
Comment 17 Andrew Isherwood 2019-10-07 10:03:34 UTC
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
Comment 18 Andrew Isherwood 2019-10-07 10:03:37 UTC
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
Comment 19 Andrew Isherwood 2019-10-07 10:03:40 UTC
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
Comment 20 Andrew Isherwood 2019-10-07 10:03:43 UTC
Created attachment 93786 [details] [review]
Bug 23681: Add unit tests
Comment 21 Kyle M Hall 2019-10-09 13:04:16 UTC
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!
Comment 22 Andrew Isherwood 2019-10-09 13:20:13 UTC
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!
Comment 23 Martin Renvoize 2019-10-10 07:46:15 UTC
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.
Comment 24 Andrew Isherwood 2019-10-30 11:52:54 UTC
Created attachment 94862 [details] [review]
Bug 23681: Add PatronRestrictionTypes syspref

This patch adds the new syspref "PatronRestrictionTypes"
Comment 25 Andrew Isherwood 2019-10-30 11:52:58 UTC
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
Comment 26 Andrew Isherwood 2019-10-30 11:53:01 UTC
Created attachment 94864 [details] [review]
Bug 23681: Add modify_patron_restrictions perm

This patch adds a new "modify_patron_restrictions" permission
Comment 27 Andrew Isherwood 2019-10-30 11:53:04 UTC
Created attachment 94865 [details] [review]
Bug 23681: Add Koha::RestrictionType(s)

This patch adds the following objects:

Koha::RestrictionType
Koha::RestrictionTypes
Comment 28 Andrew Isherwood 2019-10-30 11:53:07 UTC
Created attachment 94866 [details] [review]
Bug 23681: Add management UI

This patch adds UI to allow CRUD operations on restriction types
Comment 29 Andrew Isherwood 2019-10-30 11:53:10 UTC
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
Comment 30 Andrew Isherwood 2019-10-30 11:53:14 UTC
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
Comment 31 Andrew Isherwood 2019-10-30 11:53:19 UTC
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
Comment 32 Andrew Isherwood 2019-10-30 11:53:22 UTC
Created attachment 94870 [details] [review]
Bug 23681: Add unit tests
Comment 33 Andrew Isherwood 2019-10-31 10:15:46 UTC
Created attachment 94896 [details] [review]
Bug 23681: Add PatronRestrictionTypes syspref

This patch adds the new syspref "PatronRestrictionTypes"
Comment 34 Andrew Isherwood 2019-10-31 10:15:50 UTC
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
Comment 35 Andrew Isherwood 2019-10-31 10:15:55 UTC
Created attachment 94898 [details] [review]
Bug 23681: Add modify_patron_restrictions perm

This patch adds a new "modify_patron_restrictions" permission
Comment 36 Andrew Isherwood 2019-10-31 10:16:00 UTC
Created attachment 94899 [details] [review]
Bug 23681: Add Koha::RestrictionType(s)

This patch adds the following objects:

Koha::RestrictionType
Koha::RestrictionTypes
Comment 37 Andrew Isherwood 2019-10-31 10:16:05 UTC
Created attachment 94900 [details] [review]
Bug 23681: Add management UI

This patch adds UI to allow CRUD operations on restriction types
Comment 38 Andrew Isherwood 2019-10-31 10:16:09 UTC
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
Comment 39 Andrew Isherwood 2019-10-31 10:16:13 UTC
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
Comment 40 Andrew Isherwood 2019-10-31 10:16:18 UTC
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
Comment 41 Andrew Isherwood 2019-10-31 10:16:23 UTC
Created attachment 94904 [details] [review]
Bug 23681: Add unit tests
Comment 42 Andrew Isherwood 2019-11-07 11:47:29 UTC
Created attachment 95148 [details] [review]
Bug 23681: (follow-up) Fix typo
Comment 43 Magnus Enger 2019-11-07 13:34:32 UTC
*** Bug 6775 has been marked as a duplicate of this bug. ***
Comment 44 Andrew Isherwood 2019-11-11 12:45:15 UTC
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
Comment 45 Andrew Isherwood 2019-11-11 12:45:20 UTC
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
Comment 46 Andrew Isherwood 2019-11-11 12:45:25 UTC
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
Comment 47 Andrew Isherwood 2019-11-11 12:45:31 UTC
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
Comment 48 Andrew Isherwood 2019-11-11 12:45:37 UTC
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
Comment 49 Andrew Isherwood 2019-11-11 12:45:43 UTC
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
Comment 50 Andrew Isherwood 2019-11-11 12:45:48 UTC
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
Comment 51 Andrew Isherwood 2019-11-11 12:45:54 UTC
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
Comment 52 Andrew Isherwood 2019-11-11 12:46:00 UTC
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
Comment 53 Andrew Isherwood 2019-11-11 12:46:05 UTC
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
Comment 54 Jonathan Druart 2020-01-23 10:21:59 UTC
(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).
Comment 55 Jonathan Druart 2020-01-23 10:28:19 UTC
So you are adding a regression, the restrictions are no longer translatable.
Comment 56 Jonathan Druart 2020-01-23 10:30:45 UTC
Is the "default" flag really useful? We could imagine a new "internal" flag (to replace ronly) for AVs.
Comment 57 Andrew Isherwood 2020-01-27 09:26:55 UTC
(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.
Comment 58 Jonathan Druart 2020-01-27 10:28:49 UTC
(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).
Comment 59 Andrew Isherwood 2020-01-27 12:29:14 UTC
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.
Comment 60 Andrew Isherwood 2020-03-02 11:27:34 UTC
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
Comment 61 Andrew Isherwood 2020-03-02 11:27:37 UTC
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
Comment 62 Andrew Isherwood 2020-03-02 11:27:40 UTC
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
Comment 63 Andrew Isherwood 2020-03-02 11:27:44 UTC
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
Comment 64 Andrew Isherwood 2020-03-02 11:27:47 UTC
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
Comment 65 Andrew Isherwood 2020-03-02 11:27:51 UTC
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
Comment 66 Andrew Isherwood 2020-03-02 11:27:54 UTC
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
Comment 67 Andrew Isherwood 2020-03-02 11:27:58 UTC
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
Comment 68 Andrew Isherwood 2020-03-02 11:28:01 UTC
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
Comment 69 Andrew Isherwood 2020-03-02 11:28:05 UTC
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
Comment 70 Jonathan Druart 2020-04-01 10:58:16 UTC
(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.
Comment 71 Jonathan Druart 2020-04-01 13:27:15 UTC
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.