Skip to content

/ django-oscar

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

Issue/1970/define ordering for models #3364

Merged
merged 1 commit into from Aug 1, 2020
Merged

Issue/1970/define ordering for models #3364

merged 1 commit into from Aug 1, 2020

Conversation

@SameeranB
Copy link

SameeranB commented May 15, 2020 •
edited by solarissmoke

Fixes #1970.

This adds ordering and database indexing to the following fields in their respective models.
# App Model Ordering Change
1 analytics AbstractUserProductView -date_created
2 analytics AbstractUserSearch -date_created
3 basket AbstractBasket -date_created
4 catalogue AbstractOption name
5 communication AbstractEmail -date_sent
6 communication AbstractCommunicationEventType name
7 customer AbstractProductAlert -date_created
8 offer AbstractRange name
9 order AbstractOrderNote -date_updated
10 order AbstractOrderDiscount pk
11 payment AbstractSource pk
12 payment AbstractSourceType name
13 voucher AbstractVoucherSet -date_created
14 voucher AbstractVoucher -date_created
15 voucher AbstractVoucherApplication -date_created
@codecov
Copy link

codecov bot commented May 15, 2020 •
edited

Codecov Report

Merging #3364 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3364      +/-   ##
==========================================
+ Coverage   85.13%   85.14%   +0.01%     
==========================================
  Files         289      289              
  Lines       15528    15542      +14     
==========================================
+ Hits        13219    13233      +14     
  Misses       2309     2309
Impacted Files Coverage Δ
src/oscar/apps/analytics/abstract_models.py 94.64% <100.00%> (+0.19%) ⬆️
src/oscar/apps/catalogue/abstract_models.py 91.58% <100.00%> (+0.01%) ⬆️
src/oscar/apps/communication/abstract_models.py 94.17% <100.00%> (+0.11%) ⬆️
src/oscar/apps/customer/abstract_models.py 81.45% <100.00%> (+0.15%) ⬆️
src/oscar/apps/offer/abstract_models.py 88.61% <100.00%> (+0.02%) ⬆️
src/oscar/apps/order/abstract_models.py 93.46% <100.00%> (+0.02%) ⬆️
src/oscar/apps/payment/abstract_models.py 89.55% <100.00%> (+0.15%) ⬆️
src/oscar/apps/voucher/abstract_models.py 93.24% <100.00%> (+0.13%) ⬆️
@solarissmoke
solarissmoke requested changes May 16, 2020
View changes
Copy link
Member

solarissmoke left a comment

Thanks for this. I gave some of the indexes more thought and feel that they will not provide a performance benefit, so have requested removal of a few. A few other minor changes also requested.
src/oscar/apps/customer/abstract_models.py Outdated Show resolved Hide resolved
src/oscar/apps/order/migrations/0009_auto_20200515_0813.py Outdated Show resolved Hide resolved
src/oscar/apps/analytics/abstract_models.py Outdated Show resolved Hide resolved
src/oscar/apps/basket/abstract_models.py Outdated Show resolved Hide resolved
src/oscar/apps/communication/abstract_models.py Outdated Show resolved Hide resolved
src/oscar/apps/order/abstract_models.py Outdated Show resolved Hide resolved
src/oscar/apps/voucher/abstract_models.py Outdated Show resolved Hide resolved
@solarissmoke
solarissmoke reviewed May 16, 2020
View changes
src/oscar/apps/basket/abstract_models.py Outdated Show resolved Hide resolved
@SameeranB SameeranB force-pushed the SameeranB:issue/1970/define_ordering_for_models branch from 122409a to 60a1d60 May 16, 2020
@SameeranB
Copy link
Author

SameeranB commented May 16, 2020 •
edited

@solarissmoke, I'm failing Travis checks but I don't think it's got anything to do with my commit. Here is one of the failed tests. Would you point me in the right direction on this?

Apologies, It was a problem on my end. I forgot to run make migrations
@solarissmoke
Copy link
Member

solarissmoke commented May 16, 2020

It is to do with your commit. You have changes for which there are migrations missing from the PR.
@SameeranB
Copy link
Author

SameeranB commented May 16, 2020

Yeah, I've pushed again with the missing migrations.

@solarissmoke, I'm failing Travis checks but I don't think it's got anything to do with my commit. Here is one of the failed tests. Would you point me in the right direction on this?

Apologies, It was a problem on my end. I forgot to run make migrations
@solarissmoke
solarissmoke requested changes Jun 6, 2020
View changes
requirements.txt Outdated Show resolved Hide resolved
src/oscar/apps/analytics/abstract_models.py Outdated Show resolved Hide resolved
src/oscar/apps/analytics/abstract_models.py Outdated Show resolved Hide resolved
@SameeranB SameeranB requested a review from solarissmoke Jun 16, 2020
@solarissmoke
Copy link
Member

solarissmoke commented Jun 20, 2020

Thanks for this. It'll be a couple of weeks before we can merge (want to get v2.1 out first).
pyup-bot @solarissmoke
Define default ordering on several models.
Loading status checks…
3a677f9
@solarissmoke solarissmoke force-pushed the SameeranB:issue/1970/define_ordering_for_models branch from 3c9a819 to 3a677f9 Aug 1, 2020
@solarissmoke
solarissmoke approved these changes Aug 1, 2020
View changes
@solarissmoke solarissmoke merged commit 01a658c into django-oscar:master Aug 1, 2020
4 checks passed
4 checks passed
codecov/changes No unexpected coverage changes found
Details
codecov/patch 100.00% of diff hit (target 85.13%)
Details
codecov/project 85.14% (+0.01%) compared to c33ce21
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@solarissmoke solarissmoke

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Define ordering for models
3 participants
@SameeranB @solarissmoke @pyup-bot
You can’t perform that action at this time.