-
Notifications
You must be signed in to change notification settings - Fork 37
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 support for scheduled Timecard Notes #1062
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1062 +/- ##
==========================================
+ Coverage 88.37% 88.47% +0.09%
==========================================
Files 41 41
Lines 1781 1796 +15
==========================================
+ Hits 1574 1589 +15
Misses 207 207
Continue to review full report at Codecov.
|
@@ -314,6 +316,10 @@ class TimecardNoteManager(models.Manager): | |||
def enabled(self): | |||
return super(TimecardNoteManager, self).get_queryset().filter(enabled=True) | |||
|
|||
def active(self): | |||
now = timezone.now() | |||
return super(TimecardNoteManager, self).get_queryset().filter(Q(enabled=True) | Q(display_period_start__lte=now, display_period_end__gt=now)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this blow up if start and end were left blank?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my testing and understanding, we should be okay here. The field lookups (lt
, gt
, etc) resolve to false in cases where the value is null
blank=True, | ||
help_text='The start date for displaying this note. Note: manually enabling will override this setting.' | ||
) | ||
display_period_end = models.DateField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you wanted to start a note on May 1 and leave it up for an undetermined amount of time? How would that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right - this implementation doesn't support that, as it requires you to specify either both the start and end date, or neither. It would be pretty straightforward to modify this to support that use case if that's something we want to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose one could just put in a far-future end date and call it good, too.
Co-Authored-By: Carter Baxter <32077682+tbaxter-18f@users.noreply.github.com>
Thanks @timoballard! I think we missed a migration here, can you double check and submit a follow-up PR if necessary? |
Good catch @Jkrzy - I sure did. I overlooked that @tbaxter-18f's suggested change on the help text would imply a migration. |
Description
Adds support for scheduling Timecard Notes so that they display during a specified date range.
#967
Additional information
Enabling
a Timecard Note will cause it to display even if the scheduled date range is not active.