event duplication - wrapping up the issue

No problem Sean. I'm going to fix my module today to log the condition but not allow it to happen, then I'll reset all of my data and let it run another course. It may take another month to see the problem happen. I will let you know if my theory does not pan out in the end.

I think to shed light it will need to be run on a site that is visible to crawlers. Whether it is production data or not matters less, but the fundamental trigger of the problem is looking at calendars that are out near the end of the rendering range. This can of course be done by hand, by why not turn those crawlers, both good and evil, to good use?

Sean,

I think the patch is missing an important piece of the logic.

In my variant of eventrepeat I have a single node off which multiple event_projected_node row entries exist, all keyed back by the nid. In event repeat each repeat is a discrete node with its own nid and the repeat sequence is bound together by the rid value.

Within the date estimation function the following query is intended to bring back the last two dates of a series:

$result = db_query("SELECT event_end, event_start FROM event e_p_n WHERE e_p_n.nid = %d ORDER BY e_p_n.event_end DESC LIMIT 2", $nid);

This code in the the tarball I sent you works because the series of event_projected_node (e_p_n) rows is bound by the nid. As you have tweaked it in your patch the logic will only pull back a single node - the most recent one - and the logic in the rest of the function will be skipped because two dates off which to build the estimate could not e found in the result set.

I think you need to pass in the repeat identifer (rid) instead of a nid value (the rid is available at the point of call, so this is an easy change) and then run the query to join the event_repeat and event tables where event_repeat.rid == the rid you pass in. Then you will get the most recent two estimated dates.

I've got a debug screen in the eventproject module that I sent which shows the estimated dates for each node. I recommend that you bulid something similar in eventrepeat to ensure that your date estimation logic is working. Unless I am totally missing a nuance, I don't think your patch will ever generate an estimated date and thus won't catch/identify the problem.

Scott

Sean,

I'm still not convinced that the patch reflects the logic it needs to. The estimation function needs to take in a repeat id, not a node id.

I think that

$est_next_date = _eventproject_estimated_next_date($repeatpattern['nid'], ...

should use 'rid' as follows:

$est_next_date = _eventproject_estimated_next_date($repeatpattern['rid'], ...

And I'd change the name of that first parameter to be $rid in the call signature and the SQL within _eventproject_estimated_next_date() to be consistent.

Perhaps I'm missing something. If you tested the estimated next date function and got the results you were expecting with the patch then go ahead and ignore this.

I've continued testing out my theory and I'm pretty confident that this particular way to cause duplicated repeat events is addressed by the change.

Scott

Sean - In reading the patch I cannot help but think that the 'next date' function is going to have the very problem that is causing the repeats to happen. When the 'next date' is beyond the render period, it is going to return 0, which has a dual meaning. The original intended meaning by hunmonk was that the repeat sequence was to be closed. The accidental secondary meaning was that the repeat sequence just ran out of future dates in the 2000[default] day range the module uses. The calling function cannot distinguish between these two meanings

I know from the start that I used a sledgehammer for a carpet-tack sized problem, and perhaps you've gone one level deeper in understanding this than I have. The motivation behind estimating the next date based on prior data was because the problem was happening out at the leading edge of the calendar map data.

I don't think in the patch that this line
if($est_next_date > $render_support_end) {
will ever return true. Will it?

I too hope this wraps things up. I've not seen repeats in my eventrepeat variant for several weeks after applying similar logic. I do think this approach addresses this rather nasty duplicating problem.

Scott

sean,

No problem on the back and forth. If I were following the 'right' process I'd be using the stock module and sending you the explicit patch. I appreciate that you are willing to work with me even though I'm on a heavily modified code base.

The estimation function is far less exact than the rendered dates. It is taking a wild guess at the next repeat date based on the existing data. This is an attempt to catch the case where date generation runs off the end of the rendered dates table and errantly returned "no more dates to render". That condition prompts the code to restart the repeat (by setting last_rendered equal to end_date, which is frequently 0). That code was assuming that repeats would only end when they ran past the end_date and did not expect end_date to be 0. I believe the fundamental problem is that there is no data in the render table for the dates the code is attempting to render, and that's how events end up being reset to the beginning and create a series of duplicated events.

The snippet you quoted replaced the "set the last rendered date to the end_date (0)" with a stop-gap "set the last rendered date to now just to prevent another !@$!@$ batch of duplicated repeat events". That was for testing to allow the problem to happen without giving me a big mess to clean up. I'm sorry that was in the version I sent you - it is confusing. In my production version now it is back to setting that to end_date. With the estimation function preventing the code flow where the next date came back as "none" because we were at the end of the render range, I don't end up setting last_rendered to end_date(0) in the wrong places now, which is good.

You may have a cleaner fix that my date estimation route - I started this off noting that I was using a sledgehammer on a very nuanced problem. The main downside to the estimation function is that it may prevent events from being created right up near the end of the render range for which there actually is data. There is also a risk of it not estimating a date that is out far enough. For example, if somebody does a repeat that is configured for the second Tuesday of the month for January and February only (but every year), the estimator may not work out that there is a ten month gap that ie needs to leap over. Hence the fudge factor multiple in there - to better cover odd cases like this.

Scott

Sean,

Do you think this will work properly with events that do not have an end date, but instead have an end count? This is a condition where the sequence should be closed, but the end date will be 0.

I'm not so certain that this is a terrible thing - worst case there is a set of events that are no longer going to produce new repeated nodes, but are still 'open'.

When taking 'end after N event' repeats into account, this logic looks to be more sound than my date estimator. I've learned more about how the guts of eventrepeat were intended to work since crafting that solution.

Scott

Scott