Skip to content

fix: Move order_expiry_time into settings model #6083

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
Merged
merged 11 commits into from
Jul 1, 2019
Merged

fix: Move order_expiry_time into settings model #6083

merged 11 commits into from
Jul 1, 2019

Conversation

@kushthedude
Copy link
Member

@kushthedude kushthedude commented Jun 20, 2019

Fixes #6075

Changes proposed in this pull request:

  • Removes Order_Expiry_time from Event to Settings
  • Modified apib as according to changes .

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

@kushthedude
Copy link
Member Author

kushthedude commented Jun 20, 2019

@shreyanshdwivedi @uds5501 @mrsaicharan1 Please review, If I missed something?

@kushthedude
Copy link
Member Author

kushthedude commented Jun 21, 2019

@iamareebjamal Why is travis failing ?

@iamareebjamal
Copy link
Member

iamareebjamal commented Jun 21, 2019

https://travis-ci.org/fossasia/open-event-server/builds/548447247?utm_source=github_status&utm_medium=notification

iamareebjamal
iamareebjamal reviewed Jun 21, 2019
View reviewed changes
houndci-bot
houndci-bot reviewed Jun 21, 2019
View reviewed changes
@fossasia fossasia deleted a comment Jun 21, 2019
@fossasia fossasia deleted a comment Jun 21, 2019
houndci-bot
houndci-bot reviewed Jun 21, 2019
View reviewed changes
@fossasia fossasia deleted a comment Jun 21, 2019
houndci-bot
houndci-bot reviewed Jun 21, 2019
View reviewed changes
houndci-bot
houndci-bot reviewed Jun 21, 2019
View reviewed changes
houndci-bot
houndci-bot reviewed Jun 21, 2019
View reviewed changes
houndci-bot
houndci-bot reviewed Jun 21, 2019
View reviewed changes
@fossasia fossasia deleted a comment Jun 21, 2019
@fossasia fossasia deleted a comment Jun 21, 2019
@codecov
Copy link

codecov bot commented Jun 21, 2019 •
edited
Loading

Codecov Report

Merging #6083 into development will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development   #6083      +/-   ##
==============================================
+ Coverage         66.5%   66.5%   +<.01%     
==============================================
  Files              286     286              
  Lines            14272   14275       +3     
==============================================
+ Hits              9491    9494       +3     
  Misses            4781    4781
Impacted Files Coverage Δ
app/factories/event.py 100% <ø> (ø) ⬆️
app/api/schema/events.py 89.86% <ø> (-0.07%) ⬇️
app/models/event.py 81.78% <ø> (-0.15%) ⬇️
tests/all/integration/api/helpers/test_order.py 97.72% <100%> (+0.1%) ⬆️
app/models/setting.py 90.34% <100%> (+0.11%) ⬆️
app/api/schema/settings.py 100% <100%> (ø) ⬆️
app/api/helpers/order.py 40.9% <100%> (+1.84%) ⬆️

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 3ed0142...8d10769. Read the comment docs.

@kushthedude
Copy link
Member Author

kushthedude commented Jun 22, 2019

@iamareebjamal review please

iamareebjamal
iamareebjamal reviewed Jun 22, 2019
View reviewed changes
app/models/setting.py Outdated
# Static domain
static_domain = db.Column(db.String)
# Order Expiry Time
order_expiry_time = db.Column(db.Integer, default=15)
Copy link
Member

@iamareebjamal iamareebjamal Jun 22, 2019

Choose a reason for hiding this comment

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

Write # minutes in front of it

Copy link
Member

@shreyanshdwivedi shreyanshdwivedi Jun 25, 2019

Choose a reason for hiding this comment

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

@kushthedude I guess you missed to put a comment over here.

Copy link
Member Author

@kushthedude kushthedude Jun 26, 2019

Choose a reason for hiding this comment

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

@shreyanshdwivedi I have specified the minutes above the field, Writing minutes in front of field definition, Would fail hound

iamareebjamal
iamareebjamal reviewed Jun 22, 2019
View reviewed changes
app/models/setting.py Outdated

def __init__(self,
app_environment=Environment.PRODUCTION,
order_expiry_time=None,
Copy link
Member

@iamareebjamal iamareebjamal Jun 22, 2019

Choose a reason for hiding this comment

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

Add last

Copy link
Member Author

@kushthedude kushthedude Jun 22, 2019

Choose a reason for hiding this comment

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

What will the following do ?

Copy link
Member

@iamareebjamal iamareebjamal Jun 29, 2019

Choose a reason for hiding this comment

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

Still not added to last

Copy link
Member Author

@kushthedude kushthedude Jun 30, 2019

Choose a reason for hiding this comment

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

@iamareebjamal added to last

iamareebjamal
iamareebjamal reviewed Jun 22, 2019
View reviewed changes
iamareebjamal
iamareebjamal reviewed Jun 22, 2019
View reviewed changes
houndci-bot
houndci-bot reviewed Jun 22, 2019
View reviewed changes
@kushthedude
Copy link
Member Author

kushthedude commented Jun 25, 2019

@iamareebjamal @niranjan94 @CosmicCoder96 Please Review

shreyanshdwivedi
shreyanshdwivedi reviewed Jun 25, 2019
View reviewed changes
mrsaicharan1
mrsaicharan1 suggested changes Jun 26, 2019
View reviewed changes
@kushthedude
Copy link
Member Author

kushthedude commented Jun 26, 2019

@mrsaicharan1 @shreyanshdwivedi @iamareebjamal Please review

uds5501
uds5501 suggested changes Jun 26, 2019
View reviewed changes
@kushthedude
Copy link
Member Author

kushthedude commented Jun 28, 2019

@iamareebjamal Please Review

shreyanshdwivedi
shreyanshdwivedi reviewed Jun 29, 2019
View reviewed changes
Copy link
Member

@shreyanshdwivedi shreyanshdwivedi left a comment

Choose a reason for hiding this comment

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

@kushthedude there is still one hound violation. Please check

@kushthedude
Copy link
Member Author

kushthedude commented Jun 29, 2019 via email

@kushthedude
Copy link
Member Author

kushthedude commented Jun 30, 2019

@iamareebjamal @niranjan94 Please Review

@iamareebjamal iamareebjamal changed the title enh: Moving Order_Expiry_Time into Admin/Settings fix: Moving order_expiry_time into settings model Jun 30, 2019
@iamareebjamal iamareebjamal changed the title fix: Moving order_expiry_time into settings model fix: Move order_expiry_time into settings model Jun 30, 2019
@auto-label auto-label bot added the fix label Jun 30, 2019
@kushthedude
Copy link
Member Author

kushthedude commented Jul 1, 2019

@CosmicCoder96 @niranjan94 please review

@iamareebjamal iamareebjamal merged commit 9f47412 into fossasia:development Jul 1, 2019
@iamareebjamal iamareebjamal mentioned this pull request Jul 11, 2019
Merged
iamareebjamal pushed a commit to iamareebjamal/open-event-server that referenced this pull request Aug 2, 2019
@kushthedude @iamareebjamal
fix: Move order_expiry_time into settings model (fossasia#6083)
f2078b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iamareebjamal iamareebjamal iamareebjamal approved these changes

+4 more reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@shreyanshdwivedi shreyanshdwivedi shreyanshdwivedi left review comments

@uds5501 uds5501 uds5501 requested changes

@mrsaicharan1 mrsaicharan1 mrsaicharan1 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove Registration Time Limit from Event Model and Add it to Settings Model

7 participants

@kushthedude @iamareebjamal @houndci-bot @uds5501 @mrsaicharan1 @shreyanshdwivedi @mariobehling