Skip to content

Conversation

@ArnyminerZ
Copy link
Member

Purpose

Update the current implementation (right now no home-set is ever personal), to initialize the value based on the DAV:owner property.

Short description

HomeSet.personal will be true whenever DAV:owner is set and equal to the service's principal url.

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@ArnyminerZ ArnyminerZ self-assigned this Aug 6, 2025
@ArnyminerZ ArnyminerZ added the enhancement New feature or request label Aug 6, 2025
@ArnyminerZ ArnyminerZ linked an issue Aug 6, 2025 that may be closed by this pull request
@ArnyminerZ ArnyminerZ marked this pull request as ready for review August 6, 2025 11:16
@ArnyminerZ ArnyminerZ requested review from Copilot and sunkup August 6, 2025 11:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Updates the HomeSet initialization logic to properly determine if a home set is "personal" based on the DAV:owner property, addressing the issue where no home-set was ever marked as personal.

  • Implements personal home set detection by comparing DAV:owner property with service principal URL
  • Adds new isPersonal() method to evaluate ownership and determine personal status
  • Updates home set creation to use the new personal detection logic

Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

More detailed then necessary. See comments.

Otherwise its nice, clean and works really well. 👍

@ArnyminerZ ArnyminerZ requested a review from sunkup August 7, 2025 10:04
@ArnyminerZ ArnyminerZ requested a review from sunkup August 7, 2025 10:51
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

I just noticed something and since this is related to personal collections, here is another quick question about code in shouldPreselect:

When Settings.PRESELECT_COLLECTIONS_PERSONAL is selected, the code checks if the collection's homeSetId is in the list of personal home-set IDs. However, if the collection's homeSetId is null (which can happen if the collection is without a home-set, right?), this check will fail.

I guess that's what we want (if collection has no homeset it can't be personal and should not be preselected), but I think it's not clear that the code behaves this way. If I am right (?) we should add a short comment to document this.

Signed-off-by: Arnau Mora <[email protected]>
@ArnyminerZ
Copy link
Member Author

I guess that's what we want (if collection has no homeset it can't be personal and should not be preselected), but I think it's not clear that the code behaves this way. If I am right (?) we should add a short comment to document this.

Yes, I think that if a collection doesn't have a homeset, it won't ever be personal. Maybe @rfc2822 has something else to add, but I'd just add a comment.

@ArnyminerZ ArnyminerZ requested a review from sunkup August 8, 2025 08:27
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Looks good, but it's not done yet right?

1. Update the state
Right now the collection personal state is only saved on creation of the collection. We need to update it in refreshHomesetsAndTheirCollections and refreshCollectionsWithoutHomeSet.

2. Start using the new state
Now that we have the new personal state saved in collection we should use it:

  • use collection.personal instead of homesets.personal to filter the list of collections in the query: CollectionDao.pagePersonalByServiceAndType.
  • update HomeSetRefresher.shouldPreselect() to use the personal state from collection instead of home set

3. Remove personal state from home set
And then I wonder now: I think we do not need the home set personal state anymore? If we don't need it anymore we should remove it either now or in the future.

Since this change alters functionality (less collections are shown as personal, see screenshots below) and I guess it does not really hurt to query and save the home set personal state for a bit longer, maybe we should wait for a while and see whether there are any complaints?

Before (home set personal) Image
After (collection personal) Image

4. Show state in collection info screen
Optionally we could also show the personal state in the collection info now.


What do you think?

Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

See also #1372 (comment) – we should clarify the requirements first

@rfc2822 rfc2822 requested a review from a team as a code owner December 2, 2025 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use DAV:owner for "Show only personal"

3 participants