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

[Spree Upgrade] Fix Enterprise fee report specs #3651

Merged

Conversation

Projects
None yet
4 participants
@luisramos0
Copy link
Contributor

commented Mar 25, 2019

What? Why?

Closes #3483

Chrome driver doesnt support file downloads with standard config so we need specific configuration to make it download files and have specs get the file contents.

What should we test?

Spec should be green.
We should test the enterprise fees summary report and make sure it's working correctly.

This build is green if I include the commits from #3316 (they fix the authorization problem, they are present in build number 2 in this PR). I have removed those commits now to make the review of this PR easier. So, this should be merged only after #3316.

@luisramos0 luisramos0 self-assigned this Mar 25, 2019

@luisramos0 luisramos0 changed the title 2 0 enterprise fee report [Spree Upgrade] Fix Enterprise fee report specs Mar 25, 2019

@luisramos0 luisramos0 force-pushed the luisramos0:2-0-enterprise-fee-report branch 2 times, most recently from e357e0c to 48743fa Mar 25, 2019

@kristinalim
Copy link
Member

left a comment

Awesome work in DownloadsHelper, @luisramos0!

Show resolved Hide resolved spec/spec_helper.rb Outdated
Show resolved Hide resolved spec/features/admin/reports/enterprise_fee_summaries_spec.rb
@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@kristinalim all credit for this PR to the internet 🤣 DownloadsHelper for example was copied from here: https://collectiveidea.com/blog/archives/2012/01/27/testing-file-downloads-with-capybara-and-chromedriver

@luisramos0 luisramos0 requested a review from kristinalim Mar 28, 2019

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

The build is fine, needs #3316.

@luisramos0 luisramos0 force-pushed the luisramos0:2-0-enterprise-fee-report branch from f8ecaab to 40124b7 Apr 2, 2019

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

I just rebased this to see the green build.

@kristinalim
Copy link
Member

left a comment

Looks great, @luisramos0!

@RachL RachL added the pr-staged-fr label Apr 11, 2019

@RachL RachL self-assigned this Apr 11, 2019

@RachL

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@luisramos0 I've loaded the same period of data in staging and in production. But the results don't match. Is there anything else than the date that can change the results of this report ?

https://docs.google.com/document/d/17-d1pT1y5eWYbCg-bY2NdmQRULZDoSdZSZPGPSeqKBY/edit#

@RachL RachL added pr-staged-fr and removed pr-staged-fr labels Apr 12, 2019

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

This PR only changes specs. So, this PR cannot be breaking the report. I think the scope of testing is more about validating the report itself in v2.

Note that the data is not only truncated, it's also anonymised and that can break data connections as we have seen in another report (for example, order.email will not match order.user.email).

@RachL can you share the exact filter you used so I can explain exactly why the numbers don't match between live users and anonimized users?

@RachL

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

@luisramos0 yes of course.

So on both platform I've chosen a date range From January the 1st to April the 1st. You can reach data without breaking anything with just this filter.

The first 3 lines in the results show the 5 per cent fee for a particular hub. These 3 lines are linked to three different users.

The first line has a different sum in staging than in production :

Staging:

image

Production:

image

I've understood reading this: https://community.openfoodnetwork.org/t/report-that-isolates-various-enterprise-fees/1361/4 that there should be only one line per hub-feetype-customer-feeplacement.

I thought that maybe they were aggregated by OC, so I've filtered through the "christmas holiday" OC. But I still see differences. So what am I missing?

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

eheh, it took me a while to confirm that it's just because the data changed after the data load to staging...
we moved data from live uk to stg FR on the 29th of March and the data related to this 5 per cent fee has changed on the 30th Mars. those 0.5 are from the 30th of Mars...

Can you please use data fro Feb for example, less chances of mismatch :-)

@RachL

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

@luisramos0 it still didn't change anything I still had different figures... The feature is not yet documented enough in order for me to understand all the rules that applies. So I change my testing approach and did the follow tests :

  • create an order and see if the fees displayed well in the report : they did
  • past orders fees seems to display correctly as well.

This is the first time I'm testing this report in v2, so I guess staging AU is not updated with this compliant version right? When it is, I will ask Kirsten and Theresa to have a deeper look and help on creating the user guide for this.

@RachL RachL removed the pr-staged-fr label Apr 16, 2019

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

ok! staging AU does contain the same version of the reports because this PR is only fixing the specs..

@luisramos0 luisramos0 merged commit 29000eb into openfoodfoundation:2-0-stable Apr 16, 2019

1 check passed

semaphoreci The build passed on Semaphore.
Details

@luisramos0 luisramos0 deleted the luisramos0:2-0-enterprise-fee-report branch Apr 16, 2019

@luisramos0 luisramos0 referenced this pull request May 10, 2019

Merged

Webdrivers #3828

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.