Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

/people POST / person_signup_helper #9

Open
j-ro opened this issue Aug 7, 2015 · 25 comments
Open

/people POST / person_signup_helper #9

j-ro opened this issue Aug 7, 2015 · 25 comments

Comments

@j-ro
Copy link

j-ro commented Aug 7, 2015

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
Copy link

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

  • that is probably tangental to this issue & is more about packaging it so it can be deployed without hard-coding etc. It may tie into the security stage though

@anuditverma
Copy link
Owner

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
Copy link
Author

j-ro commented Aug 12, 2015

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 added a commit that referenced this issue Aug 18, 2015
@anuditverma
Copy link
Owner

The person signup helper is updated and now supports POST, PUT and DELETE actions.
Here is the updated code : 5e5de8b

@j-ro
Copy link
Author

j-ro commented Aug 21, 2015

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" : "[email protected]" }]
  }
} 

@anuditverma
Copy link
Owner

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":"[email protected]","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.

@anuditverma anuditverma reopened this Aug 21, 2015
@j-ro
Copy link
Author

j-ro commented Aug 21, 2015

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
Copy link
Owner

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
Copy link
Author

j-ro commented Aug 21, 2015

yep, exactly. Cool.

@eileenmcnaughton
Copy link

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
Copy link
Owner

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
Copy link
Owner

I have moved PUT & DELETE to person's endpoint. (fix : ce5fe18) and now person-sign up helper only accepts POST requests only. (fix : c01f454)
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
Copy link
Author

j-ro commented Aug 24, 2015

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

@eileenmcnaughton
Copy link

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
Copy link
Owner

@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
Copy link
Author

j-ro commented Aug 25, 2015

yep, sounds right to me

anuditverma added a commit that referenced this issue Aug 25, 2015
@anuditverma
Copy link
Owner

406e4ba -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
Copy link
Owner

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

@j-ro
Copy link
Author

j-ro commented Aug 26, 2015

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" : "[email protected]" }]
  }
} 

@anuditverma
Copy link
Owner

As I mentioned in the #9 (comment), 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" : "[email protected]" }]
}

@anuditverma anuditverma reopened this Aug 26, 2015
@j-ro
Copy link
Author

j-ro commented Aug 26, 2015

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
Copy link

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
Copy link
Author

j-ro commented Aug 27, 2015

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" : "[email protected]" }]
  }
} 

@anuditverma
Copy link
Owner

OK, I will add the support for accepting this structure of JSON.
On 28-Aug-2015 1:38 am, "Jason Rosenbaum" [email protected] 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" : "[email protected]" }]
}
}


Reply to this email directly or view it on GitHub
#9 (comment)
.

@j-ro
Copy link
Author

j-ro commented Aug 28, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants