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

Convert spree/api/products and spree/api/variants views from rabl to AMS #4101

Merged
merged 13 commits into from
Sep 2, 2019

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Jul 30, 2019

What? Why?

Part of #4060

Here we convert spree/api/variants and spree/api/products from rabl to AMS and move routes and controllers out of the spree namespace into /api 🎉

What should we test?

  • Check that /admin/products is displaying variants with all attributes correctly.
  • Check that a shop front is displaying variants correctly.
  • Check that you can add an item to an order from the admin interface.
  • Check that you can create a variant.
  • Check that you can delete a variant.

Release notes

Changelog Category: Changed
Converted variants and products api endpoints from rabl to AMS.

Dependency

This PR depends on #4055 (the target branch of this PR is remove_spree_api, it's the branch of #4055).

@luisramos0 luisramos0 self-assigned this Jul 30, 2019
…ler, using render json instad of respond with, deleting rabl templates and adapting specs

Delete unused pagination spec
…r search by sku by adding sku to the serializer and adapt a few specs to pass with AMS attrivbutes,
… the now removed rabl variants/products templates
@luisramos0 luisramos0 changed the title WIP Remove variants rabl WIP Convert variants rabl Jul 31, 2019
@luisramos0 luisramos0 changed the base branch from master to remove_spree_api August 1, 2019 12:19
@luisramos0 luisramos0 changed the title WIP Convert variants rabl Convert spree/api/products and spree/api/variants views from rabl to AMS Aug 2, 2019
@luisramos0 luisramos0 removed the pr-wip label Aug 2, 2019
@RachL RachL added the blocked label Aug 8, 2019
@luisramos0
Copy link
Contributor Author

ah @mkllnk thanks so much for adding the testing part!

@lin-d-hop
Copy link
Contributor

These 'Blocked' issues in 'Test Ready'....
I'm not sure what to do with them.
@luisramos0... to test or not to test?

@luisramos0
Copy link
Contributor Author

luisramos0 commented Aug 23, 2019 via email

@RachL RachL removed the blocked label Aug 28, 2019
@luisramos0 luisramos0 changed the base branch from remove_spree_api to master August 28, 2019 15:19
@luisramos0
Copy link
Contributor Author

this is now ready for testing.

@RachL RachL self-assigned this Aug 29, 2019
@RachL RachL added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-es labels Aug 30, 2019
@RachL
Copy link
Contributor

RachL commented Aug 30, 2019

@luisramos0 everything seems to work great as manager. As super admin however, creating and deleting variant gave me error 502 and 504 (time out).
Is it possible that the changes introduced here are making the performance worst on /products?

https://docs.google.com/document/d/1DKgv4F4TmLGYFYXrNPoM11W0cO6UfkMKiTr_h8kcJ6c/edit#

@luisramos0
Copy link
Contributor Author

yeah, I dont think the timeouts are specific to the actions, there are timeouts loading all the pages, this page loads all products in the database. Timeouts can make actions fail in some cases...

I deployed master to FR staging and I also see the timeouts exactly in the same page 45. So, I dont think this PR is making the problem worse.
Well, this PR could affect the performance of the page yes, could be slower or faster. I didnt compare the performance. I dont think the difference will be significant.

Anyway, this is fixed in #4081 where admin/products will only load one page of products 🎉

@RachL
Copy link
Contributor

RachL commented Sep 2, 2019

Ok let's move it to ready to go then!

@RachL RachL removed the pr-staged-fr staging.coopcircuits.fr label Sep 2, 2019
@sauloperez sauloperez merged commit 369a5a8 into openfoodfoundation:master Sep 2, 2019
@luisramos0 luisramos0 deleted the remove_variants_rabl branch September 2, 2019 10:19
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