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

2072 New seed data for testing #3209

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@mkllnk
Copy link
Member

mkllnk commented Dec 13, 2018

What? Why?

Closes #2072

We need more and better structured sample data for dev testing and to bootstrap staging data. This pull request introduces a new rake task that aims to replace openfoodnetwork:dev:sample_data.

The generated data is mainly the data defined in https://docs.google.com/document/d/1_va0Aw2yJ5BRe-HtuBT-OC8HNHIui0gvQ0uElWjDVs0. There are the following differences:

  • There is no difference between a producer profile and a shop profile. We just have enterprise profiles. Therefore I introduced the user Penny with her profile enterprise. The documented enterprise 1, Fred's farm, is a normal producer. And the documented enterprise 4, Mary's shop, is a normal shop.
  • VAT has not been considered yet. I can do that in another iteration.
  • The second sub-distributor and its order cycle are missing. I didn't see a need for it. That may come with VAT.
  • Some of the naming is slightly different as it made more sense to me.
  • I filled in some missing data like pickup instructions.

I tried to keep this as short as possible, but I spent 22 hours on this already. I made some effort though to make it easier to maintain and extend. It has much better code metrics than the old code.

What should we test?

This cannot be staged without a developers help. The developer needs to run the following to make this data active:

sudo /bin/systemctl stop delayed_job_openfoodnetwork.service
sudo /bin/systemctl stop unicorn_openfoodnetwork.service
mkdir -p db/backup/
pg_dump -h localhost -U ofn_user openfoodnetwork | gzip > db/backup/staging-`date +%Y%m%d%H%M%S`-before-reset.sql.gz
bundle exec rake db:reset openfoodnetwork:sample_data
sudo /bin/systemctl start delayed_job_openfoodnetwork.service
sudo /bin/systemctl start unicorn_openfoodnetwork.service

Testers can then verify that the data is set up as specified. I think, @myriamboure worked on the data with certain test scenarios in mind. Would you like to share these so that we can verify that they work with this data?

Release notes

Changed: Added and improved sample data for testing and demo instances.

Changelog Category: Changed

How is this related to the Spree upgrade?

Some of the data generation may have to be updated for the Spree upgrade.

Discourse thread

https://community.openfoodnetwork.org/t/seed-data-development-provisioning-deployment/910

@mkllnk mkllnk self-assigned this Dec 13, 2018

@luisramos0
Copy link
Contributor

luisramos0 left a comment

Hey, amazing @mkllnk , let's go forward and iterate on top of this.

As I see these factories, I wonder if we should use the factories in spec/factories here or adapt spec/factories to be used here. If we do that our automated acceptance tests will look familiar to everyone.

btw, the code climate warnings should be easy fixes.

@mkllnk

This comment has been minimized.

Copy link
Member

mkllnk commented Dec 13, 2018

I wonder if we should use the factories in spec/factories

I wondered the same. There is some potential code duplication here. But I didn't use those factories for two reasons:

  1. They generate more data that is not explicitly specified by our task.
  2. They generate some random data which is not deterministic and doesn't fit the purpose here.

While generating sample data and spec data are similar tasks, I don't like to tie the two together. I like to specify everything explicitly and design the factories to be used as easily as possible for our sample data. The old task used the spec factories, but I don't think it made it better.

One lesson from reading these factories is that our data structures are badly complex. Working on the models to make the factories easier would also reduce the potential of bugs a lot.

@luisramos0

This comment has been minimized.

Copy link
Contributor

luisramos0 commented Dec 14, 2018

👍 it's not an obvious decision. I think both ways are fine.

We could simplify the factories so they fit the sample data. And factories with random data is something we shouldn't have.

@myriamboure

This comment has been minimized.

Copy link
Contributor

myriamboure commented Dec 18, 2018

Thanks @mkllnk ! I need to dive again into it which is not easy ;-)

There is no difference between a producer profile and a shop profile. We just have enterprise profiles. Therefore I introduced the user Penny with her profile enterprise. The documented enterprise 1, Fred's farm, is a normal producer. And the documented enterprise 4, Mary's shop, is a normal shop.

Then you created duplicates, as I created those two profiles to make sure we distinguish how they look like on the map if needed (4 is in white color for instance as distributor and not using the shopfront). Actually there are differences between a producer and a shop profile.
If you wanted to only have one profile only entity (maybe that's enough...) then I would simply have deleted number 4 and transform number 1 into a "enterprise profile" that is described as a producer (so appear with the tractor and in green.

But anyway, it might not harm...

VAT has not been considered yet. I can do that in another iteration.

I don't think you will be able to checkout without a VAT zone setup in configuration... don't you think ? If it's not a problem then no problem we can iterate. But we definitely do want to test that VAT is properly calculated, and VAT reports display correctly, etc. So we would need it to avoid to recreate VAT categories and rates in config each time we want to test some VAT potential impact of a PR.

The second sub-distributor and its order cycle are missing. I didn't see a need for it. That may come with VAT.

The second hub distributor was setup to test inventories. As it's OC is sourcing info from inventory and not from product catalog, and this hub (Maryna hub) is the only one to use the inventory feature.
Also I used it to test a specific case : I created 2 OC on this hub to have a case setup with multiple OC at the same time.

Anyway, I'll write some test scenarios at the end of the document, will do it now ! Cheers :-)

@myriamboure

This comment has been minimized.

Copy link
Contributor

myriamboure commented Dec 18, 2018

Oh sorry about the second sub-distributor maybe you meant "Maryse" ? I did create her because she runs a private shop so we can have a private shop case to do tests on it. Anyway, I'll clarify all that with the scenario.

@sauloperez
Copy link
Contributor

sauloperez left a comment

Looks pretty good to me. I just left a couple comments

def log(message)
puts "[openfoodnetwork:sample_data:load] #{message}"
end
end

This comment has been minimized.

@sauloperez

sauloperez Dec 18, 2018

Contributor

we could use the rails logger as any part of Rails. TaggedLogging is a perfect fit for this.

This comment has been minimized.

@mkllnk

mkllnk Dec 19, 2018

Member

Ah, yes, that looks quite apt. But I don't see the benefit in this simple scenario. The change would be this:

-  puts "[openfoodnetwork:sample_data:load] #{message}"
+  @logger ||= ActiveSupport::TaggedLogging.new(Logger.new(STDOUT))
+  @logger.tagged("openfoodnetwork:sample_data:load") { @logger.info(message) }

That's much more complicated. What is the added value?

This comment has been minimized.

@sauloperez

sauloperez Dec 19, 2018

Contributor

It's more about using a logger rather than puts. We have logging built-in in Rails that we could leverage. You can then add those tags using the formatter attribute of the Rails logger for instance.

https://logmatic.io/blog/rails-logger-best-practices/ seems to be a good post about the topic.

This comment has been minimized.

@mkllnk

mkllnk Jan 8, 2019

Member

I changed it now as you suggested, but I don't see any added value in this case.

def country
Spree::Country.find_by_iso(ENV.fetch('DEFAULT_COUNTRY_CODE'))
end
end

This comment has been minimized.

@sauloperez

sauloperez Dec 18, 2018

Contributor

I don't see any reason why not to have this modules and classes in their own files and require them from the rake task. This is just Ruby code after all.

This comment has been minimized.

@mkllnk

mkllnk Dec 18, 2018

Member

The reason was that we don't have a structure for that yet. Where should those files go? In lib/tasks/sample_data/?

This comment has been minimized.

@sauloperez

sauloperez Dec 19, 2018

Contributor

yep, sounds good to me.

This comment has been minimized.

@kristinalim

kristinalim Jan 6, 2019

Member

I agree with @sauloperez. The Rake task is too long and difficult to read, and can be split into multiple files in lib/tasks/sample_data.

This comment has been minimized.

@mkllnk

mkllnk Jan 8, 2019

Member

I agree. I split it all up now.

@myriamboure

This comment has been minimized.

Copy link
Contributor

myriamboure commented Dec 18, 2018

Ok so I wrote at the end of the document the cases I had in mind when writing this document (it has been reviewed by Sally at that time...) https://docs.google.com/document/d/1_va0Aw2yJ5BRe-HtuBT-OC8HNHIui0gvQ0uElWjDVs0/edit so I guess in order to test we should somehow test that the different cases are setup as expected ? Please check @mkllnk that it's what you understood, as with the changes you made I think some of those test cases are not setup then. But anyway, we can test and see if that's ok as you did it to start with, and iterate.

@mkllnk

This comment has been minimized.

Copy link
Member

mkllnk commented Dec 19, 2018

Thank you!

@myriamboure

Actually there are differences between a producer and a shop profile.

Okay, I think we used different terminology. I thought a profile is an enterprise that is not a producer and sells none. That's how the shops page lists them. A profile has no connection to any products. They just have a name, image, description and so on.

From your scenarios I understand that a producer profile is what we just call a producer. They can actively manage their produce which can be sold through other enterprises.

I see four different icons on the map. And I think this is what they mean:

  • White pin: a profile, not a producer, not selling anything
  • Red pin: a hub/shop/distributor: selling own or any.
  • Green tractor: a producer, managing their stock to be sold through other enterprises
  • Red tractor: a producer and distributor combined.

So the tractor symbol indicates the producer property and the colour red indicates selling.

In my data I created Penny Profile. That is probably matching what you describe for Mary's Physical Shop. I called her Penny, because for OFN it doesn't matter if it's a physical shop, a physical farm or just a public produce swap room. It's only a profile using the OFN as directory, not as shop or producer. But now I see how you thought of it as a shop profile, because the symbol on the map is the same. I don't thing that was the intention though.

we definitely do want to test that VAT is properly calculated

I thought so. I just wanted to get some feedback after working on this for such a long time. I will add VAT.

The second hub distributor was setup to test inventories.

I created an inventory for 5-“Maryel hub”. Does it matter which hub is using the inventory?

That shop also has two order cycles and one is shared with the private shop 7-Maryse-hub. I skipped the first sub-distributor, but I can add it to distinguish between VAT and no VAT.

# - https://community.openfoodnetwork.org/t/seed-data-development-provisioning-deployment/910
# - https://github.com/openfoodfoundation/openfoodnetwork/issues/2072
#
namespace :openfoodnetwork do

This comment has been minimized.

@sauloperez

sauloperez Dec 19, 2018

Contributor

May I ask to remove this namespace? Similar to what I said a few times about the lib/openfoodnetwork/ IMO this namespace brings no value and just redundancy. Not less important, it is long to type.

We also have lib/tasks/dev.rake which has openfoodnetwork:dev:load_sample_data in it. We need to decide what we do with that dev namespace. Either we put this new sample_data:load within it or remove it.

And the last question, do we plan to have any task other than load? If not, we can simply have sample_data.

@Matt-Yorkley
Copy link
Contributor

Matt-Yorkley left a comment

Is it possible to simplify this by calling FactoryBot factories to create these objects? I think I did it once before when creating sample data in my dev environment.

@Matt-Yorkley

This comment has been minimized.

Copy link
Contributor

Matt-Yorkley commented Dec 21, 2018

This suggestion isn't strictly necessary, but: if we're creating enterprises with different configurations and relationships, can we describe it in each enterprise's profile description so that any dev or tester can click on them under /shops and see an explanation, like "This shop sells it's own produce, and supplies Enterprise X", or "This is a hub that stocks products from Enterprise Y and inventory from Enterprise Z"?

@sauloperez

This comment has been minimized.

Copy link
Contributor

sauloperez commented Dec 21, 2018

That is nice to have

@luisramos0

This comment has been minimized.

Copy link
Contributor

luisramos0 commented Jan 4, 2019

@sauloperez can we move forward with this PR? the namespace changes will be done somewhere else right?

@myriamboure

This comment has been minimized.

Copy link
Contributor

myriamboure commented Jan 5, 2019

@mkllnk I guess we can start with what you have done, when testing I suggest we carefully document then / adapt the use cases I describe to the reality of what you have done. If we realize there are important cases missing we will comment when testing. Let's move forward.

@kristinalim
Copy link
Member

kristinalim left a comment

This should be split into multiple files. I also marked a few places that I think were overlooked and bang methods should be used.

Aside from those, looks great! 🙂 Excited to start using this.

def country
Spree::Country.find_by_iso(ENV.fetch('DEFAULT_COUNTRY_CODE'))
end
end

This comment has been minimized.

@kristinalim

kristinalim Jan 6, 2019

Member

I agree with @sauloperez. The Rake task is too long and difficult to read, and can be split into multiple files in lib/tasks/sample_data.


def zone
zone = Spree::Zone.find_or_create_by_name!("Australia")
zone.members.create(zonable: country)

This comment has been minimized.

@kristinalim

kristinalim Jan 6, 2019

Member

This should be a bang method.


def create_product_with_distribution(params)
product = Spree::Product.create_with(params).find_or_create_by_name(params[:name])
ProductDistribution.create(

This comment has been minimized.

@kristinalim

kristinalim Jan 6, 2019

Member

Same here and the line above - these should be bang methods.

This comment has been minimized.

@mkllnk

mkllnk Jan 8, 2019

Member

Thank you Kristina! Changing this revealed a validation error on product distributions. :-) It's fixed now.

hub: shop,
price: 12,
count_on_hand: 5
).find_or_create_by_variant_id(product.variants.first.id)

This comment has been minimized.

@kristinalim

kristinalim Jan 6, 2019

Member

This should be a bang method.

enterprise: shop,
variant: product.variants.first,
visible: true
).find_or_create_by_variant_id(product.variants.first.id)

This comment has been minimized.

@kristinalim

kristinalim Jan 6, 2019

Member

This should be a bang method.

@mkllnk mkllnk force-pushed the mkllnk:2072-test-data branch 5 times, most recently from 227029f to 9e0499f Jan 8, 2019

@mkllnk

This comment has been minimized.

Copy link
Member

mkllnk commented Jan 8, 2019

I think I addressed all the feedback, including enterprise descriptions. The namespace will be handled in another pull request.

@daniellemoorhead

This comment has been minimized.

Copy link

daniellemoorhead commented Jan 8, 2019

Semaphoreci failed @mkllnk :(

@sigmundpetersen

This comment has been minimized.

Copy link
Contributor

sigmundpetersen commented Jan 8, 2019

It was a random Semaphore failure. It passed on a rebuild. But yes, fixing these inconsistent spec fails should be prioritized soon.

@kristinalim
Copy link
Member

kristinalim left a comment

Looks great, @mkllnk!

Do we need a new issue for having script/setup use this new Rake task after a few iterations?

@mkllnk

This comment has been minimized.

Copy link
Member

mkllnk commented Jan 9, 2019

Ah, good one. I never use the setup script. But yes, we should update that as well.


It contains more sample data.

WARNING

This comment has been minimized.

@sauloperez

sauloperez Jan 9, 2019

Contributor

nothing to do in this PR but Rails has tool for this. See https://apidock.com/rails/v3.2.13/ActiveSupport/Deprecation/warn/class

@sauloperez
Copy link
Contributor

sauloperez left a comment

👏 looks very good. I just left a few improvements I think could be made, but are not a big deal. Something for an upcoming PR if we think they deserve to be added.

is_primary_producer: false,
sells: "none",
address: address("25 Myrtle Street, Bayswater, 3153"),
long_description: <<DESC

This comment has been minimized.

@sauloperez

sauloperez Jan 9, 2019

Contributor

strip_heredoc is perfect for this.

This comment has been minimized.

@mkllnk

mkllnk Jan 9, 2019

Member

I used that in my first implementation, but faced two hurdles:

  1. The display in the admin interface HTML editor was odd, because it ignored newlines and then it missed spaces. I then added tr("\n", " ") as well, but removed the whole lot when I saw the next issue:
  2. Rubocop complained about complexity. I can avoid that by moving the logic to the create_samples method, but then it seemed a lot of effort for something that is not needed. That whitespace doesn't hurt.

I can bring that line back if you like:

data[:long_description] = data[:long_description].strip_heredoc.tr("\n", " ")

This comment has been minimized.

@sauloperez

sauloperez Jan 16, 2019

Contributor

don't worry, it was just a suggestion to make you aware of that method if you weren't. Do as you find is best.


private

def create_order_cycle(name, coordinator_name, supplier_names, distributor_names, data)

This comment has been minimized.

@sauloperez

sauloperez Jan 9, 2019

Contributor

keyword arguments would be perfect here to make calls to it a bit easier to understand. 5 arguments is already a lot.

)
end

def create_card_method(enterprise)

This comment has been minimized.

@sauloperez

sauloperez Jan 9, 2019

Contributor

At this point, it's clear to me that enterprise might be worth being an ivar. It's class state.

)
end

def create_payment_method(enterprise, name, description, calculator)

This comment has been minimized.

@sauloperez

sauloperez Jan 9, 2019

Contributor

keyword arguments would make this also a bit more readable.

mkllnk added some commits Dec 13, 2018

Rewrite task to generate sample data
#2072

We need more and better structured sample data for dev testing and to
bootstrap staging data. This new task aims to replace
openfoodnetwork:dev:sample_data.
Fix sample product creation and use bang methods
Mistakes like the missing fee when creating product distributions were
hidden, because I didn't use the bang methods to create records.

@mkllnk mkllnk force-pushed the mkllnk:2072-test-data branch from 9e0499f to 9c05bb8 Jan 10, 2019

@mkllnk

This comment has been minimized.

Copy link
Member

mkllnk commented Jan 10, 2019

Thank you for the suggestions @sauloperez. I learned a lot. I incorporated more of the feedback and since it's all approved now, I rebased it as well.

  • The task is now simply openfoodnetwork:sample_data without load and another pr will change it to ofn:sample_data.
  • I used the ActiveSupport deprecation feature in the old task.
  • I used strip_heredoc for enterprise descriptions.
  • I updated bin/setup.

I didn't go for named parameters, because that would have been a lot of redundancy. But maybe we will do it in another iteration.

@sauloperez

This comment has been minimized.

Copy link
Contributor

sauloperez commented Jan 16, 2019

Shall we merge? did you @myriamboure take a look at it? let's iterate! 💪

@sauloperez

This comment has been minimized.

Copy link
Contributor

sauloperez commented Jan 16, 2019

I just deployed to AU's staging and executed those commands but unfortunately, sample_data failed. See the output in output.log

@mkllnk mkllnk added the pr-staged-au label Jan 22, 2019

@mkllnk

This comment has been minimized.

Copy link
Member

mkllnk commented Jan 22, 2019

I finally got around to testing this. It works in development environment, but fails in staging. Investigating...

@mkllnk mkllnk removed the pr-staged-au label Jan 22, 2019

Fix creation of sample payment methods on staging
Due to an unknown reason my chosen way of creating calculator and
assigning it to a payment method didn't work in staging environment even
though it was working in development.

Without diving deeper into the cause of this, I decided to change the
record assignments and fix the problem that way.
@mkllnk

This comment has been minimized.

Copy link
Member

mkllnk commented Jan 22, 2019

Okay, I found a workaround. Back to test-ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment