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 migration to fix calculator preferences #5799

Merged
merged 2 commits into from Jul 24, 2020

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Jul 21, 2020

What? Why?

Closes #5795

This will change all adjustments config in spree_preferences to the new calculators without the Spree namespace.

What should we test?

Before staging this PR you should replicate the problem, for example, in FR staging now in fredo's farm hub shop the 10eur shipping fee are not applied:
https://staging.openfoodfrance.org/fredo-s-farm-hub/shop
I am pointing this one out because not all fees are broken (all the fees that have been edited manually since v3 release will be working correctly).
You need to test this with existing fees on the server, you cant edit any fees otherwise the problem is gone.
After this PR is staged the fees should be applied again.

Release notes

Changelog Category: Fixed
Fixed a data migration bug in a recent change to fees calculators where fees were not applied at checkout.

@luisramos0 luisramos0 self-assigned this Jul 21, 2020
@filipefurtad0 filipefurtad0 self-assigned this Jul 23, 2020
@filipefurtad0 filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Jul 23, 2020
@filipefurtad0
Copy link
Contributor

Hi @luisramos0 ,

I could not reproduce the issue on the Fredo's Farm Hub, in staging-FR. It had a flat percent (10%) on a shipping method (Home delivery), with an invisible tag set. So this shipping method was not appearing in the shopfront. Other pick-up option had zero fees set, and other enterprise fees were applied equally, before and after staging.

I avoided editing these settings explicitly, as editing would probably refresh the DB and show/apply the fees correctly.

Anyway, all looks good before after staging.

I will leave this PR in test ready for now, and try to find a shop, on another staging server, with shipping fees set, see if I can verify the issue later on.

"UPDATE spree_preferences pref SET key = " + updated_pref_key +
" WHERE pref.key like '" + from_pattern + "%'" +
" AND NOT EXISTS (SELECT 1 FROM spree_preferences existing_pref
WHERE existing_pref.key = " + updated_pref_key + ")"
Copy link
Contributor

Choose a reason for hiding this comment

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

There's blank spaces either side of the string in these keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do you ask? no blank spaces, just more characters like the name of the fee (flat_rate for example), that's why we need the %.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe you are looking at updated_pref_key and thinking it's a string? updated_pref_key is a call to the replace function and so it can have spaces before and after.

@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Jul 23, 2020
@filipefurtad0
Copy link
Contributor

A quick update:

I tried to reproduce the issue as well on an old "untouched" hub in staging-katuma, but still no success: the fees appear in the order total, and are charged as expected.

So I did not notice any hub whose settings were set to zero... This is similar to the issue we observed to Katuma, in production - right? I can't recall having seen any setting misplaced in staging servers, other than missing images. Can we pin-point such a case, in staging?

After review comments are addressed I'd move this to ready to go.

@luisramos0
Copy link
Contributor Author

luisramos0 commented Jul 24, 2020

Hey Filipe, apologies, trying to replicate it myself I just realize it is impossible for your replicate without looking at the DB!
So, in katuma DB I see:
image

and this means that this method should not be zero but 2 eur:
image

and so when I checkout in that shop home delivery should be 2eur not Free:
image

In the same way (so I test this for a payment method as well) because I see this in the DB:
image

I know this payment method should have a flat rate of 1eur and not zero:
image
and on checkout stripe should cost 1eur not be free:

image

I am now going to deploy this PR to katuma staging and verify these fees show up after the deploy.

@luisramos0
Copy link
Contributor Author

luisramos0 commented Jul 24, 2020

ok, after deploying this PR the fees were updated on the DB but the cache was holding the old values, so the issue was not fixed.
I had to connect to the server and run Rails.cache.clear, after that the fix worked, fees were back:
image

I added a commit to clear rails cache after the migration. I dont think this needs code review, it also done in the previous preferences migration, I could have thought about this 🤕
I am now going to test the new version of the PR (with the cache clear) on staging AUS just to be sure about all this 👍

@luisramos0 luisramos0 added pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-es pr-staged-uk staging.openfoodnetwork.org.uk labels Jul 24, 2020
@luisramos0
Copy link
Contributor Author

yep, all checked and nice in AUS, I run the same test as described above for an enterprise with existing fees and it all worked after the deployment of this new version. Merging now.

@luisramos0 luisramos0 merged commit 6039919 into openfoodfoundation:master Jul 24, 2020
@luisramos0 luisramos0 deleted the calcs_bug branch July 24, 2020 13:23
@luisramos0
Copy link
Contributor Author

Just to be clear: this is a release blocker, that's why I merge now. We can address anything that we find out with @Matt-Yorkley s comment above on a separate PR.

@luisramos0 luisramos0 removed the pr-staged-au staging.openfoodnetwork.org.au label Jul 24, 2020
@filipefurtad0
Copy link
Contributor

Awesome @luisramos0!

Thank you for documenting this, reproducing the issue and double checking it now works - glad you did.

Thanks.

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.

Shipping Method and Transaction Fees removed from Enterprise
4 participants