Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow displaying all events descending from a category #4982

Closed
OmeGak opened this issue Jul 1, 2021 · 6 comments · Fixed by #4983
Closed

Allow displaying all events descending from a category #4982

OmeGak opened this issue Jul 1, 2021 · 6 comments · Fixed by #4983

Comments

@OmeGak
Copy link
Member

OmeGak commented Jul 1, 2021

In the category display view, rather than listing only the events that are directly linked to the category, it could be possible to list all the events descending to the category (i.e. events linked to subcategories).

This is a feature that was implemented on the UN plugin via monkeypatching. As we are consolidating the plugin's codebase and and relying more on signals it's becoming clear: 1) that this feature requires a non-negligible number of signals to be properly implemented on the plugin side and 2) that this feature is of general usefulness to all Indico instances.

UI proposal

The flat list of events could be triggered by:

  • URL parameter (e.g. ?flat=1, ?deep=1)
  • Link under the View button (eye) in category pages (e.g. All descending events).
view trigger
image image

Performance considerations

  • The implementation relies on Category.deep_children_query to fetch all subcategories. Is this something that may cause problems when done from the Home category, for instance?
  • In large Indico instances, displaying ALL the current events descending from the Home category can produce an awfully long list. Is this something that should be limited to X number of events? Could adding some form of pagination (besides showing past and future events) relieve the strain client-side?
@ThiefMaster
Copy link
Member

The implementation relies on Category.deep_children_query to fetch all subcategories. Is this something that may cause problems when done from the Home category, for instance?

You can filter using Event.category_chain_overlaps(); no need to get the individual subcategories.

In large Indico instances, displaying ALL the current events descending from the Home category can produce an awfully long list. Is this something that should be limited to X number of events? Could adding some form of pagination (besides showing past and future events) relieve the strain client-side?

This is indeed the main problem with this a feature - I don't have a good idea so let's see if anyone else does (pinging @indico/development-team). Worst case would be an indico.conf option to disable this feature on larger instances. Or maybe only allowing it if there are max n levels of nesting below the category? Or max n events in there?

@pferreir
Copy link
Member

pferreir commented Jul 1, 2021

Sounds interesting as an advanced option, but it reminds me a bit of the old days when we had a "site map" or whatever it was which just dumped the whole event DB into a page and painstakingly rendered it.
TBH, I would avoid implementing this as a core feature if possible. The simple fact that we need "stopping criteria" is already a bit of a red flag...

@OmeGak
Copy link
Member Author

OmeGak commented Jul 1, 2021

I share the concern of just dumping the entire event DB on a page. This would not be the case for the category display view. Even for the home category, the only events that would be displayed are between past_threshold and future_threshold (one month before/after the current date) and only those that the current user has access to.

How big could this number be in large Indico instances? Even if we are talking of an order of 10K current events, this would not pose a serious load on the backend. It may take a while to load client-side, but I think it's a reasonable tradeoff in exchange of being able to see at glance (and ctrl+f!) all descendent events.

Realistically, advanced users would use this feature in subcategories and not in the Home category, reducing this number greatly. If we really want to prevent it, the feature could be disabled for the Home category. If the feature becomes more widely used and client-side performance an issue, it would be possible to add pagination like it's happening already for past and future events with RHEventList.

@ThiefMaster
Copy link
Member

For the sake of large instances I gave it a try on my dev instance w/ hundred thousands of events, and the performance was actually much better than I feared: [14 queries (0.578003s) | 3.021247s]

Another concern is that we do not perform access checks on individual events in the event list; by not having access to a category you simply cannot get its event list. However, the "overview" and "calendar" views of categories also descend into subcategories, and just rely on the "Visibility" setting of categories/events to avoid showing events from too deep in the tree in places where they shouldn't be.

However, I have the feeling people usually don't really configure the visibility properly... I'm pretty sure many categories use the default "everywhere" not realizing that this exposes event titles even though their categories are otherwise protected.

@plourenco
Copy link
Member

I would still be reluctant about just leaving the performance up to the backend. Even though pagination doesn't sound like a bad idea, I'm wondering how well would something like a lazy-loaded list that yielded per X times as big as the window we would want to have and loaded more on scroll look like?

@OmeGak
Copy link
Member Author

OmeGak commented Jul 6, 2021

As per @ThiefMaster's suggestion, I added in the PR (#4983) a Category.is_flat_view_enabled setting that is False by default. This solves all performance concerns for big instances and allows smaller instances to have it enabled by default in all categories via plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants