search

plone / plone.app.standardtiles

Plone Standard tiles (reflecting viewlets et al) to be used with Plone Mosaic
Other
7 stars 12 forks source link

Expand recurring events #125

Closed petschki closed 1 year ago

petschki commented 2 years ago

fixes #124

ksuess commented 2 years ago

Looks really good. I am interested. Can I review the draft? A local test yesterday was ok for Plone 6 / Python 3.9. Couldn't reproduce the error mentioned in ci test.

petschki commented 2 years ago

Yes, please test it!

petschki commented 2 years ago

Though the start filter shouldn't default to now() since one likes to view older events too. And if an end is in the query it should be used too.

petschki commented 2 years ago

@ksuess instead of duplicating the logic of plone.app.event.browser.event_listing here I suggest to rethink the strategy now. I have a productive site with this branch and since our changes the results gets worse ... the customer wants to display recurring events which have their end in the future and we do not reflect that right now. I'd like to refactor the whole expand_events boolean to something like use_event_listing_results and simply lookup the @@event_listing BrowserView with the whole start-end expanding logic already done ...

ksuess commented 2 years ago

@petschki that sounds like a really good plan: support start and end date range.

While refactoring plone.app.event.base.expand_events: Maybe it's worth thinking about supporting RET_MODE_BRAINS. This would allow using custom tile templates as they are, untouched, expecting brains, not objects.

petschki commented 2 years ago

Sure but this is out of scope of this package/PR ... I'd suggest to make a feature issue in plone.app.event

petschki commented 2 years ago

I've refactored this and it works good so far. But there's a bug in plone.app.event which I have to investigate further ... see https://github.com/plone/plone.app.event/issues/347

ksuess commented 2 years ago

I would like to see the limit to be applied.

The use case "show events with end date in the next 8 days" fails with displaying too many events back before today. Same use case fails also by displaying "duplicates": event and occurence with same date.

BTW this implementation lacks a rewriting of plone.app.event: _expand_events_start_end needs to generate a range for start and end both. And expand_events needs to regard ranges for both instead of dates.

petschki commented 2 years ago

I think we can move the discussion of event listing specific issues to plone.app.event issue ... when it gets fixed there its fixed here too.