Bug 20028

Summary: Export all patron related personal data in one package
Product: Koha Reporter: Michal Denar <black23>
Component: PatronsAssignee: Lari Taskula <lari.taskula>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: black23, claire.hernandez, dcook, fridolin.somers, george, gmcharlt, J.P.Knight, johanna.raisa, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, lari.taskula, m.de.rooy, magnus, martin.renvoize, michaela.sieber, mirko, niamh.walker-headon, pablo.bianchi, paul.poulain, rbit
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 18081    
Attachments: Bug 20028: Export all patron related personal data in one package
Bug 20028: Export all patron related personal data in one package
Bug 20028: Export all patron related personal data in one package
Bug 20028: Export all patron related personal data in one package
Bug 20028: Add Koha::Patron->takeout
Bug 20028: Add Koha::Patron->takeout
Bug 20028: Add Koha::Patron->export
Bug 20028: Add a system preference for patron data export
Bug 20028: Add OpenAPI spec
Bug 20028: Add Patron Export REST controller
Bug 20028: Add Koha::Patron->export
Bug 20028: Add a system preference for patron data export
Bug 20028: Add OpenAPI spec
Bug 20028: Add Patron Export REST controller
Bug 20028: Add Patron Export REST controller
Bug 20028: Add Koha::Patron->export
Bug 20028: Add a system preference for patron data export
Bug 20028: Add OpenAPI spec
Bug 20028: Add Patron Export REST controller
Bug 20028: Add Koha::Patron->export
Bug 20028: Add a system preference for patron data export
Bug 20028: Add OpenAPI spec
Bug 20028: Add Patron Export REST controller
Bug 20028: Add Koha::Patron->export
Bug 20028: Add a system preference for patron data export
Bug 20028: Add OpenAPI spec
Bug 20028: Add Patron Export REST controller
Bug 20028: (QA follow-up) Fix test plan for Patron.t
Bug 20028: (QA follow-up) Add atomic update for new pref
Bug 20028: Add Koha::Patron->export
Bug 20028: Add a system preference for patron data export
Bug 20028: Add Patron Export REST controller
Bug 20028: (QA follow-up) Fix test plan for Patron.t
Bug 20028: (QA follow-up) Add atomic update for new pref

Description Michal Denar 2018-01-18 10:51:08 UTC
At least in CVS, maybe in JSON or other machnine readable formats.
Comment 1 Josef Moravec 2018-02-26 21:53:47 UTC
We discussed it on GDPR meeting and agreed it could be done as API endpoint with access via a button in staff client and opac.

By the GDPR, the data should be in commonly used machine readable format. So JSON from REST API is OK.
Comment 2 Jon Knight 2018-05-29 10:06:49 UTC
Created attachment 75622 [details] [review]
Bug 20028: Export all patron related personal data in one package

Add a system preference to determine whether patron data export is permitted,
and if it is an extra option in the staff UI to initiate the export.  At this
stage the API for patron data isn't ready so as a placeholder it just goes
to the readingrec.pl script.

Testing plan:

* Apply patch.

* Turn the AllowGDPRPatronExport in the administration sysprefs

* Search for a patron (eg Mabel in the standard test data) and then
  click on the "More" button on the right hand side of the button bar.
  You should see "Export patron's GDPR data" option. Selecting that will
  currently just go to the readingrec.pl page.

* Turn off AllowGDPRPatronExport in the	 administration sysprefs

* The "Export patron's GDPR data" option should no long be visible from
  the "More" button in the patron page.

Full description of what the patch does
Comment 3 Jon Knight 2018-05-29 10:11:03 UTC
Note that my patch above is just the scaffolding to support this - it needs the APIs to extract all the data to be available and then call those.  The alternative is to write a new script to gather all the patron data in a single data structure and then return that as JSON.
Comment 4 Pablo AB 2018-08-16 15:09:12 UTC
Related: To export patron images on a package ready for upload I create [this script](https://wiki.koha-community.org/wiki/Debian_packages_script_library#Bulk_export_patrons_images). Also in bug 12996.
Comment 5 Jon Knight 2018-08-23 13:07:24 UTC
Created attachment 78095 [details] [review]
Bug 20028: Export all patron related personal data in one package

Modifies the readingrec.pl script to give the option to generate a JSON
encoded version of the data held about the patron. Currently exports basic
patron (borrower table) data, patron image, holds (reserves), old holds,
checkouts (issues), old checkouts and accounting information.

Test plan:

Turn on the syspref, select a patron from the staff intranet,
and then choose "Export patron's GDPR data" from the "More..." menu.  You
should then get a JSON file downloaded to the machine running your web
browser.  Compare the data in that file with the data shown on the
intranet displays for that patron.
Comment 6 Jon Knight 2018-09-25 14:06:00 UTC
Created attachment 79376 [details] [review]
Bug 20028: Export all patron related personal data in one package

Add a system preference to determine whether patron data export is permitted,
and if it is an extra option in the staff UI to initiate the export.  At this
stage the API for patron data isn't ready so as a placeholder it just goes
to the readingrec.pl script.

Testing plan:

* Apply patch.

* Turn the AllowGDPRPatronExport in the administration sysprefs

* Search for a patron (eg Mabel in the standard test data) and then
  click on the "More" button on the right hand side of the button bar.
  You should see "Export patron's GDPR data" option. Selecting that will
  generate and download a JSON file containing the patron's data to
  your client machine.

* Turn off AllowGDPRPatronExport in the	administration sysprefs

* The "Export patron's GDPR data" option should no long be visible from
  the "More" button in the patron page.
Comment 7 Michal Denar 2018-09-25 15:04:04 UTC
Created attachment 79381 [details] [review]
Bug 20028: Export all patron related personal data in one package

Add a system preference to determine whether patron data export is permitted,
and if it is an extra option in the staff UI to initiate the export.  At this
stage the API for patron data isn't ready so as a placeholder it just goes
to the readingrec.pl script.

Testing plan:

* Apply patch.

* Turn the AllowGDPRPatronExport in the administration sysprefs

* Search for a patron (eg Mabel in the standard test data) and then
  click on the "More" button on the right hand side of the button bar.
  You should see "Export patron's GDPR data" option. Selecting that will
  generate and download a JSON file containing the patron's data to
  your client machine.

* Turn off AllowGDPRPatronExport in the	administration sysprefs

* The "Export patron's GDPR data" option should no long be visible from
  the "More" button in the patron page.

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 8 Michal Denar 2018-09-25 15:07:55 UTC
Patch is working. But circulation history is collection of personal data too. We should extend functionality this way. It's first good step.

Thank You

Mike.
Comment 9 Michal Denar 2018-09-26 07:03:01 UTC
Jon,
sorry ... forgota my last comment. I tried export data from devbox with limited circulation history. So export contains limitex range of data, ofcourse. We talked about with Josef and debate around Your code. All relevant date are in export. 

Good work.
Comment 10 Jonathan Druart 2018-10-16 18:25:57 UTC
IMO it's not conceivable to push this code as it. We cannot have such code in .pl, without tests, etc.

Why not just a $patron->unblessed then use columns.def instead of listing of the patron's attributes?

Also found a typo: +	    'dateoffbirth' => $patron->dateofbirth,
Comment 11 Josef Moravec 2018-10-17 05:33:42 UTC
(In reply to Jonathan Druart from comment #10)
> IMO it's not conceivable to push this code as it. We cannot have such code
> in .pl, without tests, etc.
> 
> Why not just a $patron->unblessed then use columns.def instead of listing of
> the patron's attributes?

Hm, i am just thinking about using $RESULTSET_PATRON_ID_MAPPING from Koha::Patron and define the method Koha::Patron->takeout - does it make sense?

> 
> Also found a typo: +	    'dateoffbirth' => $patron->dateofbirth,
Comment 12 Jon Knight 2018-10-24 17:03:51 UTC
I'm not sure I understand how columns.def is going to useful in this case.  That file appears to be some sort of vaguely XML file mapping tables & columns to human readable names in a given language. However we don't want language dependent human readable attributes in the output - we're after consistent machine readable output for GDPR.
Comment 13 Marcel de Rooy 2018-11-02 10:43:24 UTC
(In reply to Jonathan Druart from comment #10)
> IMO it's not conceivable to push this code as it. We cannot have such code
> in .pl, without tests, etc.
> 
> Why not just a $patron->unblessed then use columns.def instead of listing of
> the patron's attributes?
> 
> Also found a typo: +	    'dateoffbirth' => $patron->dateofbirth,

Please adjust status if something fails qa ?
Comment 14 Marcel de Rooy 2018-11-02 10:44:11 UTC
Please move code to module. Makes it easier to test too.
Comment 15 Jonathan Druart 2018-11-02 12:26:26 UTC
(In reply to Marcel de Rooy from comment #14)
> Please move code to module. Makes it easier to test too.

I wanted to get other opinions.
Comment 16 Jon Knight 2019-08-21 14:29:26 UTC
I'm not sure what is now expected for me to take this forward? Were there other opinions? Is there a particular module this should be going in? Is it even wanted?
Comment 17 Magnus Enger 2019-08-22 19:40:13 UTC
(In reply to Jon Knight from comment #16)
> I'm not sure what is now expected for me to take this forward? Were there
> other opinions? Is there a particular module this should be going in? Is it
> even wanted?

I think the "verdict" from the QA-team is that the code needs to be moved to a module in the Koha namespace, with tests. 

I wonder if it would make sense to implement this as a plugin? Not to bypass QA, but because the functionality will only be of interest to a subset of those libraries that do have patrons. It would also decouple the development from the release cycles of Koha itself. Thoughts?
Comment 18 Lari Taskula 2021-01-26 07:47:16 UTC
(In reply to Magnus Enger from comment #17)
> I wonder if it would make sense to implement this as a plugin? Not to bypass
> QA, but because the functionality will only be of interest to a subset of
> those libraries that do have patrons. It would also decouple the development
> from the release cycles of Koha itself. Thoughts?
It does make sense to have it as a plugin, and some other systems have taken such approach. But personally I'd prefer making it a part of core Koha. This way we are by default advancing good privacy conventions in Koha.

(In reply to Jon Knight from comment #16)
> Is it even wanted?
It is, and I wanted to ask if you are still going to work on this? If not, I can try to rescue this Bug.
Comment 19 Lari Taskula 2021-01-26 11:35:05 UTC
(In reply to Lari Taskula from comment #18)
> I can try to rescue this Bug.
Jon kindly agreed to this. Setting myself as assignee.
Comment 20 Lari Taskula 2021-01-29 18:04:00 UTC
(In reply to Josef Moravec from comment #11)
> Hm, i am just thinking about using $RESULTSET_PATRON_ID_MAPPING from
> Koha::Patron and define the method Koha::Patron->takeout - does it make
> sense?
This looks interesting. Can we consider it a reliable source of information for all patron data? It should be easy to return all that data from the method itself, but how would we convert the schema names themselves to match desired REST API terminology? Perhaps with the help of Koha::Schema::Result::*->koha_object_class() (can we assume the class names of Koha objects follow the expected terminology)?
Comment 21 Lari Taskula 2021-02-03 20:20:33 UTC
What about using DBIx to determine the chain of relations to the borrowers-table? This way we could keep it dynamic, only relying on the foreign keys in our database schema.

The issue then is to format this data in a nice way, suitable for REST API output. But I believe this is an issue with any approach we take. Of course some Koha::Schema::Result::* already correspond to a Koha::Object that may have a database.column <=> API-terminology mapping, but I would imagine a lot of them do not.

Should we first implement something that simply returns all the data related to a patron, and if someone wants to focus on formatting it nicely, then solve that issue in another Bug?
Comment 22 Lari Taskula 2021-02-03 20:46:42 UTC
Another issue is how far do we want to expand the results? As an example, a patron has checked out items, do we return just the itemnumbers or should we expand the item object such that eventually we have the biblio itself (it might be interesting to know, say, the biblio.title)? I suppose expanding it all the way to biblio_metadata.metadata would be pretty hardcore?
Comment 23 Lari Taskula 2021-02-04 23:11:49 UTC Comment hidden (obsolete)
Comment 24 Lari Taskula 2021-02-04 23:14:05 UTC
(In reply to Lari Taskula from comment #21)
> What about using DBIx to determine the chain of relations to the
> borrowers-table? This way we could keep it dynamic, only relying on the
> foreign keys in our database schema.
Example of this in comment #23. I have also created an RFC for the endpoint, see https://wiki.koha-community.org/wiki/Public_patrons_export_endpoint_RFC
Comment 25 Johanna Räisä 2021-02-08 06:32:47 UTC
(In reply to Lari Taskula from comment #22)
> Another issue is how far do we want to expand the results? As an example, a
> patron has checked out items, do we return just the itemnumbers or should we
> expand the item object such that eventually we have the biblio itself (it
> might be interesting to know, say, the biblio.title)? I suppose expanding it
> all the way to biblio_metadata.metadata would be pretty hardcore?

If we return only itemnumbers then the item data have to be fetched from items-endpoint. It is not very efficient to have hundreds of API calls to get rest of the data. I think it is better to have at least an expanded endpoint for more information. Pagination options are mandatory since the return data be massive.
Comment 26 Lari Taskula 2021-02-09 15:51:05 UTC
(In reply to Johanna Räisä from comment #25)
> If we return only itemnumbers then the item data have to be fetched from
> items-endpoint. It is not very efficient to have hundreds of API calls to
> get rest of the data. I think it is better to have at least an expanded
> endpoint for more information.
Good point. While one could argue that since item data is not directly part of patron related data and should not be returned, I can see use cases where it makes sense to expand.

Expanded objects contain a lot unwanted data, so we would have to select which columns to expand.

By the way, there are 52 tables that are directly related to a patron via DBIx relationship (meaning foreign keys in borrowers table, or foreign keys in other tables pointing to borrowers). These are:

3 belongs_to relationship (borrowers refer to primary key in other table):
branches
categories
sms_providers

(the three tables above have a lot of data that should probably be left out?)

49 has_many relationship (tables that refer to borrowers):
accountlines
advanced_editor_macros
alert
api_keys
aqbasketusers
aqbudgetborrowers
aqorder_users
aqorders
article_requests
borrower_attributes
borrower_debarments
borrower_files
borrower_message_preferences
borrower_relationships
cash_register_actions
club_enrollments
club_holds_to_patron_holds
course_instructors
creator_batches
discharges
hold_fill_targets
housebound_profile
housebound_role
housebound_visit
illcomments
illrequests
issues
items_last_borrower
message_queue
messages
old_issues
old_reserves
opac_news
patron_consent
patron_list_patrons
patron_lists
patronimage
problem_reports
ratings
reserves
return_claims
reviews
subscriptionroutinglist
tags_all
tags_approval
user_permissions
virtualshelfcontents
virtualshelfshares
virtualshelves

(most of the ones above could be fully included?)

> Pagination options are mandatory since the return data be massive.
It's a costly operation to get all of that data in the first place. Especially if we expand it. If we paginate it (it's a good idea), should we cache the result for some time? Performance tests will come in handy.
Comment 27 Lari Taskula 2021-02-09 16:00:31 UTC Comment hidden (obsolete)
Comment 28 Lari Taskula 2021-02-20 01:57:16 UTC
Created attachment 117106 [details] [review]
Bug 20028: Add Koha::Patron->export

The logic behind gathering related data relies on DBIx relationships to
Borrower source.

This patch does not care about terminology - it returns related data in
a hashref containing keys of DBIx source class names, such as Borrower.

To test:
1. prove t/db_dependent/Koha/Patron.t
2. Observe success

Sponsored-by: The National Library of Finland
Comment 29 Lari Taskula 2021-02-20 01:57:21 UTC
Created attachment 117107 [details] [review]
Bug 20028: Add a system preference for patron data export

Add a system preference to determine whether patron data export is permitted.
Comment 30 Lari Taskula 2021-02-20 01:57:25 UTC
Created attachment 117108 [details] [review]
Bug 20028: Add OpenAPI spec

Includes a test that validates the existence of all related properties.

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland
Comment 31 Lari Taskula 2021-02-20 01:57:30 UTC
Created attachment 117109 [details] [review]
Bug 20028: Add Patron Export REST controller

This patch adds a REST controller for patron data exporting.

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland
Comment 32 Lari Taskula 2021-02-20 02:05:25 UTC
Added four patches containing a very basic functionality for exporting patron related data. It iterates DBIx Borrower relationships and returns a huge hashref in the following format:

>{
>  "Accountline": [
>    { "accountline_id": 1, "borrowernumber": 1481, ... },
>    { "accountline_id": 2, "borrowernumber": 1481, ... }
>  ],
>  "Borrower": { "borrowernumber": 1481, "firstname": "Export", ... },
>  ...,
>  "Virtualshelve": [
>    { "shelfnumber": 1, "owner": 1481, ... },
>    { "shelfnumber": 2, "owner": 1481, ... }
>  ]
>}

Returns the same thing from REST API, from these two endpoints:

GET /api/v1/patrons/123/export        (privileged)
GET /api/v1/public/patrons/123/export (unprivileged)

So it does not yet follow our terminology rules nor add any pagination.

Anyway, thoughts about the DBIx relationship approach?
Comment 33 Michal Denar 2021-02-20 20:29:49 UTC
Lari,
is this patch ready to testing? Because status is still In Discussion.

Thank you.
Comment 34 Lari Taskula 2021-02-20 21:32:35 UTC
(In reply to Michal Denar from comment #33)
> Lari,
> is this patch ready to testing? Because status is still In Discussion.
> 
> Thank you.

Short answer: Sign-off testing no, testing out of curiosity yes.

Longer answer: In discussion is the correct status. There are many ways to implement this feature, and as mentioned in comment #32, I'm still curious of what others think of this approach.

The response body is something that needs to be discussed. There are open questions regarding terminology, property extension and pagination.

The current patches completely ignore any REST API terminology guidelines we use. In comment #21 I have asked if we should first implement something that just returns all the data, and later enhance it in a different Bug. This will become a terminology hell, because there are many many many related objects (around 50) that ideally should be terminologically consistent. I'm not sure if this Bug is the correct place for that.

In comment #22, I have asked about extending properties so that for example items would include a biblio.title.

We also need pagination, but paginating the currently proposed model would be weird. How would one paginate such response? Should we instead return an array of objects with the same structure, such as:

[
  {
    "type": "MESSAGE",
    "object": {
      "message_id": 123,
      ... // the message object
    }
  },
  { 
    "type": "CHECKOUT",
    "object": {
       "checkout_id": 456,
       ... // the checkout object
     }

]

Here the pagination would be clearer.

I will ask these questions on koha-devel mailing list.

Also related: https://wiki.koha-community.org/wiki/Public_patrons_export_endpoint_RFC

Anyway, the patches are working and can be tested, but I wouldn't yet sign-off them.
Comment 35 Lari Taskula 2021-04-21 19:32:42 UTC
Created attachment 119968 [details] [review]
Bug 20028: Add Koha::Patron->export

The logic behind gathering related data relies on DBIx relationships to Borrower
source.

This patch does not care about object name terminology - it returns related data
in a hashref containing keys of DBIx source class names, such as Borrower.

Return example:

    {
        "Borrower" => Koha::Patron,
        "BorrowerMessagePreference" => DBIx::Class::ResultSet,
        "ReturnClaim" => Koha::Checkouts::ReturnClaims
    }

This has the benefit of giving caller the ability to format response as they
want. For example, you can then easily use the "unblessed" or "to_api"
representation of the object.

To test:
1. prove t/db_dependent/Koha/Patron.t
2. Observe success

Sponsored-by: The National Library of Finland
Comment 36 Lari Taskula 2021-04-21 19:32:48 UTC
Created attachment 119969 [details] [review]
Bug 20028: Add a system preference for patron data export

Add a system preference to determine whether patron data export is permitted.
Comment 37 Lari Taskula 2021-04-21 19:32:54 UTC
Created attachment 119970 [details] [review]
Bug 20028: Add OpenAPI spec

Includes a test that validates the existence of all related properties.

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland
Comment 38 Lari Taskula 2021-04-21 19:33:00 UTC
Created attachment 119971 [details] [review]
Bug 20028: Add Patron Export REST controller

This patch adds a REST controller for patron data exporting.

GET /api/v1/patrons/51/export
GET /api/v1/public/patrons/51/export

Supports pagination parameters "_per_page" and "_page".

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland
Comment 39 Lari Taskula 2021-04-21 19:41:56 UTC
Created attachment 119972 [details] [review]
Bug 20028: Add Patron Export REST controller

This patch adds a REST controller for patron data exporting.

GET /api/v1/patrons/51/export
GET /api/v1/public/patrons/51/export

Supports pagination parameters "_per_page" and "_page".

Response is an array that contains "_per_page" amount of objects, each
with the following format:

{
  "data": {the object, e.g. patron}
  "type": DBIx source name, e.g. "Borrower"
}

Example response:

[
  {
    "data":{
      "address":null,
      ...,
      "patron_id":1,
      ...
    },
    "type":"Borrower"
  },
  {
    "data":{
      "borrower_message_preference_id":24,
      "borrowernumber":1,
      ...,
      "wants_digest":1
    },
    "type":"BorrowerMessagePreference"
  },
  {
    "data":{
      "auto_renew":false,
      ...,
      "checkout_id":97,
      "due_date":"2021-02-19T23:59:00+00:00",
      ...,
      "patron_id":1,
      ...
    },
    "type":"Issue"
  },
  {
    "data":{
      "auto_renew":false,
      ...,
      "checkout_id":42,
      "due_date":"2021-01-19T23:59:00+00:00",
      ...,
      "patron_id":1,
      ...
    },
    "type":"OldIssue"
  }
]

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland
Comment 40 Michal Denar 2021-04-21 19:54:34 UTC
Created attachment 119974 [details] [review]
Bug 20028: Add Koha::Patron->export

The logic behind gathering related data relies on DBIx relationships to Borrower
source.

This patch does not care about object name terminology - it returns related data
in a hashref containing keys of DBIx source class names, such as Borrower.

Return example:

    {
        "Borrower" => Koha::Patron,
        "BorrowerMessagePreference" => DBIx::Class::ResultSet,
        "ReturnClaim" => Koha::Checkouts::ReturnClaims
    }

This has the benefit of giving caller the ability to format response as they
want. For example, you can then easily use the "unblessed" or "to_api"
representation of the object.

To test:
1. prove t/db_dependent/Koha/Patron.t
2. Observe success

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 41 Michal Denar 2021-04-21 19:54:40 UTC
Created attachment 119975 [details] [review]
Bug 20028: Add a system preference for patron data export

Add a system preference to determine whether patron data export is permitted.

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 42 Michal Denar 2021-04-21 19:54:46 UTC
Created attachment 119976 [details] [review]
Bug 20028: Add OpenAPI spec

Includes a test that validates the existence of all related properties.

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 43 Michal Denar 2021-04-21 19:54:52 UTC
Created attachment 119977 [details] [review]
Bug 20028: Add Patron Export REST controller

This patch adds a REST controller for patron data exporting.

GET /api/v1/patrons/51/export
GET /api/v1/public/patrons/51/export

Supports pagination parameters "_per_page" and "_page".

Response is an array that contains "_per_page" amount of objects, each
with the following format:

{
  "data": {the object, e.g. patron}
  "type": DBIx source name, e.g. "Borrower"
}

Example response:

[
  {
    "data":{
      "address":null,
      ...,
      "patron_id":1,
      ...
    },
    "type":"Borrower"
  },
  {
    "data":{
      "borrower_message_preference_id":24,
      "borrowernumber":1,
      ...,
      "wants_digest":1
    },
    "type":"BorrowerMessagePreference"
  },
  {
    "data":{
      "auto_renew":false,
      ...,
      "checkout_id":97,
      "due_date":"2021-02-19T23:59:00+00:00",
      ...,
      "patron_id":1,
      ...
    },
    "type":"Issue"
  },
  {
    "data":{
      "auto_renew":false,
      ...,
      "checkout_id":42,
      "due_date":"2021-01-19T23:59:00+00:00",
      ...,
      "patron_id":1,
      ...
    },
    "type":"OldIssue"
  }
]

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 44 Lari Taskula 2021-05-11 14:33:31 UTC
Created attachment 120844 [details] [review]
Bug 20028: Add Koha::Patron->export

The logic behind gathering related data relies on DBIx relationships to Borrower
source.

This patch does not care about object name terminology - it returns related data
in a hashref containing keys of DBIx source class names, such as Borrower.

Return example:

    {
        "Borrower" => Koha::Patron,
        "BorrowerMessagePreference" => DBIx::Class::ResultSet,
        "ReturnClaim" => Koha::Checkouts::ReturnClaims
    }

This has the benefit of giving caller the ability to format response as they
want. For example, you can then easily use the "unblessed" or "to_api"
representation of the object.

To test:
1. prove t/db_dependent/Koha/Patron.t
2. Observe success

Sponsored-by: The National Library of Finland
Comment 45 Lari Taskula 2021-05-11 14:33:37 UTC
Created attachment 120845 [details] [review]
Bug 20028: Add a system preference for patron data export

Add a system preference to determine whether patron data export is permitted.
Comment 46 Lari Taskula 2021-05-11 14:33:42 UTC
Created attachment 120846 [details] [review]
Bug 20028: Add OpenAPI spec

Includes a test that validates the existence of all related properties.

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland
Comment 47 Lari Taskula 2021-05-11 14:33:48 UTC
Created attachment 120847 [details] [review]
Bug 20028: Add Patron Export REST controller

This patch adds a REST controller for patron data exporting.

GET /api/v1/patrons/51/export
GET /api/v1/public/patrons/51/export

Supports pagination parameters "_per_page" and "_page".

Response is an array that contains "_per_page" amount of objects, each
with the following format:

{
  "data": {the object, e.g. patron}
  "type": DBIx source name, e.g. "Borrower"
}

Example response:

[
  {
    "data":{
      "address":null,
      ...,
      "patron_id":1,
      ...
    },
    "type":"Borrower"
  },
  {
    "data":{
      "borrower_message_preference_id":24,
      "borrowernumber":1,
      ...,
      "wants_digest":1
    },
    "type":"BorrowerMessagePreference"
  },
  {
    "data":{
      "auto_renew":false,
      ...,
      "checkout_id":97,
      "due_date":"2021-02-19T23:59:00+00:00",
      ...,
      "patron_id":1,
      ...
    },
    "type":"Issue"
  },
  {
    "data":{
      "auto_renew":false,
      ...,
      "checkout_id":42,
      "due_date":"2021-01-19T23:59:00+00:00",
      ...,
      "patron_id":1,
      ...
    },
    "type":"OldIssue"
  }
]

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland
Comment 48 Lari Taskula 2021-05-11 14:33:58 UTC
Rebased
Comment 49 Marcel de Rooy 2021-08-27 07:59:22 UTC
QA: Looking here
Comment 50 Marcel de Rooy 2021-08-27 09:03:29 UTC
(In reply to Lari Taskula from comment #48)
> Rebased

You removed the signoff lines of Michal?
Comment 51 Marcel de Rooy 2021-08-27 09:04:23 UTC
Created attachment 124156 [details] [review]
Bug 20028: Add Koha::Patron->export

The logic behind gathering related data relies on DBIx relationships to Borrower
source.

This patch does not care about object name terminology - it returns related data
in a hashref containing keys of DBIx source class names, such as Borrower.

Return example:

    {
        "Borrower" => Koha::Patron,
        "BorrowerMessagePreference" => DBIx::Class::ResultSet,
        "ReturnClaim" => Koha::Checkouts::ReturnClaims
    }

This has the benefit of giving caller the ability to format response as they
want. For example, you can then easily use the "unblessed" or "to_api"
representation of the object.

To test:
1. prove t/db_dependent/Koha/Patron.t
2. Observe success

Sponsored-by: The National Library of Finland

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Marcel de Rooy 2021-08-27 09:04:31 UTC
Created attachment 124157 [details] [review]
Bug 20028: Add a system preference for patron data export

Add a system preference to determine whether patron data export is permitted.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2021-08-27 09:04:37 UTC
Created attachment 124158 [details] [review]
Bug 20028: Add OpenAPI spec

Includes a test that validates the existence of all related properties.

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Marcel de Rooy 2021-08-27 09:04:42 UTC
Created attachment 124159 [details] [review]
Bug 20028: Add Patron Export REST controller

This patch adds a REST controller for patron data exporting.

GET /api/v1/patrons/51/export
GET /api/v1/public/patrons/51/export

Supports pagination parameters "_per_page" and "_page".

Response is an array that contains "_per_page" amount of objects, each
with the following format:

{
  "data": {the object, e.g. patron}
  "type": DBIx source name, e.g. "Borrower"
}

Example response:

[
  {
    "data":{
      "address":null,
      ...,
      "patron_id":1,
      ...
    },
    "type":"Borrower"
  },
  {
    "data":{
      "borrower_message_preference_id":24,
      "borrowernumber":1,
      ...,
      "wants_digest":1
    },
    "type":"BorrowerMessagePreference"
  },
  {
    "data":{
      "auto_renew":false,
      ...,
      "checkout_id":97,
      "due_date":"2021-02-19T23:59:00+00:00",
      ...,
      "patron_id":1,
      ...
    },
    "type":"Issue"
  },
  {
    "data":{
      "auto_renew":false,
      ...,
      "checkout_id":42,
      "due_date":"2021-01-19T23:59:00+00:00",
      ...,
      "patron_id":1,
      ...
    },
    "type":"OldIssue"
  }
]

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Marcel de Rooy 2021-08-27 09:04:47 UTC
Created attachment 124160 [details] [review]
Bug 20028: (QA follow-up) Fix test plan for Patron.t

t/db_dependent/Koha/Patron.t .. 8/8 # Looks like you planned 8 tests but ran 9.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 56 Marcel de Rooy 2021-08-27 09:04:52 UTC
Created attachment 124161 [details] [review]
Bug 20028: (QA follow-up) Add atomic update for new pref

Setting the initial value to 0 just to be on the safe side.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 57 Marcel de Rooy 2021-08-27 09:06:24 UTC
Lari raised various questions on this bug, but did not receive (many) answers. I do like what he wrote and I think it would be a good starting point to refine. Since it is controlled by a pref, I would not see much harm in pushing this first step and refining later.

> Should we first implement something that simply returns all the data related to a patron, and if someone wants to focus on formatting it nicely, then solve that issue in another Bug?
I would agree with that direction.

> Another issue is how far do we want to expand the results?
Hmm. This will open up a lot of trouble? Since there are so many relations, we would imo need some mechanism to do so instead of ad-hoc stuff.

> The current patches completely ignore any REST API terminology guidelines we use. [...] This will become a terminology hell.
Sure. Your approach is understandable. And you are using to_api now.

> We also need pagination, but paginating the currently proposed model would be weird. How would one paginate such response? Should we instead return an array of objects with the same structure ...
You added basic pagination with "_per_page" and "_page". Hard to say what would be the best pagination approach here ;)

> I will ask these questions on koha-devel mailing list.
I may have missed that one. I have searched in the archive, but could not find anything.

Passing QA to stir up the discussion ;)
Comment 58 Lari Taskula 2021-08-27 09:44:35 UTC
(In reply to Marcel de Rooy from comment #50)
> You removed the signoff lines of Michal?

Good spot, I'm pretty sure this happened by accident. I probably didn't pull Michal's signed-off patches before rebasing. Shall I add it back?

Also I agree with setting syspref default to 0.
Comment 59 Marcel de Rooy 2021-08-27 09:57:04 UTC
(In reply to Lari Taskula from comment #58)
> Good spot, I'm pretty sure this happened by accident. I probably didn't pull
> Michal's signed-off patches before rebasing. Shall I add it back?

If you do not remove mine :)
Comment 60 Lari Taskula 2021-08-27 10:00:02 UTC
Created attachment 124164 [details] [review]
Bug 20028: Add Koha::Patron->export

The logic behind gathering related data relies on DBIx relationships to Borrower
source.

This patch does not care about object name terminology - it returns related data
in a hashref containing keys of DBIx source class names, such as Borrower.

Return example:

    {
        "Borrower" => Koha::Patron,
        "BorrowerMessagePreference" => DBIx::Class::ResultSet,
        "ReturnClaim" => Koha::Checkouts::ReturnClaims
    }

This has the benefit of giving caller the ability to format response as they
want. For example, you can then easily use the "unblessed" or "to_api"
representation of the object.

To test:
1. prove t/db_dependent/Koha/Patron.t
2. Observe success

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 61 Lari Taskula 2021-08-27 10:00:22 UTC
Created attachment 124165 [details] [review]
Bug 20028: Add a system preference for patron data export

Add a system preference to determine whether patron data export is permitted.

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Lari Taskula 2021-08-27 10:00:47 UTC
Created attachment 124166 [details] [review]
Bug 20028: Add Patron Export REST controller

This patch adds a REST controller for patron data exporting.

GET /api/v1/patrons/51/export
GET /api/v1/public/patrons/51/export

Supports pagination parameters "_per_page" and "_page".

Response is an array that contains "_per_page" amount of objects, each
with the following format:

{
  "data": {the object, e.g. patron}
  "type": DBIx source name, e.g. "Borrower"
}

Example response:

[
  {
    "data":{
      "address":null,
      ...,
      "patron_id":1,
      ...
    },
    "type":"Borrower"
  },
  {
    "data":{
      "borrower_message_preference_id":24,
      "borrowernumber":1,
      ...,
      "wants_digest":1
    },
    "type":"BorrowerMessagePreference"
  },
  {
    "data":{
      "auto_renew":false,
      ...,
      "checkout_id":97,
      "due_date":"2021-02-19T23:59:00+00:00",
      ...,
      "patron_id":1,
      ...
    },
    "type":"Issue"
  },
  {
    "data":{
      "auto_renew":false,
      ...,
      "checkout_id":42,
      "due_date":"2021-01-19T23:59:00+00:00",
      ...,
      "patron_id":1,
      ...
    },
    "type":"OldIssue"
  }
]

To test:
1. prove t/db_dependent/api/v1/patrons_export.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 63 Lari Taskula 2021-08-27 10:00:53 UTC
Created attachment 124167 [details] [review]
Bug 20028: (QA follow-up) Fix test plan for Patron.t

t/db_dependent/Koha/Patron.t .. 8/8 # Looks like you planned 8 tests but ran 9.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 64 Lari Taskula 2021-08-27 10:01:01 UTC
Created attachment 124168 [details] [review]
Bug 20028: (QA follow-up) Add atomic update for new pref

Setting the initial value to 0 just to be on the safe side.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 65 Jonathan Druart 2021-11-08 11:09:42 UTC
I am very sorry but I am not confident in pushing this for 21.11. Also sorry to have been so slow to provide you feedback.

I think it should get more eyes from the QA team. Postponed and letting the next RM decide.
Comment 66 Fridolin Somers 2022-03-25 07:26:01 UTC
I'm looking at it.

It is quite a complex code scrapping DBIx datas.

I my opinion it should not send "type":"Borrower".
The database names are not the current terminology for objects.
Better use module name Koha::Patron, so it is easy to guess one must call API on route '/api/v1/patron'

I will try to gather feedback at hackfest next week.
Comment 67 Martin Renvoize 2022-05-04 15:56:55 UTC
As that's now two RM's who have asked for more work on this I think it's safe to call it FQA and look at it again with fresh light.