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

Adjust minimum validators on CartLine, OrderLine and FulfillmentLine #2925

Merged
merged 3 commits into from Sep 21, 2018

Conversation

patrick-emmanuel
Copy link
Contributor

I want to merge this change because...
close #2912

Screenshots

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. The changes are tested.
  6. The code is documented (docstrings, project documentation).
  7. GraphQL schema and type definitions are up to date.

@patrick-emmanuel patrick-emmanuel changed the title Adjust minimum validators Adjust minimum validators on CartLine, OrderLine and FulfilmentLine Sep 20, 2018
@Pacu2
Copy link
Contributor

Pacu2 commented Sep 20, 2018

I think you need to include migrations for those changes.

@codecov
Copy link

codecov bot commented Sep 20, 2018

Codecov Report

Merging #2925 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2925   +/-   ##
=======================================
  Coverage   89.63%   89.63%           
=======================================
  Files         208      208           
  Lines       10728    10728           
  Branches     1039     1039           
=======================================
  Hits         9616     9616           
  Misses        792      792           
  Partials      320      320
Impacted Files Coverage Δ
saleor/graphql/core/mutations.py 93.51% <ø> (ø) ⬆️
saleor/checkout/models.py 98.76% <100%> (ø) ⬆️
saleor/order/models.py 95.87% <100%> (ø) ⬆️

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 cbc4040...36c9bd2. Read the comment docs.

@patrick-emmanuel
Copy link
Contributor Author

patrick-emmanuel commented Sep 20, 2018

Yeah, that's right.

What do you think about writing tests for models with simple logic like this one?

Copy link
Contributor

@Pacu2 Pacu2 left a comment

Choose a reason for hiding this comment

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

I think we could also remove MaxValueValidator, not sure what they're for? What do you think @maarcingebala ?

@maarcingebala
Copy link
Member

@Pacu2 Yeah, I guess we could drop them. Would you mind removing them @the-bionic?

@patrick-emmanuel
Copy link
Contributor Author

I'll do that right away.

@patrick-emmanuel
Copy link
Contributor Author

How about dropping the MaxValueValidator on quantity_fulfilled as well?

@Pacu2
Copy link
Contributor

Pacu2 commented Sep 21, 2018

@the-bionic Yes please

@patrick-emmanuel patrick-emmanuel changed the title Adjust minimum validators on CartLine, OrderLine and FulfilmentLine Adjust minimum validators on CartLine, OrderLine and FulfillmentLine Sep 21, 2018
Copy link
Contributor

@Pacu2 Pacu2 left a comment

Choose a reason for hiding this comment

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

LGTM, could you please rebase with the latest master? I think we are already on 61st migration in orders

@Pacu2
Copy link
Contributor

Pacu2 commented Sep 21, 2018

Thank you for picking this up!

@patrick-emmanuel
Copy link
Contributor Author

Glad to be of assistance.

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.

It should not be possible to create a line with 0 quantity
3 participants