Ical processing: first event in event list ignored

[andiempettJISC]

relates to work done for #384 and #443
in the ical util, function get_events_from_string_ical(ical_data, limit=0) has, what i make out as superfluous code added to it using a counter count.
the behaviour of this function is changed to add a limit, which is a great idea but the counter being used to define how many events are used from the ical stops the zeroth event in the events list being picked up.
this becomes a problem when using the checkrepo plugin.

i propose a pull request changing from:

count = 0

def get_events_from_string_ical(ical_data, limit=0):
    global count
    # See https://github.com/collective/icalendar#api-change
    f = getattr(Calendar, 'from_ical', getattr(Calendar, 'from_string', None))
    cal = f(ical_data)
    if limit > 0:
	events = cal.walk('vevent')[count:limit+count]
	for event in events:
	    if event['DTSTART'].dt.replace(tzinfo=None) < datetime.utcnow():
	        count += 1
	if count > 0:
	    events = cal.walk('vevent')[count:limit+count]
    else:
	events = cal.walk('vevent')
    return events

to:

def get_events_from_string_ical(ical_data, limit=0):
    # See https://github.com/collective/icalendar#api-change
    f = getattr(Calendar, 'from_ical', getattr(Calendar, 'from_string', None))
    cal = f(ical_data)
    if limit > 0:
	events = cal.walk('vevent')[0:limit]
	print events
    else:
	events = cal.walk('vevent')
    return events

let me know if i'm missing something.
thanks
-andy

[Alfro]

@androidwiltron I think I know what the issue is. It's not simply that the zeroth element is not being picked up.

I think the intention behind the changes added in #384 was to prevent the get_events_from_string_ical function from returning events that are already over. However, since the function compares the current date with the beginning date of the event, an ongoing event will be filtered out.

[Alfro]

Reverting the changes would fix that, but it would reintroduce the #384 issue.

[andiempettJISC]

Thanks for taking a look. No let's not revert the code, I'm sure there's a better way to fix both, I'm working on one now

…

On Mon, 12 Dec 2016, 09:51 Alfonso Rodríguez Pereira, < ***@***.***> wrote: Reverting the changes would fix that, but it would reintroduce the #384 <#384> issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#451 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFexPJfXo1lbdMQiBqCEsLk65H8LXyrmks5rHRkogaJpZM4LI3vW> .

[andiempettJISC]

hi

how about this:

count = 0

def get_events_from_string_ical(ical_data, limit=0):
    global count
    # See https://github.com/collective/icalendar#api-change
    f = getattr(Calendar, 'from_ical', getattr(Calendar, 'from_string', None))
    cal = f(ical_data)
    if limit > 0:
        events = cal.walk('vevent')[count:limit + count]
        for event in events:
            if event['DTSTART'].dt.replace(tzinfo=None) < datetime.utcnow():
                count += 1
        count -= 1
        if count > 0:
            events = cal.walk('vevent')[count:limit + count]
    else:
        events = cal.walk('vevent')
    return events

all i've added is count -=1 this makes sure the correct range is passed to the cal.walk.

does this look right?
-andy

[Alfro]

That would return the correct range when there is an ongoing event, but an extra finished event when not.
Instead, I would change the following if statement:

if event['DTSTART'].dt.replace(tzinfo=None) < datetime.utcnow():
    count += 1

to check against the END time of the event before skipping it instead of the start, this way ongoing events will still be listed.

I don't remember the event data structure by heart, but if there is no "DTEND", it could be computed adding the "DTSTART" and the "DURATION" of the event.

[andiempettJISC]

Cool, thanks I'll test this out and have a look. By the way, how do you test an iCal file from the Harvard scheduler tool. When I use one it gets overwritten by GC getting a new iCal file?

…

On Fri, 6 Jan 2017, 12:20 Alfonso Rodríguez Pereira, < ***@***.***> wrote: That would return the correct range when there is an ongoing event, but an extra finished event when not. Instead, I would change the following if statement: if event['DTSTART'].dt.replace(tzinfo=None) < datetime.utcnow(): count += 1 to check against the END time of the event before skipping it instead of the start, this way ongoing events will still be listed. I don't remember the event data structure by heart, but if there is no "DTEND", it could be computing adding the "DTSTART" and the "DURATION" of the event. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#451 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFexPDE86qSLa_TITYKcxoFta6ZdW8j4ks5rPjGkgaJpZM4LI3vW> .

[Alfro]

@androidwiltron Hi! Could you take a look at this?

[andiempettJISC]

this is the best i've gotten so far. I think this ensures the correct count is returned to be used filtering cal.walk('vevent')

count = 0

def get_events_from_string_ical(ical_data, limit=0):
    global count
    # See https://github.com/collective/icalendar#api-change
    f = getattr(Calendar, 'from_ical', getattr(Calendar, 'from_string', None))
    cal = f(ical_data)
    if limit > 0:
        events = cal.walk('vevent')[count:limit + count]
        for event in events:
            if event['DTSTART'].dt.replace(tzinfo=None) < datetime.utcnow():
                count += 1
        count -= 1
        if count > 0:
            events = cal.walk('vevent')[count:limit + count]
    else:
        events = cal.walk('vevent')
    # reset the counter
    count = 0
    return events

it also resets the count to stop the count persisting between long heartbeats

[smarquard]

Why not just declare count inside the function and set it to 0 at the top, as it's only used in that function?

Also I don't understand the count -= 1 in the @androidwiltron code snippet above?

[smarquard]

The logic of this seems to be flawed, because Opencast (certainly in 4.x) can return in the iCal feed:

• events that have passed
• events in any order

So you can have an ical with:

new event
old event

(rather than the expected old event, new event).

In this case, the filtering will set count=1 and then get the old event and not the new event. You could also have old events in the middle of a list.

So unless Opencast changes to return events strictly in order (and preferably exclude old events), there's no safe way to restrict to a limit.

[smarquard]

Related #569

[Alfro]

Doing some clean-up on issues. This issue was not updated, but I did a major refactor of this logic on the 2.1.x branch. It now loops over all events, filter past events out and sort them, then return all of them, or a number of them, if there's a limit set.

I think this issue should be fixed now, but please feel free to reopen if there are more things I might have missed.

[mliradelc]

Hi Alfonso, Here at Uni Köln we still have problems with the Galicaster Scheduling. We found this problem only occurs "sometimes" so it's very difficult to track it.

We think that this is happening when the event is modified.