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.

Sign up for GitHub

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

Jump to bottom

Rewrite API RABL views as Jbuilder #2147

Merged
merged 9 commits into from
Aug 23, 2017
Merged

Rewrite API RABL views as Jbuilder #2147

merged 9 commits into from
Aug 23, 2017

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Aug 11, 2017 •
edited
Loading

This rewrites all of our (previously RABL) API views in Jbuilder

TODO

  • Re-add caching.

@jhawthorn jhawthorn force-pushed the api_jbuilder branch 3 times, most recently from adc5f19 to 5fcfa3b Compare August 14, 2017 23:41
@jhawthorn jhawthorn mentioned this pull request Aug 15, 2017
Merged
@jhawthorn jhawthorn force-pushed the api_jbuilder branch 3 times, most recently from 786315b to 33d5314 Compare August 16, 2017 20:20
@jhawthorn jhawthorn added this to the 2.4.0 milestone Aug 16, 2017
jhawthorn and others added 5 commits August 16, 2017 17:33
@jhawthorn
Add jbuilder dependency to API
b5ac844
@jhawthorn
Rewrite API RABL views as Jbuilder
aa95018
@jhawthorn
Remove RABL from API
7be366d
@jhawthorn
Add rubocop for API views
e96a17f
@jhawthorn
Re-add caching to API templates
a6002e7 
This copies the JSON caching previously employed by RABL, without
serious consideration of its correctness or utility.
@jhawthorn
Failing spec for taxon product caching
54bb3ea 
This spec failed under the previous RABL caching.
@jhawthorn jhawthorn mentioned this pull request Aug 17, 2017
Closed
@solidusio solidusio deleted a comment from houndci-bot Aug 17, 2017
@solidusio solidusio deleted a comment from houndci-bot Aug 17, 2017
@solidusio solidusio deleted a comment from houndci-bot Aug 17, 2017
@cbrunsdon
Copy link
Contributor

This all looks good to me, are the changes here 100% compatible (as far as you can tell) or is there something we should maybe be aware of that behaves differently?

@jhawthorn
Copy link
Contributor Author

This all looks good to me, are the changes here 100% compatible (as far as you can tell) or is there something we should maybe be aware of that behaves differently?

Changes should be 100% compatible.

One thing to watch out for is we no longer have RABL's implicit checking for records being nil, so we have to check explicitly for associations being nil. I've tried to do this but it's possible I've missed some that aren't exposed by our tests.

@jhawthorn
Copy link
Contributor Author

I did a hacky test with our spec suite to ensure that the responses were compatible. For each get/put/post/delete in our test suite I recorded the response (normalized by sorting the JSON keys, starting from the same seed) and compared the output JSON.

I found one discrepancy (associations being null vs just skipping the key) which is fixed by the latest commit (81a5638).

Afaik, this is good to go :shipit:

tvdeyen
tvdeyen approved these changes Aug 23, 2017
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks John for the work and the thorough testing.

gmacdougall
gmacdougall approved these changes Aug 23, 2017
View reviewed changes
Copy link
Member

@gmacdougall gmacdougall left a comment

Choose a reason for hiding this comment

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

👍

Great work John! Thanks for the effort on this one.

kennyadsl
kennyadsl approved these changes Aug 23, 2017
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Great job! 👏

api/app/views/spree/api/addresses/_address.json.jbuilder
@@ -0,0 +1,17 @@
json.cache! address do
Copy link
Member

Choose a reason for hiding this comment

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

I know we just want to reflect views as they are, I just want to take note that this is the only place where cache! is used without I18n.locale key. Probably stuff for another PR.

mamhoff
mamhoff approved these changes Aug 23, 2017
View reviewed changes
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you John!

@jhawthorn jhawthorn merged commit 96b30d6 into solidusio:master Aug 23, 2017
@jhawthorn jhawthorn mentioned this pull request Aug 23, 2017
Closed
@cbrunsdon
Copy link
Contributor

We'll need a serious change log entry for this. Everyone that has extended or changed rabl templates will have to adapt.

@kennyadsl kennyadsl mentioned this pull request Oct 4, 2017
Closed
JuanCrg90 added a commit to JuanCrg90/solidus that referenced this pull request Nov 10, 2019
@JuanCrg90
Remove RABL remnants
45972bc 
RABL was removed from the project on version 2.4.0 (See solidusio#2147 solidusio#2146).
This updates the responder template to match with Jbuilder and updates
the README file.
@JuanCrg90 JuanCrg90 mentioned this pull request Nov 10, 2019
Merged
5 tasks
JuanCrg90 added a commit to JuanCrg90/solidus that referenced this pull request Nov 15, 2019
@JuanCrg90
Remove RABL remnants
3e07558 
RABL was removed from the project on version 2.4.0 (See solidusio#2147 solidusio#2146).
This updates the responder template to match with Jbuilder and updates
the README file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmacdougall gmacdougall gmacdougall approved these changes

@mamhoff mamhoff mamhoff approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

@tvdeyen tvdeyen tvdeyen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jhawthorn @cbrunsdon @tvdeyen @kennyadsl @mamhoff @gmacdougall