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

Add an Api Resource Controller #319

Merged
merged 1 commit into from
Sep 11, 2015
Merged

Add an Api Resource Controller #319

merged 1 commit into from
Sep 11, 2015

Conversation

athal7
Copy link

@athal7 athal7 commented Aug 24, 2015

To abstract away a lot of the similarities among the api controllers.
This should make it easier to make changes to how we do templating,
authorization, or param whitelisting in the future.

Even though the api/users_controller no longer has logic, I left the specs
in place for this PR to demonstrate API compatibility. Happy to remove them if we
see them as being superfluous.

I also chose to not reuse too many pieces of the admin/resource_controller because
there is a lot of hard-to-follow code in there, so hopefully this new one can be a
good clean space to refactor and then share code back to the admin/resource_controller.

More controllers to inherit from this later barring successful acceptance into the codebase.

@BenMorganIO
Copy link
Contributor

One thing I'd love to start work on with Solidus is a second version of the API. I think we should move all API requests to a v1 namespace and maybe start building out a second API and deprecate this existing one and move it to its own gem.

I have experimented with a Spree::Api::V2::ResourcesController and it did feel like a pretty good controller to have around. Debugging can be kinda weird though.

I would personally try and put effort into building a V2, rather than refactoring the V1, but I'm 👍 for this PR.

@BenMorganIO
Copy link
Contributor

@athal7 I totally agree with you on the ResourcesController being hard to read. Having built one on my own privately, it can be super awkward. Steps to ensure the meta-programming is easy to read and/or follow should be a high priority; especially for debugging.

@athal7
Copy link
Author

athal7 commented Aug 25, 2015

@BenMorganIO I'm all for a V2 if we have/desire breaking API changes. As long as the API contract is consistent here, I think we can do this refactoring in V1. And hopefully it will make the V2 build easier as well.

I'm curious to chat through (on a separate thread maybe here) what you are looking to see from a V2, since I think a lot of people have opinions.

@athal7 athal7 self-assigned this Aug 28, 2015
@athal7
Copy link
Author

athal7 commented Aug 28, 2015

@jhawthorn updated to authorize on new. I still think it makes sense to return an empty set and 404 respectively for index / show as opposed to 401ing, but happy to chat it through more.

@athal7 athal7 removed their assignment Aug 28, 2015
@hhff
Copy link

hhff commented Aug 29, 2015

👍

@athal7
Copy link
Author

athal7 commented Sep 3, 2015

ping @solidusio/owners curious to get some more eyes on this

@hhff
Copy link

hhff commented Sep 3, 2015

I would recommend using respond_with in the new action as well - for the sake of consistency. If a developer needs to intercept / extend respond_with, etc

To abstract away a lot of the similarities among the api controllers.
This should make it easier to make changes to how we do templating,
authorization, or param whitelisting in the future.
@athal7
Copy link
Author

athal7 commented Sep 3, 2015

@hhff updated

@hhff
Copy link

hhff commented Sep 3, 2015

👍

@magnusvk
Copy link
Contributor

magnusvk commented Sep 4, 2015

I think this looks good. 👍 from me; @jhawthorn should we merge this? Or loop another entity controller into it first?

@jhawthorn
Copy link
Contributor

Yeah, I really like this 👍

@magnusvk
Copy link
Contributor

Great; hitting merge here as Andrew is out.

magnusvk added a commit that referenced this pull request Sep 11, 2015
@magnusvk magnusvk merged commit 732b39b into solidusio:master Sep 11, 2015
jordan-brough added a commit to jordan-brough/solidus that referenced this pull request Sep 17, 2015
These were removed in solidusio#319
and replaced by generically-named equivalents, but controllers in apps
like ours can be reasonably expecting these to be there.
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

Successfully merging this pull request may close these issues.

5 participants