Remove limit from axios call that retrieves groups

[lephisto]

Fixes #24547

Signed-off-by: Mephisto mephisto@mephis.to

[kesselb]

/compile amend /

[lephisto]

Will this PR make it?

[nickvergessen]

This is a bad idea and will bring down servers with a lot of groups.
Why can't you type ahead to search?

[lephisto]

This is not the Issue. Please read it. The Comboboxes will stay empty for existing workflows, if the groups are not within the first 20.

[nickvergessen]

Then this needs fixing. Loading potentially 2k or more groups which breaks the UI completely is not the option

[lephisto]

Limiting breaks the UI as well. How could this be fixed? It could be limited if the actually selected group in a workflowcondition would be guaranteed to be part of the result from the axios call. This would guarantee that the combo wouldn't be empty..

Is having 2k groups really a realistic case? Seems to be rather theoretical to me.

[rullzer]

Limiting breaks the UI as well. How could this be fixed? It could be limited if the actually selected group in a workflowcondition would be guaranteed to be part of the result from the axios call. This would guarantee that the combo wouldn't be empty..

Is having 2k groups really a realistic case? Seems to be rather theoretical to me.

There are several customer running with far more than 2k groups ;)

[juliusknorr]

Showing not all groups is expected here, since as already mentioned this would be quite an issue with larger amount of groups. However I do not see any issue with the original implementation as you can still get to groups that are not displayed by searching for them.

[kesselb]

However I do not see any issue with the original implementation as you can still get to groups that are not displayed by searching for them.

It's not confusing if one add a group to a workflow and after a page reload the group is not shown anymore? 🤔

[juliusknorr]

Ah, then I misunderstood the issue. In that case we most likely fail because we don't have a group match (with a displayname) in

server/apps/workflowengine/src/components/Checks/RequestUserGroup.vue

Line 69 in f9b4084

return this.groups.find(group => group.id === this.value) || null

I'm not sure how to best solve this other than doing a request for each currently set value yet, but that might of course also cause a high amount of requests for each rule that has a different group set.

[juliusknorr]

I'm not sure how to best solve this other than doing a request for each currently set value yet, but that might of course also cause a high amount of requests for each rule that has a different group set.

It actually seems to be the case already, so maybe @lephisto can clarify a bit on the original issue.

[ClCfe]

you have two ways to fix this

1. simple but not the cleanest
   adding a limit parameter to the method searchAsync(searchQuery, limit=20)
   if limit is null, no limit parameter should be added to the axios query

if (this.groups.length === 0) {
	await this.searchAsync('', null)
}

=> so it will retrieve all groups ONLY at initialization

2. cleanest but more complex
   you pass to the axios query an array of groups IDs (as a new parameter values[]) already involved in the rules of the edited workflow, but this implies changes on the backend

[lephisto]

I'm not sure how to best solve this other than doing a request for each currently set value yet, but that might of course also cause a high amount of requests for each rule that has a different group set.

It actually seems to be the case already, so maybe @lephisto can clarify a bit on the original issue.

No this is not the case. The current behaviour is, that if you load an existing workflow and there is one group assignment which isn't coincidentally in the first 20 results of the result-limited query, the Dropdown just stays empty, which is confusing.

This is why i formulated the PR to not limit it at all, but I get that it might result in an uncool behaviour if ppl use groups excessively.. As proposed earlier, one idea might be to pass the currently assigned groups to the axios call, to guarantee that they're part of the resultset thus avoiding the confusion. But this exceeds my Nextcloud Coding style awareness to not make it totally ugly. :)

[nickvergessen]

Closing again. This is not the right way. The required data needs to be loaded manually or with initial state on the settings module. But removing the limit and returning all groups is not an option.