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.

Sign up for GitHub

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

Jump to bottom

Fix for populatedb and currency issue #3512

Merged
merged 1 commit into from Jan 9, 2019
Merged

Fix for populatedb and currency issue #3512

merged 1 commit into from Jan 9, 2019

Conversation

stephenmoloney
Copy link
Contributor

@stephenmoloney stephenmoloney commented Dec 22, 2018 •
edited by maarcingebala

What does this commit/MR/PR do?

  • Changes the currency of the demo objects to the default currency

Why is this commit/MR/PR needed?

  • So that populateDB works

Relevant issues:

#3511

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. Database migration files are up to date.
  6. The changes are tested.
  7. The code is documented (docstrings, project documentation).
  8. GraphQL schema and type definitions are up to date.
  9. Changes are mentioned in the changelog.

@stephenmoloney
Copy link
Contributor Author

@Pacu2 could you take a look at this. I'm not a python developer.

I haven't tested this yet at all either. But something along these lines might fix #3511 .

@Pacu2
Copy link
Contributor

Pacu2 commented Dec 23, 2018

@stephenmoloney I like the changes, however I think we could move it directly to the methods affected.

Eg. create_product_variants
https://github.com/mirumee/saleor/pull/3512/files#diff-103ebc2d83ae6722bfe0b0b21f446da6L199

and create_products
https://github.com/mirumee/saleor/pull/3512/files#diff-103ebc2d83ae6722bfe0b0b21f446da6L186

@stephenmoloney
Copy link
Contributor Author

Any ideas to overcome this error -
https://travis-ci.org/mirumee/saleor/jobs/471641544#L1500

I'm not familiar with python types

Pacu2
Pacu2 reviewed Dec 24, 2018
View reviewed changes
saleor/core/utils/random_data.py Outdated Show resolved Hide resolved
saleor/core/utils/random_data.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 24, 2018 •
edited

Codecov Report

Merging #3512 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3512   +/-   ##
=======================================
  Coverage   89.83%   89.83%           
=======================================
  Files         250      250           
  Lines       13217    13217           
  Branches     1338     1338           
=======================================
  Hits        11874    11874           
  Misses        930      930           
  Partials      413      413

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 511e85a...eaa4924. Read the comment docs.

@stephenmoloney
Copy link
Contributor Author

stephenmoloney commented Dec 24, 2018 •
edited

I think I sorted this - disregard !

@Pacu2
I ran into a code complexity error, so I've externalized the logic into an intermediate function.
Again, I'm not a python guy - maybe you can have a look.
https://codeclimate.com/github/mirumee/saleor/pull/3512#

image

I'm getting this error now:

https://travis-ci.org/mirumee/saleor/jobs/471870324#L1493

I think I sorted this - disregard !

@stephenmoloney stephenmoloney force-pushed the fix/populate-db-currency branch 3 times, most recently from 624b95e to a416588 Compare December 24, 2018 19:08
@stephenmoloney stephenmoloney force-pushed the fix/populate-db-currency branch 3 times, most recently from 3253f76 to 4e1773f Compare January 1, 2019 07:35
@stephenmoloney
Copy link
Contributor Author

@Pacu2 Is there anything more you want done in this PR ?

maarcingebala
maarcingebala reviewed Jan 8, 2019
View reviewed changes
Copy link
Member

@maarcingebala maarcingebala left a comment •
edited

Choose a reason for hiding this comment

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

Hey, sorry for the late review, I didn't merge this PR yet because I think this function is a bit overcomplicated. I left a few comments. These are small changes but would greatly improve the readability.

saleor/core/utils/random_data.py Outdated Show resolved Hide resolved
saleor/core/utils/random_data.py Outdated Show resolved Hide resolved
@stephenmoloney stephenmoloney force-pushed the fix/populate-db-currency branch 3 times, most recently from 43a97c1 to 62e3186 Compare January 8, 2019 12:42
@stephenmoloney
Fix for populatedb and currency issue
eaa4924 
What does this commit/MR/PR do?

- Changes the currency of the demo objects to the default currency

Why is this commit/MR/PR needed?

- So that populateDB doesn't break due to incorrect currency
patrys
patrys reviewed Jan 8, 2019
View reviewed changes
saleor/core/utils/random_data.py
@@ -183,6 +183,8 @@ def create_products(products_data, placeholder_dir, create_images):
defaults['weight'] = get_weight(defaults['weight'])
defaults['category_id'] = defaults.pop('category')
defaults['product_type_id'] = defaults.pop('product_type')
defaults['price'] = get_in_default_currency(
Copy link
Member

Choose a reason for hiding this comment

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

Why do this workaround instead of fixing how defaults are generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be good if the defaults were generated with the right currency - they seem to be coming from a static json file. Right now I've only got time and sufficient python to offer this workaround.

Yea - it's static json - no generation involved. https://github.com/mirumee/saleor/blob/11f30e13551069299a97e73b65f086a920cf63ec/saleor/core/utils/random_data.py#L210

Copy link
Member

Choose a reason for hiding this comment

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

Exactly, this data is coming from db.json which has USD currency hardcoded. I think this is an acceptable solution for now. If we have a better idea I'd like to have it described in this PR or a new issue.

@maarcingebala maarcingebala merged commit 04a15be into saleor:master Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrys patrys patrys left review comments

@Pacu2 Pacu2 Pacu2 left review comments

@maarcingebala maarcingebala maarcingebala approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@stephenmoloney @Pacu2 @patrys @maarcingebala