Fix issue 375

[CianciuStyles]

This PR is for a possible fix for the isse #375.

I chose to implement a filter for removing the ending day under some circumstances, so that if the events already present in the database are setup correctly (especially the all_day property) no models should be touched at all.

[berkerpeksag]

Thank you! Your approach looks good to me, but using a template filter might be problematic in the future (e.g. someone may forget to use it) Can't we modify dt_start and dt_end properties instead if all_day is True? That way we will also avoid moving more logic to Django templates.

[CianciuStyles]

That was my idea initially, but I went this road instead to avoid a migration for the events which have been already parsed and stored in the database. How would you modify dt_start and dt_end to achieve the same result?

Another way to possibly make the change a little bit more future-proof is to extract the part of the template dealing with the event start and end times as a different partial template (similar to the time_tag one), so that we should only include it wherever we need it (obviously, this won't address your comment on moving more logic to the templates).

Let me know what you think about this.

[berkerpeksag]

I was thinking moving the logic in templates and templatetag into the properties in the models, for example:

def dt_end(self):
    ...
    if self.all_day:
        return recurrence - timedelta(days=1)
    ...

Then we would do the following in the templates:

{% if next_time.all_day %}
    {{ next_time.dt_end|date:"d N" }}
{% else %}
    {{ next_time.dt_end|date:"d N" }} at {{ next_time.dt_end|date:"fA"|lower }} {{ next_time.dt_end|date:"e" }}
{% endif %}

I don't mind running a data migration if needed. Currently, there 601 events in the database and a data migration would run quickly without any problem.

[berkerpeksag]

Also, looking at this again, perhaps we should fix the importer instead. We probably need to do the dt - timedelta(days=1) step here if all_day returns True:

pythondotorg/events/importer.py

Lines 28 to 47 in fea2f39

	def import_occurrence(self, event, event_data):
	# Django will already convert to datetime by setting the time to 0:00,
	# but won't add any timezone information. We will convert them to
	# aware datetime objects manually.
	dt_start = convert_dt_to_aware(event_data['DTSTART'].dt)
	dt_end = convert_dt_to_aware(event_data['DTEND'].dt)
	# Let's mark those occurrences as 'all-day'.
	all_day = (
	dt_start.resolution == DATE_RESOLUTION or
	dt_end.resolution == DATE_RESOLUTION
	)
	defaults = {
	'dt_start': dt_start,
	'dt_end': dt_end,
	'all_day': all_day
	}
	self.create_or_update_model(OccurringRule, event=event, defaults=defaults)

[CianciuStyles]

If we fix the importer we should also run a migration for applying the same logic to the already existing events in the database (subtract timedelta(days=1) from dt_end for all the events for which all_day is True), as dt_end on OccurringRule is a field, not a property).

Other than that, this sounds sensible to me. I'll try and change the code according to what we have discussed here.

[berkerpeksag]

Could you also add a test for #560?

BEGIN:VEVENT
DTSTART;VALUE=DATE:20150328
DTEND;VALUE=DATE:20150330
DTSTAMP:20150202T092425Z

Unless I'm missing something, this will also fix #560.

[berkerpeksag]

Superseded by #1167.