Bug 28701

Summary: primary_contact_method not part of the REST API spec
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: REST APIAssignee: Alexis Ripetti <alexis.ripetti>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: alexis.ripetti, andrewfh, cbrannon, lucas, m.de.rooy, nick, noemie.labine, tomascohen
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: 11879    
Bug Blocks:    
Attachments: Bug 28701 - Adding primary_contact_method to the REST API spec
Bug 28701 - Adding primary_contact_method to the REST API spec
Bug 28701 - Adding primary_contact_method to the REST API spec
Bug 28701: (QA follow-up) Make primary_contact_method visible

Description Jonathan Druart 2021-07-12 13:10:27 UTC
Bug 11879 added borrowers.primary_contact_method but not the REST API spec changes.
Comment 1 Noémie Labine 2022-05-16 19:43:49 UTC
+1
Comment 2 Alexis Ripetti 2022-06-15 20:34:29 UTC
Created attachment 136124 [details] [review]
Bug 28701 - Adding primary_contact_method to the REST API spec
Comment 3 Alexis Ripetti 2022-06-15 20:50:06 UTC
I tried to test this patch but my master cannot access the API because of modules dependencies.
I test this patch in 21.05 and it works.
If anyone wants to try, you just need to be connected in the OPAC and to call a endpoint involving patron. Like `/api/v1/patrons`. You may need to check your permissions.
Comment 4 Nick Clemens 2022-11-16 21:32:46 UTC
Created attachment 143972 [details] [review]
Bug 28701 - Adding primary_contact_method to the REST API spec

This patch adds the primary_contact_method field to the api response for patrons

To test:
1 - GET http://localhost:8081/api/v1/patrons/51
2 - Look for primary_contact_method, it's not there
3 - Apply patch
4 - GET http://localhost:8081/api/v1/patrons/51
5 - Look for primary_contact_method, it's not there

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Tomás Cohen Arazi 2022-11-17 00:21:26 UTC
I think it should be an enum
Comment 6 Marcel de Rooy 2023-01-27 08:03:02 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> I think it should be an enum

`primary_contact_method` varchar(45) DEFAULT NULL COMMENT 'useful for reporting purposes',

Scope creep ?
Comment 7 Marcel de Rooy 2023-01-27 08:09:21 UTC
> This patch adds the primary_contact_method field to the api response for patrons
Please look at step 5 of your test plan. Contradiction :)

> To test:
> 4 - GET http://localhost:8081/api/v1/patrons/51
> 5 - Look for primary_contact_method, it's not there

It was not there indeed. But it should be! Adding a follow-up.
Comment 8 Marcel de Rooy 2023-01-27 08:12:00 UTC
Created attachment 145715 [details] [review]
Bug 28701 - Adding primary_contact_method to the REST API spec

This patch adds the primary_contact_method field to the api response for patrons

To test:
1 - GET http://localhost:8081/api/v1/patrons/51
2 - Look for primary_contact_method, it's not there
3 - Apply patch
4 - GET http://localhost:8081/api/v1/patrons/51
5 - Look for primary_contact_method, it's not there

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2023-01-27 08:12:04 UTC
Created attachment 145716 [details] [review]
Bug 28701: (QA follow-up) Make primary_contact_method visible

Via to_api_mapping.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Tomás Cohen Arazi 2023-01-27 10:29:15 UTC
(In reply to Marcel de Rooy from comment #6)
> (In reply to Tomás Cohen Arazi from comment #5)
> > I think it should be an enum
> 
> `primary_contact_method` varchar(45) DEFAULT NULL COMMENT 'useful for
> reporting purposes',
> 
> Scope creep ?

If you git grep primary_contact_method, you will notice that the possible values there are hardcoded:

koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                                                <select id="borrower_primary_contact_method" name
="borrower_primary_contact_method">
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                                                        [% IF ( borrower.primary_contact_method =
= 'phone' ) %]
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                                                        [% IF ( borrower.primary_contact_method =
= 'phonepro' ) %]
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                                                        [% IF ( borrower.primary_contact_method =
= 'mobile' ) %]
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                                                        [% IF ( borrower.primary_contact_method =
= 'email' ) %]
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                                                        [% IF ( borrower.primary_contact_method =
= 'emailpro' ) %]
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                                                        [% IF ( borrower.primary_contact_method =
= 'fax' ) %]

when we write endpoint specs, we want them to be future proof (as much as possible). So I'm not expecting this bug to update the DB structure, but either:

(a) make the type of the 'new API object attribute' be an enum with those values
(b) choose better names for those hardcoded values, and list them as an enum IN THE SPEC and make to_api() handle the name mapping.

I'd prefer (b).

Regarding the follow-up, it does the job, but we've better only map things that need mapping (i.e. mapping to null removes it from the response, mapping to a value makes the API translate the name, not mapping makes the API return it as-is.
Comment 11 Lucas Gass 2023-03-29 20:17:45 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> (In reply to Marcel de Rooy from comment #6)
> > (In reply to Tomás Cohen Arazi from comment #5)
> > > I think it should be an enum
> > 
> > `primary_contact_method` varchar(45) DEFAULT NULL COMMENT 'useful for
> > reporting purposes',
> > 
> > Scope creep ?
> 
> If you git grep primary_contact_method, you will notice that the possible
> values there are hardcoded:
> 
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> <select id="borrower_primary_contact_method" name
> ="borrower_primary_contact_method">
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> [% IF ( borrower.primary_contact_method =
> = 'phone' ) %]
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> [% IF ( borrower.primary_contact_method =
> = 'phonepro' ) %]
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> [% IF ( borrower.primary_contact_method =
> = 'mobile' ) %]
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> [% IF ( borrower.primary_contact_method =
> = 'email' ) %]
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> [% IF ( borrower.primary_contact_method =
> = 'emailpro' ) %]
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> [% IF ( borrower.primary_contact_method =
> = 'fax' ) %]
> 
> when we write endpoint specs, we want them to be future proof (as much as
> possible). So I'm not expecting this bug to update the DB structure, but
> either:
> 
> (a) make the type of the 'new API object attribute' be an enum with those
> values
> (b) choose better names for those hardcoded values, and list them as an enum
> IN THE SPEC and make to_api() handle the name mapping.
> 
> I'd prefer (b).
> 
> Regarding the follow-up, it does the job, but we've better only map things
> that need mapping (i.e. mapping to null removes it from the response,
> mapping to a value makes the API translate the name, not mapping makes the
> API return it as-is.


I'd like to point out that many librarians don't like the fact that these are hard coded values. I've written an enhancement to make them customizable via authorized values in Bug 31498.
Comment 12 Christopher Brannon 2023-10-12 23:52:53 UTC
Looking forward to the API being updated to include the contact method.  :)