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

Removed Dates and Update Status for Continuous Journal #1091

Merged
merged 5 commits into from
Mar 22, 2023

Conversation

shreyalpandit
Copy link
Collaborator

Addresses Issue #1090 so that the continuous journal does not have a value for all dates except start date.

@avishkar58
Copy link
Contributor

Hey @amritpurshotam would you mind reviewing this PR please?

Copy link
Collaborator

@amritpurshotam amritpurshotam left a comment

Choose a reason for hiding this comment

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

Nice work @shreyalpandit !. Just some small changes to clean up the Event construction and some optional changes for each of the @property fields. Otherwise looks good :)

api/app/events/api.py Outdated Show resolved Hide resolved
api/app/events/models.py Outdated Show resolved Hide resolved
api/app/events/models.py Outdated Show resolved Hide resolved
api/app/events/models.py Outdated Show resolved Hide resolved
Comment on lines +15 to +23
def check_open(open, close):
now = datetime.now()
if open and close:
return now >= open and now < close
elif open:
return now >= open
elif close:
return now < close
return True
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this, much much cleaner than what I suggested!

Copy link
Collaborator

@amritpurshotam amritpurshotam left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks for the updates!

@shreyalpandit shreyalpandit merged commit c4dd024 into develop Mar 22, 2023
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.

3 participants