Skip to content

fix: show all personal spaces in filter on multi-personal (Kiteworks) accounts#4922

Open
mvanhorn wants to merge 1 commit into
owncloud:masterfrom
mvanhorn:fix/4674-kiteworks-space-filter
Open

fix: show all personal spaces in filter on multi-personal (Kiteworks) accounts#4922
mvanhorn wants to merge 1 commit into
owncloud:masterfrom
mvanhorn:fix/4674-kiteworks-space-filter

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Jul 8, 2026

Copy link
Copy Markdown

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 (from capabilities.spaces.hasMultiplePersonalSpaces) was already set in SpacesListFragment but never consulted by the filter branch.

The non-empty searchFilter path now branches on isMultiPersonal: 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.

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required) - N/A, bug fix with no release-note entry required

QA

  1. On a Kiteworks (multi-personal) account, open the spaces list and type a filter string: every personal space whose name matches is shown, none is force-pinned.
  2. On a single-personal ownCloud server, type a filter string: behavior is unchanged (personal space pinned at position 0, non-personal spaces matched by name).
  3. Disabled spaces still follow shouldShowDisabledSpace in both branches.

@mvanhorn mvanhorn requested a review from a team as a code owner July 8, 2026 11:21
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jesmrec

jesmrec commented Jul 8, 2026

Copy link
Copy Markdown
Member

ey @mvanhorn, thanks a lot for you contribution!! let's take a first look!

  • PR title matches conventional commits, cool! ✅
  • Commit name also matches conventional commits ✅
  • Commits must be GPG/PGP signed, including a Signed-off-by line. Use git commit -s -S -m "..." for that ❌
  • CLA not signed (check CI). That's a must ❌
  • If it's posible, please separate the calens file in another commit and title it as chore: add calens file. It helps. 🙏
  • Also, regarding calens content, please add the link to the PR as well, not only the issue one. 🙏

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 dj4oC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.spaces is already scoped to the active account at the repository/use-case layer (accountName-filtered queries) and isMultiPersonal is 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 !isMultiPersonal branch's personalSpace?.let { add(0, personalSpace) } re-adds the personal space without re-checking shouldShowDisabledSpace, 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; collectLatestLifecycleFlow usage is unchanged and isMultiPersonal is guaranteed initialized (via a blocking call in onViewCreated) before this collector ever runs, so there's no race window. Separately noted: CapabilityViewModel.checkMultiPersonal() blocks the main thread via runBlocking — 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.setData uses DiffUtil, 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 !isMultiPersonal branch still does an extra list copy via toMutableList() 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, or isMultiPersonal branching (verified via GitHub code search, not the PR's own manual-QA claims). See the inline suggestion: extracting the filter into a pure SpacesFilter.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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

dj4oC commented Jul 8, 2026

Copy link
Copy Markdown
Contributor
  • Security: no findings
  • Stability: no findings
  • Performance: no findings
  • Test coverage: 0% (JUnit), below 85% — tests proposed in review
  • TODOs found: none
  • Dependency touched: no
  • CI status: all green
  • Stale review: no

🤖 Automated review by Claude Code (security · stability · performance · coverage)


Generated by Claude Code

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Kiteworks accounts with bad Space filtering

4 participants