Skip to content

Conversation

@prateekj117
Copy link
Member

Fixes #5996

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.

Short description of what this resolves:

Corrects 'days' attribute not found in scheduled_jobs.py

@iamareebjamal iamareebjamal changed the title Correct 'days' attribute not found in expire tickets fix: Correct 'days' attribute not found in expire tickets Jun 4, 2019
@auto-label auto-label bot added the fix label Jun 4, 2019
@iamareebjamal
Copy link
Member

@uds5501 @shreyanshdwivedi @mrsaicharan1 Please review

@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #5997 into development will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #5997   +/-   ##
============================================
  Coverage        66.28%   66.28%           
============================================
  Files              285      285           
  Lines            13992    13992           
============================================
  Hits              9274     9274           
  Misses            4718     4718
Impacted Files Coverage Δ
app/api/helpers/scheduled_jobs.py 22.34% <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 caff08d...4e53787. Read the comment docs.

with app.app_context():
db.session.query(Order).filter(Order.status == 'pending',
(datetime.datetime.today() - Order.created_at).days > 3).\
(Order.created_at + datetime.timedelta(days=3)) > datetime.datetime.now()).\
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain what this does here? @prateekj117

Copy link
Contributor

Choose a reason for hiding this comment

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

@prateekj117 just wanna ask, wouldn't order-today be actually negative? Also, please make it >= it will be a better choice

Copy link
Member Author

@prateekj117 prateekj117 Jun 4, 2019

Choose a reason for hiding this comment

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

@mrsaicharan1 the main problem was with the days attribute. I don't get it how your condition will solve it.
Refer this for more info on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, using .today() method will take time to be 12:00 of that particular day, we should use .now() instead.

Copy link
Member

Choose a reason for hiding this comment

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

Also, your condition seems to be wrong.
Suppose I placed an order yesterday. Won't it be expired?

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.python.org/3/library/datetime.html#timedelta-objects
This would be a better solution

datetime.datetime.now().day - Order.created_at.day > 3

Copy link
Member Author

Choose a reason for hiding this comment

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

@mrsaicharan1 I read on stackoverflow that there might be a case that python datetime isn't compatible with Sqlalchemy datetime object. So I implemented it this way which shows on stackoverflow that it works. Silly mistake on the opposite equality sign part, will correct it.

Copy link
Member

@mrsaicharan1 mrsaicharan1 Jun 4, 2019

Choose a reason for hiding this comment

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

yes @prateekj117, there might be cases , but we are comparing difference of days.Difference will never change. But anyways, if you change the sign, your fix works too.

@iamareebjamal iamareebjamal merged commit 485aa17 into fossasia:development Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No attribute 'days' error in expiring pending tickets after 3 days function

4 participants