Skip to content

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

Merged
joragua merged 2 commits into
owncloud:masterfrom
mvanhorn:fix/4674-kiteworks-space-filter
Aug 3, 2026
Merged

fix: show all personal spaces in filter on multi-personal (Kiteworks) accounts#4922
joragua merged 2 commits into
owncloud:masterfrom
mvanhorn:fix/4674-kiteworks-space-filter

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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

CLAassistant commented Jul 8, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@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

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

@joragua

joragua commented Jul 23, 2026

Copy link
Copy Markdown
Member

Hi @mvanhorn! What is the status of this PR? If you have any questions, feel free to ask and we will hep you! 🙌🏻I

@mvanhorn

Copy link
Copy Markdown
Contributor Author

Status update: added the unit tests the review asked for. Pulled the personal-space filter out of the fragment into a testable filterSpaces() extension and covered the multi-personal name match, the single-personal pin-at-front case, and the single-personal regression in SpacesListFragmentTest (6e8aa2a). Ready for another look - let me know if you'd rather see a fragment-level test too.

@jesmrec

jesmrec commented Jul 24, 2026

Copy link
Copy Markdown
Member

@mvanhorn thanks for the update!! please mind also the first update we commented: #4922 (comment)

It's important to put the PR in shape to be aligned with the codebase. Let us know any issue!

@mvanhorn
mvanhorn force-pushed the fix/4674-kiteworks-space-filter branch from 6e8aa2a to 7af6d53 Compare July 25, 2026 20:45
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Thanks for the clear checklist, all four items are done now. Branch rewritten, head is 7af6d53.

  • CLA signed, license/cla is green.
  • Both existing commits now carry a Signed-off-by line. They were already GPG-signed; the sign-off was the part I had missed.
  • The calens file is its own commit, chore: add calens file, split out of the code commit.
  • That entry now lists the PR alongside the issue:
https://github.com/owncloud/android/issues/4674
https://github.com/owncloud/android/pull/4922

So the branch is now three commits: the fix, the test, then the calens file.

The only content difference from the previous head is that one added PR link; the code and test trees are byte-identical to what you already reviewed, so nothing needs re-reading. All seven checks are green and it shows mergeable.

Sorry for the round trip on the sign-offs.

@joragua

joragua commented Jul 27, 2026

Copy link
Copy Markdown
Member

Thanks for all changes @mvanhorn! 🍻

I'll review the PR as soon as possible and suggest any changes if needed. Stay tuned! 😄

@joragua joragua left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @mvanhorn! Some comments here about your code 💯

Let us know if you ave any doubts and we will help you! 😄

Comment thread changelog/unreleased/4922
Comment thread changelog/unreleased/4674 Outdated
mvanhorn added 2 commits July 31, 2026 10:34
… accounts

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
@mvanhorn
mvanhorn force-pushed the fix/4674-kiteworks-space-filter branch from 7af6d53 to d39ed24 Compare July 31, 2026 17:34
@mvanhorn

Copy link
Copy Markdown
Contributor Author

All six addressed, thanks for the detailed pass.

  • Dropped the test class, and dropped its commit rather than adding a deletion on top, so the history is just the fix plus calens now.
  • filterSpaces is a private method on SpacesListFragment; the internal one is gone with the test.
  • multi -> multiPersonal.
  • visible parameter dropped, it calls shouldShowDisabledSpace directly.
  • Calens file renamed to 4922 with your suggested title, folded into the existing chore: add calens file commit rather than a new one.

@joragua joragua left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Nice job @mvanhorn!

Let's move it to QA 🚀

@jesmrec

jesmrec commented Aug 3, 2026

Copy link
Copy Markdown
Member

QA checks:

  • Bridged Kiteworks account filter - List of spaces
  • Bridged Kiteworks account filter - Folder picker
  • ownCloud account filter - List of spaces
  • ownCloud account filter - Folder Picker (showing always Personal)

@jesmrec

jesmrec commented Aug 3, 2026

Copy link
Copy Markdown
Member

Approved on my side! thanks a lot for the contribution @mvanhorn!! will be available in the 4.9 release!

@joragua
joragua merged commit 5e9c4ee into owncloud:master Aug 3, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Kiteworks accounts with bad Space filtering

5 participants