Bug 17505 - Add routes for messaging preferences
Summary: Add routes for messaging preferences
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: REST api (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Tomás Cohen Arazi
URL:
Whiteboard:
Keywords: needs_rfc
Depends on: 17499 18137
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-26 17:02 UTC by Lari Taskula
Modified: 2018-03-30 12:13 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17505: REST API route for messaging preferences (35.87 KB, patch)
2017-07-05 09:59 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17505: REST API route for messaging preferences (35.92 KB, patch)
2017-07-30 14:30 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2016-10-26 17:02:08 UTC
REST API: Add route for messaging preferences
Comment 1 Jiri Kozlovsky 2017-03-12 20:32:09 UTC
Just to be clear about the implementation details ..

For messaging preferences to work, there must be set  EnhancedMessagingPreferences to Allow in order to let anyone change anything. This will ensure only staff can edit messaging preferences. For regular users to allow them editing those,  EnhancedMessagingPreferencesOPAC must be also enabled. If is  EnhancedMessagingPreferencesOPAC enabled, but  EnhancedMessagingPreferences is not, no editing is allowed to anyone.

API URL will be in this format: /api/v1/messaging_preferences/{borrowernumber}

Next, I suppose, there will be GET & POST methods. No else.

GET method only returns user's messaging prefs. User can view only it's own prefs, unless he has 'borrowers' rights.

POST method updates user's messaging pref. User can update only it's own prefs, unless he has 'borrowers' rights.

Both, GET & POST methods will send / receive JSON in this format:
{
  item_due: { email: 0/1, digest_only: 0/1, do_not_notify: 0/1 }
  advance_notice: { days_in_advance: 0-30, email: 0/1, digest_only: 0/1, do_not_notify: 0/1 }
  hold_filled: { email: 0/1, do_not_notify: 0/1 }
  item_checkin: { email: 0/1, do_not_notify: 0/1 }
  item_checkout: { email: 0/1, do_not_notify: 0/1 }
}

Please someone take a look at this concept, if it is okay to implement this feature in this way.
Comment 2 Lari Taskula 2017-05-12 13:59:53 UTC
Hey. Any update on this? I will start working on it if you haven't already. I attached Koha-objects to Bug 17499 which could be useful for this one.
Comment 3 Jiri Kozlovsky 2017-05-12 16:07:56 UTC
(In reply to Lari Taskula from comment #2)
> Hey. Any update on this? I will start working on it if you haven't already.
> I attached Koha-objects to Bug 17499 which could be useful for this one.

Hey there Lari. Yes, I've already done some work, but now I'm going to be away from work on Koha until I finish final school exams for bachelor's degree.

If you want, you can take a look at the work & finish it ;) All the work is in this single commit:
https://github.com/open-source-knihovna/KohaCZ/commit/7bddf4f709fce467296ddce814b4612aeaa00088

Basically I made it work for GET request. Feel free to change my coding style as you wish :D
Comment 4 Lari Taskula 2017-05-15 10:40:09 UTC
(In reply to Jiri Kozlovsky from comment #3)
> (In reply to Lari Taskula from comment #2)
> > Hey. Any update on this? I will start working on it if you haven't already.
> > I attached Koha-objects to Bug 17499 which could be useful for this one.
> 
> Hey there Lari. Yes, I've already done some work, but now I'm going to be
> away from work on Koha until I finish final school exams for bachelor's
> degree.
> 
> If you want, you can take a look at the work & finish it ;) All the work is
> in this single commit:
> https://github.com/open-source-knihovna/KohaCZ/commit/
> 7bddf4f709fce467296ddce814b4612aeaa00088
> 
> Basically I made it work for GET request. Feel free to change my coding
> style as you wish :D

Cheers! It looks awesome! I will integrate your work with mine, and attach a patch here. Good luck with your exams!
Comment 5 Michal Denar 2017-06-24 20:54:17 UTC
Hi Lari,
any progress on it? Jiri propably will ready to join us again and do some code on REST. So, we need to know actual state of Your work.

Thank You.
Comment 6 Lari Taskula 2017-07-03 14:59:47 UTC
(In reply to Mike from comment #5)
> Hi Lari,
> any progress on it? Jiri propably will ready to join us again and do some
> code on REST. So, we need to know actual state of Your work.
> 
> Thank You.

Hi! Sorry for late answer, I was on holidays. Yes, I have some code ready, attaching it by tomorrow so you can review it. Nice to hear Jiri is joining you again!
Comment 7 Lari Taskula 2017-07-05 09:59:30 UTC
Created attachment 64796 [details] [review]
Bug 17505: REST API route for messaging preferences

Get patron's / category's messaging preferences:
    GET /messaging_preferences?borrowernumber=123
        - allowed if getting own preferences
        - otherwise borrowers flag required
    GET /messaging_preferences?categorycode=K
        - borrowers flag required

Modify patron's / category's messaging preferences:
    PUT /messaging_preferences?borrowernumber=123
        - allowed if modifying own preferences
        - otherwise borrowers flag required

    PUT /messaging_preferences?categorycode=K
        - borrowers flag required

Returns the following JSON object (same for PUT except for readOnly values):
{
  "Advanced_Notice": {
    "days_in_advance": {
      "configurable": true,
      "value": 20
    },
    "digest": {
      "configurable": true,
      "value": true
    },
    "transport_types": {
      "email": true,
      "sms": false
    }
  },
  "Another_Message_Name": {
    "days_in_advance": {
      "configurable": false,
      "value": null
    },
    "digest": { ...
  },
  ...
}

To test:
1. prove t/db_dependent/api/v1/messagingpreferences.t
2. Send GET and PUT requests to /api/v1/messaging_preferences
Comment 8 Lari Taskula 2017-07-05 10:00:34 UTC
Depends on Bug 18137 and 17499. These Bugs will conflict in Koha/Exceptions.pm but should be easily fixable.
Comment 9 Jiri Kozlovsky 2017-07-15 17:49:55 UTC
(In reply to Lari Taskula from comment #8)
> Depends on Bug 18137 and 17499. These Bugs will conflict in
> Koha/Exceptions.pm but should be easily fixable.

I somehow cannot apply the patch, it shows following error:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17505: REST API route for messaging preferences
fatal: sha1 information is lacking or useless (api/v1/swagger/definitions.json).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 17505: REST API route for messaging preferences
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-17505-REST-API-route-for-messaging-preferences-QwXcYD.patch

When trying to apply it manually using 'git apply /home/vagrant/kohaclone/.git/rebase-apply/patch -3 -v', it also shows an error:

error: while searching for:
  "holds": {
    "$ref": "definitions/holds.json"
  },
  "patron": {
    "$ref": "definitions/patron.json"
  }

error: patch failed: api/v1/swagger/definitions.json:11
error: repository lacks the necessary blob to fall back on 3-way merge.
error: api/v1/swagger/definitions.json: patch does not apply

... Didn't you forgot to add another dependency? I've applied both 17499 & 18137 on top of current master.
Comment 10 Lari Taskula 2017-07-17 10:31:36 UTC
The only dependencies are Bug 18137 and Bug 17499... Are you sure all patches from Bug 18137 were applied? (Especially the "sort definitions.json"?) Because otherwise I don't see why it has an issue in definitions.json... However Bug 18137 needs a rebase anyway since it conflicts in the Holds controller after Bug 18276 and Bug 18260.
Comment 11 Jiri Kozlovsky 2017-07-30 13:59:41 UTC
(In reply to Lari Taskula from comment #10)
> The only dependencies are Bug 18137 and Bug 17499... Are you sure all
> patches from Bug 18137 were applied? (Especially the "sort
> definitions.json"?) Because otherwise I don't see why it has an issue in
> definitions.json... However Bug 18137 needs a rebase anyway since it
> conflicts in the Holds controller after Bug 18276 and Bug 18260.

Well yes, I'm sure I've applied all the patches. It shows out that patches 18137 & 17499 must be applied in correct order.

If I run "git bz apply 17505", it first wants to apply 17499 & then 18137, which results in a fail. But if I apply those patches manually in reversed order (18137 first & 17499 second), then it works.
Comment 12 Jiri Kozlovsky 2017-07-30 14:15:25 UTC
To overcome testing problems with missing OpenApi (from bug 18137) one should run:
$ sudo cpanm Mojolicious::Plugin::OpenAPI@1.10

.. but that doesn't work for me, here is my output:
--> Working on Mojolicious::Plugin::OpenAPI
Fetching http://backpan.perl.org/authors/id/J/JH/JHTHORSEN/Mojolicious-Plugin-OpenAPI-1.10.tar.gz ... OK
Configuring Mojolicious-Plugin-OpenAPI-1.10 ... OK
==> Found dependencies: JSON::Validator, Mojolicious
--> Working on JSON::Validator
Fetching http://www.cpan.org/authors/id/J/JH/JHTHORSEN/JSON-Validator-1.00.tar.gz ... OK
Configuring JSON-Validator-1.00 ... OK
==> Found dependencies: Mojolicious
--> Working on Mojolicious
Fetching http://www.cpan.org/authors/id/S/SR/SRI/Mojolicious-7.37.tar.gz ... OK
Configuring Mojolicious-7.37 ... OK
==> Found dependencies: IO::Socket::IP
--> Working on IO::Socket::IP
Fetching http://www.cpan.org/authors/id/P/PE/PEVANS/IO-Socket-IP-0.39.tar.gz ... OK
Configuring IO-Socket-IP-0.39 ... OK
Building and testing IO-Socket-IP-0.39 ... OK
Successfully installed IO-Socket-IP-0.39 (upgraded from 0.29)
Building and testing Mojolicious-7.37 ... OK
Successfully installed Mojolicious-7.37 (upgraded from 6.15)
Building and testing JSON-Validator-1.00 ... OK
Successfully installed JSON-Validator-1.00 (upgraded from 0.67)
Building and testing Mojolicious-Plugin-OpenAPI-1.10 ... FAIL
! Installing Mojolicious::Plugin::OpenAPI failed. See /root/.cpanm/work/1501423561.19833/build.log for details. Retry with --force to force install it.
3 distributions installed

What worked is this process as proposed by Tomás (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18137#c54):

$ sudo cpanm Mojolicious@7.31
$ sudo cpanm JSON::Validator@0.96
$ sudo cpanm Mojolicious::Plugin::OpenAPI@1.13
Comment 13 Jiri Kozlovsky 2017-07-30 14:30:01 UTC
Created attachment 65326 [details] [review]
Bug 17505: REST API route for messaging preferences

Get patron's / category's messaging preferences:
    GET /messaging_preferences?borrowernumber=123
        - allowed if getting own preferences
        - otherwise borrowers flag required
    GET /messaging_preferences?categorycode=K
        - borrowers flag required

Modify patron's / category's messaging preferences:
    PUT /messaging_preferences?borrowernumber=123
        - allowed if modifying own preferences
        - otherwise borrowers flag required

    PUT /messaging_preferences?categorycode=K
        - borrowers flag required

Returns the following JSON object (same for PUT except for readOnly values):
{
  "Advanced_Notice": {
    "days_in_advance": {
      "configurable": true,
      "value": 20
    },
    "digest": {
      "configurable": true,
      "value": true
    },
    "transport_types": {
      "email": true,
      "sms": false
    }
  },
  "Another_Message_Name": {
    "days_in_advance": {
      "configurable": false,
      "value": null
    },
    "digest": { ...
  },
  ...
}

To test:
1. prove t/db_dependent/api/v1/messagingpreferences.t
2. Send GET and PUT requests to /api/v1/messaging_preferences

Signed-off-by: Jiri Kozlovsky <mail@jkozlovsky.cz>
Comment 14 Josef Moravec 2017-12-19 21:37:36 UTC
I think the endpoint should be 
/patrons/<patron_id>/messaging_preferences

and somehow like
/categories/<category_id>/messaging_preferences

It seems more logical to me...

I am CCing to Tomas, to get his opinion
Comment 15 Katrin Fischer 2018-02-19 07:03:27 UTC
Tomas, could we get your opinion here, plz? :)

Lari, did you see https://wiki.koha-community.org/wiki/REST_api_RFCs?

I think it might be good to make some changes to make this endpoint match. For example:
borrowernumber > patron_id
categorycode > category_id
Comment 16 Tomás Cohen Arazi 2018-02-27 22:53:13 UTC
Hi Jiri, all looks good. We agreed new endpoints need to be proposed in the form of an RFC and voted on a dev meeting. This is mostly because (usually) the attribute names might require some tweaks. Could you please add an RFC [1], check the agreed coding guidelines for the REST api [2] and add the RFC for consideration on a dev meeting [3]? It should be (probably straight forward, but a needed step). I will help with the process if needed!

Thanks for your contributions and looking forward to communicate with you about your work on the REST api!

[1] https://wiki.koha-community.org/wiki/REST_api_RFCs
[2] https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API
[3] https://wiki.koha-community.org/wiki/Next_IRC_meetings
Comment 17 Josef Moravec 2018-02-28 13:51:09 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> Hi Jiri, all looks good. We agreed new endpoints need to be proposed in the
> form of an RFC and voted on a dev meeting. This is mostly because (usually)
> the attribute names might require some tweaks. Could you please add an RFC
> [1], check the agreed coding guidelines for the REST api [2] and add the RFC
> for consideration on a dev meeting [3]? It should be (probably straight
> forward, but a needed step). I will help with the process if needed!
> 
> Thanks for your contributions and looking forward to communicate with you
> about your work on the REST api!
> 
> [1] https://wiki.koha-community.org/wiki/REST_api_RFCs
> [2] https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API
> [3] https://wiki.koha-community.org/wiki/Next_IRC_meetings

As I know, Jiri is not working on Koha anymore,

I tried to somehow make a first draft of RF for this: 
https://wiki.koha-community.org/wiki/Patrons_messaging_preferences_endpoint_RFC

I wanted to simplify it, but I have not an idea if it will work ;) Maybe I over-simplify it ;)

If you can have a quick look on it, I would really aprecciate it.
Comment 18 Lari Taskula 2018-02-28 14:36:21 UTC
I'm open to simplifying, but some thoughts behind the current patch that I think should not be forgotten (most handled in the current patch):

For building a selection list client-wise, user should know which messaging transport types they are able to select.
* Which transport types are enabled in Koha, and which transport types are available for a particular message type? 
* Does patron have defined a valid contact information to select the transport type (it does not make sense to provide sms selection as transport type if patron has no smsalertnumber)?
* Is digest available for this transport type? What if a library wants to force digest ON for some messages?
* Is days in advance available for this message?

Combine above with patron's current settings. Or do you want another endpoint for checking what options are available and another for displaying patron's current selections?

Also consider checking the same restrictions when PUTting data.

Some of these thoughts are probably outside of scope of this Bug but my main point is that we should announce the available configuration options to API consumer, not just the current selections for this patron.
Comment 19 Tomás Cohen Arazi 2018-02-28 14:39:50 UTC
(In reply to Lari Taskula from comment #18)
> I'm open to simplifying, but some thoughts behind the current patch that I
> think should not be forgotten (most handled in the current patch):
> 
> For building a selection list client-wise, user should know which messaging
> transport types they are able to select.
> * Which transport types are enabled in Koha, and which transport types are
> available for a particular message type? 
> * Does patron have defined a valid contact information to select the
> transport type (it does not make sense to provide sms selection as transport
> type if patron has no smsalertnumber)?
> * Is digest available for this transport type? What if a library wants to
> force digest ON for some messages?
> * Is days in advance available for this message?
> 
> Combine above with patron's current settings. Or do you want another
> endpoint for checking what options are available and another for displaying
> patron's current selections?
> 
> Also consider checking the same restrictions when PUTting data.
> 
> Some of these thoughts are probably outside of scope of this Bug but my main
> point is that we should announce the available configuration options to API
> consumer, not just the current selections for this patron.

Maybe there's room for a separate endpoint, I'd say.
Comment 20 Katrin Fischer 2018-03-30 12:13:22 UTC
Left some comments on the wiki, hope they make sense!