fix: show all personal spaces in filter on multi-personal (Kiteworks) accounts#4922
fix: show all personal spaces in filter on multi-personal (Kiteworks) accounts#4922mvanhorn wants to merge 1 commit into
Conversation
|
|
|
ey @mvanhorn, thanks a lot for you contribution!! let's take a first look!
If you didn't do that yet, please check our CONTRIBUTING file where you'll find the contribution guidelines. Once the PR is in shape, it will be code-reviewed first, and then tested. Keep tuned in case the team gives you feedback, improvements or fixes related with your code!! Anything you need, please don't hesitate to ping us. Welcome! |
dj4oC
left a comment
There was a problem hiding this comment.
Fixes #4674: on multi-personal (Kiteworks) accounts, the spaces search filter now matches all personal spaces by name instead of excluding them and force-pinning only the first one.
Findings
- Security: no blocking findings.
uiState.spacesis already scoped to the active account at the repository/use-case layer (accountName-filtered queries) andisMultiPersonalis that same account's capability flag, so this diff only changes which of the account's own already-fetched spaces pass the client-side name filter — no cross-tenant exposure (checked against A01:2021/CWE-284). One pre-existing, untouched-by-this-diff nit: the!isMultiPersonalbranch'spersonalSpace?.let { add(0, personalSpace) }re-adds the personal space without re-checkingshouldShowDisabledSpace, so a disabled personal space could still surface there — worth a separate follow-up ticket, not a blocker here. - Stability: no blocking findings. No sync-adapter, threading, or coroutine-scope changes;
collectLatestLifecycleFlowusage is unchanged andisMultiPersonalis guaranteed initialized (via a blocking call inonViewCreated) before this collector ever runs, so there's no race window. Separately noted:CapabilityViewModel.checkMultiPersonal()blocks the main thread viarunBlocking— pre-existing pattern shared with other fragments, out of scope for this PR, but worth a follow-up ANR-risk ticket. - Performance: no blocking findings. Pure in-memory list filtering on an already-loaded list;
SpacesListAdapter.setDatausesDiffUtil, so no RecyclerView regression. Two non-blocking nits:it.name.lowercase()is recomputed per space per keystroke (could be precomputed once when spaces load), and the untouched!isMultiPersonalbranch still does an extra list copy viatoMutableList()to prepend the personal space — both pre-existing or negligible at current space-list sizes, not blocking. - Test coverage: 0% on the touched branch — below the required 85% gate. No test in the repo references
SpacesListFragment's filter logic,searchFilter, orisMultiPersonalbranching (verified via GitHub code search, not the PR's own manual-QA claims). See the inline suggestion: extracting the filter into a pureSpacesFilter.filterSpaces(...)function (out of the Fragment's flow collector) makes it directly unit-testable with this codebase's existing JUnit4+MockK ViewModel-test pattern, and doubles as the fix for the underlying MVVM concern (filtering is business logic that shouldn't live inline in a Fragment). A ready-to-use 4-case JUnit test is included inline. - Compliance: CLA/commit-signing gaps were already flagged by @jesmrec in the human review thread — not duplicated here. No WCAG-relevant UI change (no new views/content descriptions). No dependency manifest touched. No TODO/FIXME/HACK/XXX introduced. CI is fully green (6/6 checks).
Verdict
Changes requested — coverage gap is the only gating issue; everything else above is non-blocking/pre-existing and offered as follow-up context.
🤖 Automated review by Claude Code (security · stability · performance · coverage)
Generated by Claude Code
| } | ||
| } | ||
| } | ||
| showOrHideEmptyView(spacesToListFiltered) |
There was a problem hiding this comment.
Test coverage: 0% on this branch — below the 85% gate. I searched the repo (no local checkout, used GitHub code search) and confirmed there is no SpacesListFragmentTest, no SpacesListViewModelTest, and no reference to searchFilter/isMultiPersonal anywhere in src/test or src/androidTest. The PR description only lists manual QA steps.
Root cause: the new isMultiPersonal branch (and the personal-space pinning it now skips) lives inline inside a Fragment lambda, and this codebase has no precedent for unit-testing Fragments directly (Robolectric isn't wired into owncloudApp; the existing pattern is plain JUnit4 + MockK against ViewModels, see owncloudApp/src/test/java/.../viewmodels/*ViewModelTest.kt).
Suggested fix — extract to a pure, testable function so this exact branch can be unit tested without Robolectric:
object SpacesFilter {
fun filterSpaces(
spaces: List<OCSpace>,
searchFilter: String,
isMultiPersonal: Boolean,
shouldShowDisabledSpace: (OCSpace) -> Boolean,
): List<OCSpace> {
if (searchFilter.isEmpty()) return spaces.filter { shouldShowDisabledSpace(it) }
val lowerFilter = searchFilter.lowercase()
var filtered = if (isMultiPersonal) {
spaces.filter { it.name.lowercase().contains(lowerFilter) && shouldShowDisabledSpace(it) }
} else {
spaces.filter { it.name.lowercase().contains(lowerFilter) && !it.isPersonal && shouldShowDisabledSpace(it) }
}
if (!isMultiPersonal) {
spaces.find { it.isPersonal }?.let { personalSpace ->
filtered = filtered.toMutableList().apply { add(0, personalSpace) }
}
}
return filtered
}
}SpacesListFragment would then call SpacesFilter.filterSpaces(uiState.spaces, uiState.searchFilter, isMultiPersonal) { shouldShowDisabledSpace(it) }. Proposed test (owncloudApp/src/test/java/com/owncloud/android/presentation/spaces/SpacesFilterTest.kt), covering multi-personal multi-match, single-personal pinning, no-match, and empty-filter cases:
class SpacesFilterTest {
private fun space(name: String, isPersonal: Boolean, isDisabled: Boolean = false): OCSpace =
mockk<OCSpace>(relaxed = true).also {
every { it.name } returns name
every { it.isPersonal } returns isPersonal
every { it.isDisabled } returns isDisabled
}
private val showAll: (OCSpace) -> Boolean = { true }
@Test fun `multi-personal with filter matches multiple personal spaces`() {
val a = space("Personal Alpha", true); val b = space("Personal Beta", true)
val project = space("Marketing Project", false)
assertEquals(listOf(a, b), SpacesFilter.filterSpaces(listOf(a, b, project), "personal", true, showAll))
}
@Test fun `single-personal with filter still pins one personal space at top`() {
val personal = space("Personal", true); val match = space("Engineering Docs", false); val noMatch = space("Random", false)
assertEquals(listOf(personal, match), SpacesFilter.filterSpaces(listOf(personal, match, noMatch), "eng", false, showAll))
}
@Test fun `no matches returns empty list`() {
val personal = space("Personal", true); val project = space("Marketing", false)
assertEquals(emptyList<OCSpace>(), SpacesFilter.filterSpaces(listOf(personal, project), "zzz-no-match", false, showAll))
}
@Test fun `empty filter returns all spaces passing disabled-space check`() {
val personal = space("Personal", true); val project = space("Marketing", false); val disabled = space("Archived", false, true)
assertEquals(listOf(personal, project), SpacesFilter.filterSpaces(listOf(personal, project, disabled), "", true) { !it.isDisabled })
}
}This also resolves the MVVM nit below: filtering is business logic and reads better as a pure function than inline in the Fragment's flow collector.
Generated by Claude Code
🤖 Automated review by Claude Code (security · stability · performance · coverage) Generated by Claude Code |
Related Issues
App:
Fixes #4674
On Kiteworks multi-personal accounts, the spaces search filter excluded every personal space from the name match (
!it.isPersonal) and then re-added only the first personal space pinned at position 0, so other personal spaces could never be surfaced by name. As DeepDiver1975 pointed out in the issue,isMultiPersonal(fromcapabilities.spaces.hasMultiplePersonalSpaces) was already set inSpacesListFragmentbut never consulted by the filter branch.The non-empty
searchFilterpath now branches onisMultiPersonal: multi-personal accounts filter all spaces uniformly by name (no personal exclusion, no pinned re-add), while single-personal ownCloud servers keep the current behavior unchanged (non-personal name match plus the personal space pinned at position 0). Purely client-side list filtering; no ViewModel, data-layer, or server change.ReleaseNotesViewModel.ktcreating a newReleaseNote()with String resources (if required) - N/A, bug fix with no release-note entry requiredQA
shouldShowDisabledSpacein both branches.