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

OFN without spree_backend 🎉 #4621

Merged
merged 24 commits into from
Feb 22, 2020

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Dec 20, 2019

What? Why?

What a wonderful PR! It makes you dream of future PRs "OFN without Spree" or even "OFN on rails 6". We can do it 💪

Closes #4595 and #4050.

We remove spree_backend and make final adaptations to our code to make it possible.

Dear reviewer: I think you can review just the final changes without going through the commit history, I think it's readable :-)

What should we test?

The bad news about this PR... we need to test every single corner of the backoffice...

I'd say we need to go through all the test steps of most PRs in both epics: #4595 and #4050.

I have gone through them all and I made a summary here as a task list (please follow the test steps in each of these PRs):

Release notes

Changelog Category: Changed
OFN is now independent of spree_backend 🎉

@luisramos0 luisramos0 self-assigned this Dec 20, 2019
@luisramos0 luisramos0 changed the base branch from master to spree_backend_base December 20, 2019 13:11
@luisramos0 luisramos0 changed the base branch from spree_backend_base to spree_backend_base_js December 20, 2019 13:11
@luisramos0 luisramos0 changed the base branch from spree_backend_base_js to spree_backend_base December 20, 2019 13:12
@luisramos0 luisramos0 changed the base branch from spree_backend_base to spree_backend_base_js December 20, 2019 13:13
@luisramos0 luisramos0 changed the title WIP Spree backend js WIP OFN depending on spree_backend without ruby, js and css code Dec 20, 2019
@luisramos0 luisramos0 force-pushed the spree_backend_base_js branch from 2776489 to 94e418f Compare December 22, 2019 17:00
@luisramos0 luisramos0 force-pushed the spree_backend_js branch 9 times, most recently from 14d0922 to 21b110d Compare December 26, 2019 19:12
@luisramos0 luisramos0 changed the base branch from spree_backend_base_js to master December 26, 2019 19:30
@luisramos0 luisramos0 changed the base branch from master to spree_backend_base_js December 26, 2019 19:30
@luisramos0 luisramos0 changed the base branch from spree_backend_base_js to master December 26, 2019 23:19
@luisramos0 luisramos0 changed the base branch from master to spree_backend_base_js December 26, 2019 23:19
@luisramos0 luisramos0 force-pushed the spree_backend_js branch 5 times, most recently from 82805bc to 2cd5421 Compare December 28, 2019 19:00
@luisramos0 luisramos0 changed the title WIP OFN depending on spree_backend without ruby, js and css code OFN without spree_backend 🎉 Feb 6, 2020
@luisramos0 luisramos0 removed the pr-wip label Feb 6, 2020
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Wow! 🚀

It all makes sense. No logic change. The real burden is really on testers. Now I wish we had automated visual testing.

Just as an idea. Could we override visit in all feature specs to save a screenshot. Run it on master then run it on the pr and do a diff of all the images? Maybe too much work...

@mkllnk
Copy link
Member

mkllnk commented Feb 7, 2020

This looks promising: https://github.com/gabrielrotbart/gatling

@luisramos0
Copy link
Contributor Author

Test Ready....
_

@RachL RachL self-assigned this Feb 19, 2020
@RachL RachL added the pr-staged-fr staging.coopcircuits.fr label Feb 19, 2020
@RachL
Copy link
Contributor

RachL commented Feb 19, 2020

@luisramos0 this is what I see when I try to add a new image through the product edit page. From build page, everything is ok
image

If there is already an image, clicking the new image button, just makes the button disappear 🙈

This is not happening in production.

Also on the test:

Add a state to a country and then change to another country and try to add a state to that new country.

I didn't see how to change a country's state? I don't think it is possible through the UI? Not that you can have the same state to one country:

image

Full testing notes:

https://docs.google.com/document/d/1WtsRAmGHKRbBLLDTRH4cj5My4-C0AfTM5jTp9EEagVY/edit#

After your answers I will still have to run a couple of sanity test and we should be good to go.

…orrectly

Testing the file upload would be a bit more complicated
@luisramos0
Copy link
Contributor Author

luisramos0 commented Feb 20, 2020

hello @RachL thanks a lot for testing all this!!!!

Re iamges, it was a little bug, I fixed it and added a spec to verify this "new image" page loads correctly. We are not yet testing the image upload itself though.
You can restage and retest. Should be good now.

EDITED: Re states, when I write "and then change to another country" I mean to change the page to another country on the top selector so that you can edit the states of another country.

@luisramos0
Copy link
Contributor Author

I redeployed and tested in FR staging, the images issue is now fixed 👍

@RachL
Copy link
Contributor

RachL commented Feb 21, 2020

Re state: all good then!!

Re images: all good as well. I noticed thanks to this that we allow a product to have several images, but it does not appear in the shopfront. This is like this in production as well, so all good here.

Sanity check : all good.

I believe we are ready to go!!!

@RachL RachL removed the pr-staged-fr staging.coopcircuits.fr label Feb 21, 2020
@luisramos0 luisramos0 merged commit a3a6196 into openfoodfoundation:master Feb 22, 2020
@luisramos0 luisramos0 deleted the spree_backend_js branch February 22, 2020 10:25
@luisramos0
Copy link
Contributor Author

great!

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.

Make OFN independent of spree_backend js, css, fonts and images
4 participants