anuditverma / org.civicrm.osdi

OSDI API Implementation for CiviCRM - Google Summer of Code (GSoC) 2015
Other
1 stars 3 forks source link

/people POST / person_signup_helper #9

Open j-ro opened 9 years ago

j-ro commented 9 years ago

The person signup helper should not be a form. It should be an API endpoint that takes a POST request. So, if I use the browser here:

http://camus.fuzion.co.nz/hal-browser/browser.html#/sites/default/ext/osdi/api/v3/People/index.php

And I click the orange non-get button next to self at the top of the left column, that'll set up a POST request to the people endpoint. There you'll receive JSON in the person_signup_helper format (or could be another endpoint if you want), and process that into a person resource.

eileenmcnaughton commented 9 years ago

Annudit - I don't think you have a form there - I believe it's a raw php file which checks the $_POST variable directly - it would probably be better to convert it at some stage to being a class that is registered in the civicrm menu system (as CRM_Utils_Rest is - which would be done by using a hook to register the class

http://wiki.civicrm.org/confluence/display/CRMDOC/hook_civicrm_xmlMenu

anuditverma commented 9 years ago

I have pushed the person-sign up helper file here https://github.com/anuditverma/OSDI-Implementation/blob/master/api/v3/signup.php Right now I am adding records in the civi system, there are many ways to add a record to civi db. Here I am using REST URL but I am facing some issues. Also I want to know, when we go to HAL-browser and make a NON-GET request then on the pop form there is a field for Target URI which I believe by default is same as the self links of endpoints, so for any system like here in Civi I am receiving the POST request from that form and passing it on to the apt method to add the resources so like wise if for any system there may be a different way of adding such resources. So what exactly is the significance of Target URI does it defines a way of adding resources like within a directory followed by the URI present in it ? Also is there a standard way of performing POST request through NON-GET that will be compatible with any system ? Right now if I change the value of $ch in my code to a Target URI and provide auth keys with it so would it be a way of adding resources to any specific target end point ?

j-ro commented 9 years ago

I think it's probably best if you do the proxying, yeah. So you set up an endpoint for receiving POST/person_signup (can be anything you want, really, doesn't have to be the self link), and then you proxy the request to the Civi endpoint that can handle it.

anuditverma commented 9 years ago

The person signup helper is updated and now supports POST, PUT and DELETE actions. Here is the updated code : https://github.com/anuditverma/org.civicrm.osdi/commit/5e5de8b2eaa86c471ea8a8a14b3622537646c649

j-ro commented 9 years ago

I'm unclear how to get this to work on the browser. I POST this code to /sites/default/ext/osdi/api/v3/signup.php but nothing seems to happen:

{
  "person" : {
    "family_name" : "Smith",
    "given_name" : "John",
    "postal_addresses" : [ { "postal_code" : "20009" }],
    "email_addresses" : [ { "address" : "jsmith@mail.com" }]
  }
} 
anuditverma commented 9 years ago

Right now it seems this would be accepted if we provide it in this way:

{ "action":"post","contact_type":"Individual","given_name":"John","additional_name":"","family_name":"Smith","gender":"Male","location_type_id":"Home","postal_addresses":"Testfieldspacing","email":"jsmith@mail.com","phone":12345 }

where, action- field specifies what action has to be taken since I have added support for all other actions as well which would be fixed.

contact_type & location_type_id -are mandatory in Civi if we want to provide names and address respectively but right now it would be also be mandatory to provide these values this is because I am calling the APIs in chain like address entities and other like phone entities. SO yes this would be also be fixed if someone only wants to provide the contact's first and last name.

j-ro commented 9 years ago

So, you can't change the POST format -- that's set in the OSDI spec. @eileenmcnaughton or others can chime in, but probably those things can either be set by the server when passed on to Civi (contact_type will always be individual I think), or mapped to existing OSDI fields. Last resort would be namespaced, but I don't think we need it.

Now, you can choose what fields are required, and return an error if not enough are there. So that would be up to you all to decide what's required for a Civi POST. But I'd suggest hiding these extra fields if we can, and having the server pass them automatically.

anuditverma commented 9 years ago

Agreed, since we want to stick to the POST format according to OSDI spec and it does make sense if the mandatory fields should be set to a value which would be same all the time and must be hidden. So that ultimately we would end up passing only fields which are defined in the OSDI spec.

j-ro commented 9 years ago

yep, exactly. Cool.

eileenmcnaughton commented 9 years ago

Yes, makes sense re setting values like contact_type.

Why are you passing post as an action rather than just interrogating the $_POST var?

anuditverma commented 9 years ago

I am passing post as an action right now because person sign up's endpoint is having all other actions as well like PUT & DELETE which will be moved to person's end point. So after that it will always and automatically invoke POST requests.

anuditverma commented 9 years ago

