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

Move lib/spree to OFN #5733

Merged
merged 24 commits into from Aug 19, 2020
Merged

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Jul 6, 2020

What? Why?

Closes #5734
We bring most of what is needed from spree_core/lib/spree
After this PR, we are basically missing app/models (big one) and some of the test_support stuff 🎉

What should we test?

Quite a few core features impacted with these files.

  • Verify basic order fees and shipping fees are working
  • Create product and edit details
  • Calculators - are all types of calculators available in the app? I dont think we need to test them individually. Just verify you can select all of them in a shipping method and in an enterprise fee for example.
  • Mail Interceptor - in Mail method config make sure you can intercept emails (last option in the page)
  • Mail Settings - make sure the mail settings are preserved as they were before the PR was staged. For example, if deliveries are activated and the domain emails are sent from.
  • Permalinks - make sure enterprise/shop permalinks are correctly generated and existing ones are correctly parsed (for example, access an existing shop and check the permalink is correct)
  • S3 support - verify S3 connection is working and images are rendered correctly according to settings
  • I18n - make sure you can change locale as usual
  • Product duplicator - verify product cloning is working well

Release notes

Changelog Category: Added
Brought the last batch of core lib classes (we are only missing spree models now) from spree_core so we can become independent of Spree.

@luisramos0 luisramos0 marked this pull request as draft July 6, 2020 14:44
@luisramos0 luisramos0 self-assigned this Jul 6, 2020
@luisramos0 luisramos0 changed the title Base ctrl Move lib/spree to OFN Jul 10, 2020
@luisramos0
Copy link
Contributor Author

Yes, it's not nice, it's used in the product model as well as in the variant model and the gateway model. We can remove it later

@luisramos0
Copy link
Contributor Author

can you still approve the PR after adding a commit?

@sauloperez
Copy link
Contributor

sauloperez commented Aug 3, 2020

Unless someone disagrees I'm making the call to this to Test Ready. Given the time it's been sitting there and the nature of the fixes we're doing recently, it's fairly easy that it'll end up with merge conflicts that will bring up the costs of the tech debt.

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.

Great, next time I'm debugging, I will be grateful to have all this in the same code base. And we can improve it!

@sauloperez
Copy link
Contributor

YES! the future is indeed bright @mkllnk 🚀 !

@filipefurtad0 filipefurtad0 self-assigned this Aug 18, 2020
@filipefurtad0 filipefurtad0 added pr-staged-fr staging.coopcircuits.fr pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-fr staging.coopcircuits.fr labels Aug 18, 2020
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Aug 18, 2020

Hey @luisramos0 !

Indeed a lot going on here, let's go step-wise:

  • Verified basic order fees, shipping fees, enterprise are working
  • Created products and edited details
  • Checked all types of calculators are available in the app, under enterprise/shipment/transaction fees. They are available and work correctly - did a smoke test, on several of them.
  • Mail Interceptor works correctly: when creating orders, the intercept recipient gets the email, and not the customer.
  • Mail Settings such as "Enable Mail Delivery" and "Send Mails As" are preserved before and after this PR, in their settings and functionality.
  • Permalinks work correctly for previous ones and are created in the correct way for new enterprises. If a permalink already exists, then numbers are added in the end.
  • tested I18n by changing locale to different languages, all good.
  • Product duplicator works as before. Products get cloned but not their variants - which is known from release testing notes, although I can't seem to find an open issue on this - I'm not sure this is a bug.

So far so good.

However, I'm not too sure on how to test S3 support. The configurations appear the same as before staging the PR. But some images were broken on staging-FR (since yesterday, after dev-test - remember Luís?) and I broke it a bit further, by accidentally removing the access credentials to S3. So, I would need help to fix this and the configuration issue which came up before, on staging-fr.
I staged this or in staging-UK, in which S3 was not activated, although there were some credentials saved. Activating S3 breaks the images, which may related to the need to point out an image bucket, I'm not sure. How can we best check this?

Possible issues found (not related to this PR):

  • While "Enable Mail Delivery" and "Send Mails As" configurations are retained (as mentioned above), additional email settings like "Send Copy of All Mails To" and "Intercept Email Address" are erased every time a new PR is deployed. I'm guessing this is intended/expected behavior, but maybe worth to mention

  • I could not get "Send Copy of All Mails To" option to work properly. I did not find any issue currently dealing with this, so I think it's best to open one.

I would say the PR looks good for merging, but maybe better to clarify the S3 access? Awaiting advice here. Thank you!

@filipefurtad0 filipefurtad0 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Aug 18, 2020
@luisramos0 luisramos0 added pr-staged-fr staging.coopcircuits.fr pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-fr staging.coopcircuits.fr pr-staged-uk staging.openfoodnetwork.org.uk labels Aug 19, 2020
@luisramos0
Copy link
Contributor Author

Nice Filipe, thank you!

I was trying to fix S3 in FR or UK staging but then I realized S3 is perfectly working and configured in staging AU so I just run the test myself now:

  • before PR deployment - check images in S3, upload new image to S3 and verify the S3 link in the shopfront - PASS
  • after deploy - same test - PASS

Awesome, I am merging this PR.

@luisramos0 luisramos0 merged commit 40d4ed2 into openfoodfoundation:master Aug 19, 2020
@luisramos0 luisramos0 removed feedback-needed pr-staged-au staging.openfoodnetwork.org.au labels Aug 19, 2020
@luisramos0 luisramos0 deleted the base_ctrl branch August 19, 2020 17:38
@filipefurtad0
Copy link
Contributor

Ah so they were indeed broken on both servers - thanks for checking this @luisramos0 !
And for testing this in AU as well 👍

@luisramos0
Copy link
Contributor Author

The master build is broken because this PR is incompatible with PR #5763
There's some code that needs to be adapted for this PR to work with the changes in 5763.
This will fix the build. The errors are related to mail interceptor, so maybe it will be related to #5932

@luisramos0
Copy link
Contributor Author

The master build issue is not related to #5962 at all.
It's really just an imcompatibility between 5763 and 5733. This PR here 5733 didnt take into account changes from 5763 where order_mailer.confirm_email was removed.

I just created PR #5935 to fix the mater build:
#5935

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.

[ByeBye Spree] Move lib/spree to OFN
4 participants