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

REST API v1 #429

Closed
wants to merge 42 commits into from
Closed

REST API v1 #429

wants to merge 42 commits into from

Conversation

wvengen
Copy link
Member

@wvengen wvengen commented May 23, 2016

Starting off from #423, this is a first version of an API for Foodsoft. By design, it only supports user-facing resources, so API users can only see and update one's own orders. Perhaps in a later version of the API, we may add admin-functionality as well.

note this PR will be split into smaller PRs, so don't merge this one

note this feature is finished, but I'd rather wait with merging until foodsoft-shop is ready (or someone else wants to use the API asap); before merging we can still change the api, afterwards I'd be more careful

Pending:

  • Fix merge conflict with master
  • Make CORS work for multiple scopes (instead of a hardcoded /f)
  • Make sure relevant errors return json with description
  • Use current language in Doorkeeper
  • Review security
    • CORS settings
    • Make sure ransack searches don't leak data (like who ordered what)
  • Document endpoints (at least a bit)
  • Include tests (see also Add REST API to retrieve financial transactions #423 (comment))
  • Use model name as root key (instead of data)
    • and include order_article to group_order_article update endpoint (and maybe others)
  • Allow plugins to register themselves for OAuth2 credentials (use Doorkeeper::Application)
  • Make Doorkeeper work with multicoops (!)

For later: return all orders, not just open ones (and allow filtering by status)

@wvengen
Copy link
Member Author

wvengen commented May 25, 2016

Decided to not return json for server error (status 500) e.a., so that clients are aware that perhaps at some point a json body isn't present - which also might be the case if something breaks really terribly.

@wvengen wvengen force-pushed the feature/rest-for-shop branch from c432fd1 to 48c4b43 Compare May 25, 2016 22:17
@wvengen
Copy link
Member Author

wvengen commented May 28, 2016

The url used with petstore.swagger.io in doc/API.md is already the one for the master branch here. To test it now, use this link instead.

@wvengen wvengen force-pushed the feature/rest-for-shop branch 5 times, most recently from d5e734f to 889f574 Compare May 28, 2016 14:43
def index
@user = self.current_user
@user = current_user
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it broke since moving current_user to a concern, this makes it work again.

@wvengen
Copy link
Member Author

wvengen commented Oct 9, 2017

One reason I'm not merging this yet, is because a question came up in foodsoft-shop. Now we use data as the main json key (with meta for pagination and such). This is all fine, except when updating a group_order_article. The update returns the new group_order_article, but the related order_article is necessarily also updated. Foodsoft-shop now does a second request to update this, but this adds latency: it would be much nicer to embed this in the first response. With the data key, there is no natural place for this.

If we'd use the model name as the json key (group_order_article(s)), the order_article could just be included under its own json key. Does it make sense to change this? (Even though it would need more code on the client-side, because there is no standard place to look for the data.)

What do you think?

@carchrae
Copy link
Contributor

i think we should prioritize this PR. even if it doesn't have a complete front end. each time i go to update a rails view or fix bootstrap 2 design stuff it makes me wince.

I've fixed many performance issues, most are due to sloppy rails joins that would be better done by front end code.

I'll try and give this some time in the next week to review. i did a big api project with spree/solidus so I have some experience with building a rails api. we actually built exactly the same design, a really fast client in angular and rails just for data and server logic.

@carchrae
Copy link
Contributor

in order to ship, i propose you make a milestone for api and split each item in your checklist into an issue in that milestone. easier to submit and comment on that way too

@wvengen
Copy link
Member Author

wvengen commented Oct 13, 2018

I'd like to consider merging this soonish!
@paroga would you like to have a look, or shall I self-review a bit and merge?

@paroga
Copy link
Member

paroga commented Oct 13, 2018

It would be nice to make small logical PR out of it first, but keep this here for the overview where it's heading (like I do with the bank transaction stuff)

@paroga
Copy link
Member

paroga commented Oct 13, 2018

do we want to add the foodsoft-shop in this repo too or how do we make sure they stay compatible?

@wvengen
Copy link
Member Author

wvengen commented Oct 13, 2018

Ok, I'll split it in the following PRs:

Even though I've packed foodsoft-shop in a Foodsoft plugin (that I'd like to ship within Foodsoft), they should remain working even across versions, as long as the API version is supported. (Even though this might break subtly if the API consumers expect newly introduced attributes to be always present - but at least older API consumers will work with newer Foodsoft versios).

@carchrae
Copy link
Contributor

i want to say think you @wvengen and @paroga - i would love to switch to an api version. i have many improvements to contribute back but they are mixed up with customisation for my coop, keeping the custom stuff in the front-end, where possible, would help a lot. hopefully i get a bit of time to do that in the next month.

@wvengen
Copy link
Member Author

wvengen commented Oct 15, 2018

Closing if favour of the 5 other API PRs.

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

Successfully merging this pull request may close these issues.

4 participants