I have moved PUT & DELETE to person's endpoint. (fix : https://github.com/anuditverma/org.civicrm.osdi/commit/ce5fe185faccc0646977f841f2da241129544d79) and now person-sign up helper only accepts POST requests only. (fix : https://github.com/anuditverma/org.civicrm.osdi/commit/c01f45427e936330c7a1da3ce21f867eecda7e13) But before finalizing this issue we need to discuss about passing the default values for some mandatory fields or throw an error if user doesn't provide them because POST-ing utilizes REST API calls in chains where chained APIs' fields are mandatory. So I think mandatory fields should be set to some default values and will be passed unless user provide that field's value but the problem with this method is that unnecessary data will add up in the database for that person's record. This is something paradoxical here, we don't want to add unwanted data rather ignored fields should be shown as empties but Civi system uses some mandatory fields in chain APIs waiting for the data to be passed in them. FYI here is the LOC which uses chain APIs (observe the Address, email and phone as chained APIs) https://github.com/anuditverma/org.civicrm.osdi/blob/master/api/v3/signup.php#L28

j-ro commented 9 years ago

So, essentially you're passing contact_type as a default? And I guess an entity?

eileenmcnaughton commented 9 years ago

You definitely need to set a location type if you are passing in location data - I think you should set it conditionally - ie. add it to your request if there is location data to set.

A default is fine for location_type_id. The best default would be the one that you get by calling civicrm_api3('location_type', 'get', array('is_default' => 1);

Since there should only be one default you could probably refine that to

civicrm_api3('location_type', 'getsingle', array('is_default' => 1);

anuditverma commented 9 years ago

@j-ro Currently, I am passing contact-type as 'Individual' which will be default value set for this field. So as per the OSDI spec I think this will be apt to set this value, moreover we are only concerned with OSDI spec and also the contact-type field lies more towards the Civi side.

@eileenmcnaughton OK, I'll set the default value for location_type_id within the wrapper.

j-ro commented 9 years ago

yep, sounds right to me

anuditverma commented 9 years ago

https://github.com/anuditverma/org.civicrm.osdi/commit/406e4baf3eab079d97c7d43bb38b29464ad543c8 -So I am passing "Main" as the default value for 'location_type_id', it can be changed if user wants to set different location id ie. "Main", "Work", "Billing" or "Other".

anuditverma commented 9 years ago

Fixed, "Home" has been chosen as the default value for 'location_type_id'

j-ro commented 9 years ago

How does this work exactly? I try posting this but nothing seems to happen:

{
    "family_name" : "Smith",
    "given_name" : "John",
    "postal_addresses" : [ { "postal_code" : "20009" }],
    "email_addresses" : [ { "address" : "jsmith@mail.com" }]
  }
} 
anuditverma commented 9 years ago

As I mentioned in the https://github.com/anuditverma/org.civicrm.osdi/issues/9#issuecomment-133538984, it is accepting that JSON format. Actually it is mandatory to provide other fields' values because it is the requirement to invoke the creat.api to POST a new record. So I tried passing null values into the REST URI when POST-ing via curl but it doesn't accepts null or empty values, some random string like "fixme" or "blank" can be provided automatically when user doesn't want to provide that fields' data but this would add unnecessary data to the db. I think this can be done if I use dedicated API calls to add the fields one by one because when chaining of the APIs is used it becomes rather mandatory to provide all the fields manually or automatically.

Also when I see your JSON request below that you are providing the sub fields' data like postal_addresses having postal code, locality, region, country etc so I think I have to make this JSON format acceptable instead the one I mentioned in the comment above.

{
 "postal_addresses" : [ { "postal_code" : "20009" }],
 "email_addresses" : [ { "address" : "jsmith@mail.com" }]
}
j-ro commented 9 years ago

So, it's ok that you require certain fields -- each server can decide what's required or not. But you do have to stick with the OSDI format for posting. And yes, you'd want to not add data where there is none posted, assuming it wasn't required.

eileenmcnaughton commented 9 years ago

I think the extra default fields and required for the chained calls? So you could check whether enough info for the chained calls has been provided & potentially not apppend the chained url parameter if not enough is there

j-ro commented 9 years ago

ok, I'm still not getting a response when I try something like this:

{
    "family_name" : "Smith",
    "given_name" : "John",
    "postal_addresses" : [ { "postal_code" : "20009" }],
    "email_addresses" : [ { "address" : "jsmith@mail.com" }]
  }
} 
anuditverma commented 9 years ago

OK, I will add the support for accepting this structure of JSON. On 28-Aug-2015 1:38 am, "Jason Rosenbaum" notifications@github.com wrote:

ok, I'm still not getting a response when I try something like this:

{ "family_name" : "Smith", "given_name" : "John", "postal_addresses" : [ { "postal_code" : "20009" }], "email_addresses" : [ { "address" : "jsmith@mail.com" }] } }

— Reply to this email directly or view it on GitHub https://github.com/anuditverma/org.civicrm.osdi/issues/9#issuecomment-135538574 .

j-ro commented 9 years ago

there's an extra } in there I think, but yeah, that's the basic POST structure via OSDI spec.