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 publication date to collections #3369

Merged
merged 12 commits into from Dec 20, 2018
Merged

Add publication date to collections #3369

merged 12 commits into from Dec 20, 2018

Conversation

k-brk
Copy link
Contributor

@k-brk k-brk commented Dec 1, 2018

I want to merge this change because...

resolves #3125

  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.

@Pacu2
Copy link
Contributor

Pacu2 commented Dec 1, 2018

Could you please use closing keywords in the PR description? It makes maintainers life easier https://help.github.com/articles/closing-issues-using-keywords/

Copy link
Member

@NyanKiyoshi NyanKiyoshi left a comment

Choose a reason for hiding this comment

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

  • Published at is wrong English (at <place>) and even replacing it by published on would be kinda odd. I would suggest replacing published at with publish_date;
  • The availability rendering has some logic issues;
  • The 404 tests could get merged into a multi-cases test.

saleor/product/models.py Outdated Show resolved Hide resolved
saleor/product/models.py Outdated Show resolved Hide resolved
saleor/graphql/product/mutations/products.py Outdated Show resolved Hide resolved
saleor/dashboard/collection/forms.py Outdated Show resolved Hide resolved
templates/collection/index.html Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/test_collection.py Outdated Show resolved Hide resolved
templates/dashboard/includes/_collection_availability.html Outdated Show resolved Hide resolved
saleor/product/models.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 2, 2018

Codecov Report

Merging #3369 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3369      +/-   ##
==========================================
+ Coverage   89.62%   89.64%   +0.02%     
==========================================
  Files         237      237              
  Lines       12783    12789       +6     
  Branches     1283     1283              
==========================================
+ Hits        11457    11465       +8     
+ Misses        923      922       -1     
+ Partials      403      402       -1
Impacted Files Coverage Δ
saleor/dashboard/collection/forms.py 100% <ø> (ø) ⬆️
saleor/core/templatetags/status.py 78.72% <100%> (ø) ⬆️
saleor/graphql/product/mutations/products.py 96.24% <100%> (ø) ⬆️
saleor/product/models.py 92.48% <100%> (+0.12%) ⬆️
saleor/product/utils/__init__.py 95.4% <0%> (+2.29%) ⬆️

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 044f85d...3e1a539. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 2, 2018

Codecov Report

Merging #3369 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3369      +/-   ##
==========================================
+ Coverage   89.87%   89.89%   +0.01%     
==========================================
  Files         241      241              
  Lines       13103    13107       +4     
  Branches     1322     1322              
==========================================
+ Hits        11776    11782       +6     
+ Misses        922      921       -1     
+ Partials      405      404       -1
Impacted Files Coverage Δ
saleor/dashboard/collection/forms.py 100% <ø> (ø) ⬆️
saleor/core/templatetags/status.py 78.72% <100%> (ø) ⬆️
saleor/graphql/product/mutations/products.py 96.04% <100%> (ø) ⬆️
saleor/product/models.py 93.09% <100%> (+0.06%) ⬆️
saleor/product/utils/__init__.py 95.4% <0%> (+2.29%) ⬆️

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 8106049...de9ce0e. Read the comment docs.

saleor/product/migrations/0081_auto_20181202_1337.py Outdated Show resolved Hide resolved
saleor/product/models.py Outdated Show resolved Hide resolved
templates/collection/index.html Outdated Show resolved Hide resolved
templates/collection/index.html Outdated Show resolved Hide resolved
templates/dashboard/includes/_collection_availability.html Outdated Show resolved Hide resolved
templates/dashboard/includes/_collection_availability.html Outdated Show resolved Hide resolved
tests/test_collection.py Outdated Show resolved Hide resolved
tests/test_collection.py Show resolved Hide resolved
@NyanKiyoshi
Copy link
Member

@k-brk could you flag the yellow outdated reviews as resolved? That would be awesome, because the page is really messy! 👍

@k-brk
Copy link
Contributor Author

k-brk commented Dec 7, 2018

Commit 04c5b22 introduced changes in type of CollectionInput(graphql/product/mutations/products.py) published_date field, from string to date. Had to change data dumper from json to djangoJsonEncoder(which is almost the same as json.JSONEncoder) in tests/api/conftest.py due to the fact that built in json module does not serialize dates. After this change security/snyk is failing. Should I roll back it to string as it was before? Similar string approach is used in PageInput (graphql/page/mutations.py) available_on field.

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.

Apart from this comment, looks good to me

templates/collection/index.html Outdated Show resolved Hide resolved
@Pacu2
Copy link
Contributor

Pacu2 commented Dec 12, 2018

@k-brk Snyk is checking for dependency vulnerabilities and this error is not connected to your changes, as you've introduced no new packages.

Probably package was flagged as vulnerable between your commits, that's why we've started seeing this message after certain push.

Copy link
Member

@maarcingebala maarcingebala left a comment

Choose a reason for hiding this comment

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

I've tested the PR and it seems fine, although I have some objections about inconsistent naming of the model fields related to publication date - here we use published_date while in other models we have available_on. I'd unify all of those fields to publication_date but that's a separate issue and I recommend to handle that separately.

@maarcingebala
Copy link
Member

maarcingebala commented Dec 18, 2018

@k-brk We've added the changelog file recently, would you mind adding a note describing your changes?

@maarcingebala maarcingebala changed the title Add publication date to collections #3125 Add publication date to collections Dec 18, 2018
@maarcingebala maarcingebala merged commit d615eb5 into saleor:master Dec 20, 2018
@maarcingebala
Copy link
Member

Congrats on your first contribution to Saleor! 🎉

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.

Add publication date to collections
4 participants