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

WIP Proposal for People Import Helper #335

Closed
wants to merge 2 commits into from

Conversation

joshco
Copy link
Contributor

@joshco joshco commented Apr 12, 2019

Based on the need of apps like Spoke or CRMs to have people imported in batches vs single API call per person, this is a proposal for a new helper osdi:people_import_helper. It's been prototyped as a Spoke contacts API with links below.

This helper allows a client to essentially batch up a set of person_signup_helper object into an array and upload all at once. By wrapping the signup objects, rather than an array of people, this allows the helper action functions like add_tags et al to be invoked individually for each person in the array.

The implementation could use the same endpoint for the osdi:person_signup_helper and osdi:people_import_helper but parse the body and behave accordingly. This allows a client to discover if it can do a batched import, or if it must go through the pain of individually uploading each person.

As for the response to the batch helper, I think Larry has the right idea. The response should be status information on errors/validation failures. We currently have osdi:error defined which could be a good way to represent this, and the data Larry's code is returning. I've added a new attribute within osdi:errors to accommodate batch requests.

I've added some stats attributes to the response object as a stake in the ground, but am interested in hearing what others think about what's necessary. Please let me know

LINKS
Larry, @lperson original PR: https://github.com/MoveOnOrg/Spoke/pull/967
Josh's OSDI-ifying branch: MoveOnOrg/Spoke@main...joshco:osdi_contacts
GHP Formatted spec for person_import_helper: https://joshco.github.io/osdi-docs/people_import_helper.html

@joshco joshco mentioned this pull request Apr 12, 2019
2 tasks
@stale
Copy link

stale bot commented Jun 11, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further comments occur. Thank you for your contributions.

@stale stale bot added the stale label Jun 11, 2019
@stale stale bot closed this Jun 18, 2019
@joshco joshco reopened this Jun 24, 2019
@stale stale bot removed the stale label Jun 24, 2019
@mlncn
Copy link

mlncn commented Jul 12, 2019

Technically seems like a great idea.

Minor question on the markdown formatting of the documentation:

Sometimes JSON is introduced with four backticks instead of three:

 ````json 

and likewise the use of four backticks around osdi:people_import_helper seems maybe more than intended; what does that do that one backtick doesn't do?

Thanks for this work!

@joshco
Copy link
Contributor Author

joshco commented Jul 15, 2019

Thanks for the feedback and the eagle eye on the back-ticks. That's my error.
At some point my brain must have transposed the 3 and the 4 in the markdown definition for code blocks.

"Blocks of code are either fenced by lines with three back-ticks ```, or are indented with four spaces. "

As far as I know, there's no other benefit to the 4th back-tick, except perhaps the visual order of even / balanced back-ticks.

If you have the time, please put together a pull request changing them to 3 back-ticks,, so you'll be recognized for your contribution.

If not, LMK, I'll handle it.

@@ -99,6 +107,13 @@ _[Back to top...](#)_

_[Back to top...](#)_

### Batch Requests
When a Batch Request response includes ````osdi:error```` status information, the response code in the parent request should reflect the status of the batch operation itself, such as the import operation.
Copy link

@mlncn mlncn Jul 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove excess tick marks

Suggested change
When a Batch Request response includes ````osdi:error```` status information, the response code in the parent request should reflect the status of the batch operation itself, such as the import operation.
When a Batch Request response includes `osdi:error` status information, the response code in the parent request should reflect the status of the batch operation itself, such as the import operation.

Copy link

@mlncn mlncn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the changes to the markdown code formatting (number of tickmarks made standard, only).

GitHub claims there's a way now for these comments to be accepted as commits!


For example, if the import operation succeeds, even if not every record was successfully imported, the response code should be 200. Conversely, if there is a format error, or fault with the parent request itself, such as a JSON parse error, the response should be 400.

The sub-request error status is contained within an array with attribute name ````batch_errors````. The sub-request array may omit status on successful sub-requests and only contain errors.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The sub-request error status is contained within an array with attribute name ````batch_errors````. The sub-request array may omit status on successful sub-requests and only contain errors.
The sub-request error status is contained within an array with attribute name `batch_errors`. The sub-request array may omit status on successful sub-requests and only contain errors.

@@ -367,3 +382,128 @@ Cache-Control: max-age=0, private, must-revalidate

_[Back to top...](#)_

## Batch Request

A batch of people is uploaded via the ````osdi:people_import_helper````. The overall upload succeeds, but there are two errors that are reported.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A batch of people is uploaded via the ````osdi:people_import_helper````. The overall upload succeeds, but there are two errors that are reported.
A batch of people is uploaded via the `osdi:people_import_helper`. The overall upload succeeds, but there are two errors that are reported.

## Batch Request

A batch of people is uploaded via the ````osdi:people_import_helper````. The overall upload succeeds, but there are two errors that are reported.
The first is an invalid tag in the ````add_tags```` helper action function, but the import of the person itself succeeds.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The first is an invalid tag in the ````add_tags```` helper action function, but the import of the person itself succeeds.
The first is an invalid tag in the `add_tags` helper action function, but the import of the person itself succeeds.


The response to a successful People import helper POST is an empty body, if there were no errors on any of the signups.

If errors occurred on individual signups, the response should contain an ````osdi:error```` object for batch requests indicating the individual status information.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If errors occurred on individual signups, the response should contain an ````osdi:error```` object for batch requests indicating the individual status information.
If errors occurred on individual signups, the response should contain an `osdi:error` object for batch requests indicating the individual status information.


{% include endpoints_and_url_structures.md %}

The link relation label for the People import helper is ```osdi:people_import_helper```.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The link relation label for the People import helper is ```osdi:people_import_helper```.
The link relation label for the People import helper is `osdi:people_import_helper`.

]
}
}
````
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
````


#### Response

````json
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
````json
```json

]
}
}
````
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
````


The top level Batch Request is known as the parent request, while the array items are called sub-requests.

In order to report errors for each sub-request, the ````batch_errors```` attribute is included, which is an array of ````osdi:error```` objects defined here.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In order to report errors for each sub-request, the ````batch_errors```` attribute is included, which is an array of ````osdi:error```` objects defined here.
In order to report errors for each sub-request, the `batch_errors` attribute is included, which is an array of `osdi:error` objects defined here.

@@ -39,6 +40,13 @@ The ````osdi:error```` object for both types of requests is identical, except th

_[Back to top...](#)_

### Batch Requests

Batch requests are a collection of similar requests sent to the server in one OSDI call. For example, the ````osdi:people_import_helper```` request includes an array of signups, where each array item is what would normally be in a single request to ````osdi:person_signup_helper````.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Batch requests are a collection of similar requests sent to the server in one OSDI call. For example, the ````osdi:people_import_helper```` request includes an array of signups, where each array item is what would normally be in a single request to ````osdi:person_signup_helper````.
Batch requests are a collection of similar requests sent to the server in one OSDI call. For example, the `osdi:people_import_helper` request includes an array of signups, where each array item is what would normally be in a single request to `osdi:person_signup_helper`.

@stale
Copy link

stale bot commented Sep 20, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further comments occur. Thank you for your contributions.

@stale
Copy link

stale bot commented Jan 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further comments occur. Thank you for your contributions.

@stale stale bot added the stale label Jan 8, 2020
@stale stale bot closed this Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants