At least in CVS, maybe in JSON or other machnine readable formats.
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.
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
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.
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.
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.
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.
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>
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.
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.
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,
(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,
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.
(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 ?
Please move code to module. Makes it easier to test too.
(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.
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?
(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?
(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.
(In reply to Lari Taskula from comment #18) > I can try to rescue this Bug. Jon kindly agreed to this. Setting myself as assignee.
(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)?
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?
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?
Created attachment 116344 [details] [review] Bug 20028: Add Koha::Patron->takeout To test: 1. prove t/db_dependent/Koha/Patron.t 2. Observe success
(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
(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.
(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.
Created attachment 116604 [details] [review] Bug 20028: Add Koha::Patron->takeout To test: 1. prove t/db_dependent/Koha/Patron.t 2. Observe success
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
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.
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
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
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?
Lari, is this patch ready to testing? Because status is still In Discussion. Thank you.
(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.
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
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.
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
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
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
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>
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>
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>
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>
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
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.
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
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
Rebased
QA: Looking here
(In reply to Lari Taskula from comment #48) > Rebased You removed the signoff lines of Michal?
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>
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>
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>
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>
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>
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>
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 ;)
(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.
(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 :)
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>
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>
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>
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>
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>
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.
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.
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.