Added start- and enddates for kiosk items

[krestenlaust]

Resolves #288

• I'm not sure how to format django filters properly ran black
• Change requires migrations to database (I think), unsure about whether start_datetime should be nullable as well

I specifically chose datetime instead of date because time of day might be relevant. Maybe change uploaded_date to datetime as well.

[falkecarlsen]

Please make sure that timezones are taken into account with these. AFAIR, we run and present UTC.
If it turns out that we are using only UTC already, continue that.

[falkecarlsen]

Suggested change

	def is_active_period(self, current_time) -> bool:
	return (self.start_datetime < current_time) and (self.end_datetime is None or self.end_datetime > current_time)
	def is_active_period(self) -> bool:
	now = timezone.now()
	return (self.start_datetime <= now) and (self.end_datetime is None or self.end_datetime >= now)

No need to pass time to filter as arg. May be @property as well

[krestenlaust]

My thought process was that it could be used to introduce a mockup time in case of a test, but there's probably not much use for that.

[falkecarlsen]

Look at our usage of freezegun for mocking time in tests:

stregsystemet/stregsystem/tests.py

Line 823 in 9748ebe

def test_promille_staggered_male(self):

Not needed for this feature as the queries are so simple.

[falkecarlsen]

Django doesn't support passing properties to the ORM filter. You could consider using the filter made in kiosk/models is_active_period() to filter all KioskItems:

active_items = random.shuffle([x for x in KioskItem.objects if x.is_active_period()])
if active_items.len > 0:
    return active_items[0]
else:
    raise Http404("No active kiosk items found") 

But as I'm writing this suggestion, I realise this isn't better. In that case, consider removing the unused is_active_period()

[falkecarlsen]

Kiosk entries are typically timeframe-bound information, either internal events or sponsorships from industry which we sell for a period of time. Previously, we've forgot to stop sponsorships because nobody knew whether we were still getting paid for it and similarly, internal events has been advertised long after the event has stopped.

We might want to explicitly disallow not setting an end-time, such that this issue is mitigated. Add a tooltip for the admin interface to the tune of "Set end time to day after event".

I suggest requiring end-time, and allow blank start-time, but setting timezone.now() as default if unset.

[VirtualSatai]

Cool addition to the kiosk I didn't think of when I implemented it 👍

I dislike putting too many restrictions on the usage of the kiosk, I think it's better to trust the TREO / forstyrelse to admin it. I think an unbounded endtime is fine, otherwise you'd just have to put in a stupidly late time for the generic items (like the "sign up for the f-klub"-ad). Defaulting start-time to timezone.now() is a fine choice if you really want to disallow null.

[krestenlaust]

I agree. As far as I understand, it's not uncommon to have indefinite kiosk entries. There could also be a case where an end date is unknown at upload time, having a nullable entry also communicates a missing end date in case of null.

The current PR implementation makes both date fields nullable. The advantage I see, is it leaves all existing entries untouched. I believe the semantic value of nullable fields is worth more, than avoiding null for the sake of avoiding it.

Furthermore, the reason I don't find a default value compelling, is that the field 'created at' already exists.

Though, I haven't experienced the kiosk in action, and I haven't used it as an admin either. (Only locally)

[falkecarlsen]

I'm fine with either approach, my argument is to force thoughtfulness when picking end_date. Leaving it blank is a much easier error to make, than setting date to >=anno 2040.

My hope is that this could mitigate entries running for longer than they're meant to. I don't subscribe to the:

There could also be a case where an end date is unknown at upload time

comment as an argument for allowing nullable end_date as it allows admins to make temporary settings for kiosk entries - and as we especially know in stregsystemet - there is nothing as permanent as a temporary solution.

The above comments are nitpicky, we should just decide on one of the two approaches

[codecov-commenter]

Codecov Report

Merging #342 (e83044b) into next (10a11e4) will increase coverage by 0.03%.
The diff coverage is 70.00%.

❗ Current head e83044b differs from pull request most recent head 006b926. Consider uploading reports for the commit 006b926 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             next     #342      +/-   ##
==========================================
+ Coverage   80.63%   80.66%   +0.03%     
==========================================
  Files          32       32              
  Lines        2814     2819       +5     
  Branches      217      217              
==========================================
+ Hits         2269     2274       +5     
  Misses        510      510              
  Partials       35       35              

Files Changed	Coverage Δ
kiosk/views.py	38.46% <40.00%> (+5.12%)	⬆️
kiosk/admin.py	86.66% <100.00%> (ø)
kiosk/models.py	89.47% <100.00%> (+1.97%)	⬆️

[falkecarlsen]