Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix(workflowengine): fix group not shown in Group membership check
this might have occured on instances with
- more than twenty groups, and
- on rules with more than one Group membership checks
- and at least one of them being not in the set of the first 20 groups

Signed-off-by: Arthur Schiwon <[email protected]>
  • Loading branch information
blizzz committed May 5, 2025
commit c2dea5faf65dca0f6775ed6d73661eb9738eaf0b
37 changes: 36 additions & 1 deletion apps/workflowengine/src/components/Checks/RequestUserGroup.vue
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import axios from '@nextcloud/axios'
import NcSelect from '@nextcloud/vue/components/NcSelect'

const groups = []
const wantedGroups = []
const status = {
isLoading: false,
}
Expand All @@ -49,6 +50,7 @@ export default {
return {
groups,
status,
wantedGroups,
newValue: '',
}
},
Expand Down Expand Up @@ -82,6 +84,13 @@ export default {

searchAsync(searchQuery) {
if (this.status.isLoading) {
if (searchQuery) {
// The first 20 groups are loaded up front (indicated by an
// empty searchQuery parameter), afterwards we may load
// groups that have not been fetched yet, but are used
// in existing rules.
this.enqueueWantedGroup(searchQuery)
}
return
}

Expand All @@ -94,11 +103,15 @@ export default {
})
})
this.status.isLoading = false
this.findGroupByQueue()
}, (error) => {
console.error('Error while loading group list', error.response)
})
},
updateInternalValue() {
async updateInternalValue() {
if (!this.newValue) {
await this.searchAsync(this.modelValue)
}
this.newValue = this.modelValue
},
addGroup(group) {
Expand All @@ -107,10 +120,32 @@ export default {
this.groups.push(group)
}
},
hasGroup(group) {
const index = this.groups.findIndex((item) => item.id === group)
return index > -1
},
update(value) {
this.newValue = value.id
this.$emit('update:model-value', this.newValue)
},
enqueueWantedGroup(expectedGroupId) {
const index = this.wantedGroups.findIndex((groupId) => groupId === expectedGroupId)
if (index === -1) {
this.wantedGroups.push(expectedGroupId)
}
},
async findGroupByQueue() {
let nextQuery
do {
nextQuery = this.wantedGroups.shift()
if (this.hasGroup(nextQuery)) {
nextQuery = undefined
}
} while (!nextQuery && this.wantedGroups.length > 0)
if (nextQuery) {
await this.searchAsync(nextQuery)
}
Comment on lines +138 to +147
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easier to read.

Suggested change
let nextQuery
do {
nextQuery = this.wantedGroups.shift()
if (this.hasGroup(nextQuery)) {
nextQuery = undefined
}
} while (!nextQuery && this.wantedGroups.length > 0)
if (nextQuery) {
await this.searchAsync(nextQuery)
}
while (this.wantedGroups.length > 0) {
const groupId = this.wantedGroups.shift()
if (this.hasGroup(groupId)) {
continue
}
await this.searchAsync(groupId)
return
}

Also, the current implementation would only search 1 wanted group, I assuming this intended as this.findGroupByQueue() again called at the end of search

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intentionally done as a queue in order to not DoS the server with group search requests, but have them coming sequentially.

},
},
}
</script>
Expand Down