Skip to content

fix(files_sharing): Show remote shares as external - #52423

Merged
nfebe merged 3 commits into
masterfrom
fix/51226/show-remote-shares-as-external-2
Apr 28, 2025
Merged

fix(files_sharing): Show remote shares as external#52423
nfebe merged 3 commits into
masterfrom
fix/51226/show-remote-shares-as-external-2

Conversation

@nfebe

Copy link
Copy Markdown
Contributor

Description

pick 4b68caa fix(files_sharing): Show remote shares in external shares section
pick 62b77c6 perf(files_sharing): Change sharing filtering from O(3n) to O(n)

Screenshots

BeforeAfter
beforeafter

Resolves : #51226

  • ...

Checklist

@nfebenfebe changed the title Fix/51226/show remote shares as external 2fix(files_sharing): Show remote shares as external 2Apr 24, 2025
@nfebenfebe changed the title fix(files_sharing): Show remote shares as external 2fix(files_sharing): Show remote shares as externalApr 24, 2025
@nfebenfebe self-assigned this Apr 24, 2025
@github-project-automationgithub-project-automationBot moved this to 🏗️ In progress in 📁 Files teamApr 24, 2025
@nfebe
nfebeforce-pushed the fix/51226/show-remote-shares-as-external-2 branch from 62b77c6 to c6e4e63CompareApril 24, 2025 23:14
@nfebe
nfebe marked this pull request as ready for review April 24, 2025 23:14
@nfebe
nfebe requested a review from a team as a code ownerApril 24, 2025 23:14
@nfebe
nfebe enabled auto-merge April 24, 2025 23:14
@nfebe

Copy link
Copy Markdown
ContributorAuthor

/compile

@AndyScherzinger

Copy link
Copy Markdown
Member

Looping in @maximelehericy@jancborchardt and @ArtificialOwl to check this also works nicely in a Global Scale setup where federated shared (optionally?) should be seen as internal shares.

@nfebe
nfebeforce-pushed the fix/51226/show-remote-shares-as-external-2 branch from 54b7f09 to d190d59CompareApril 25, 2025 10:22
@kesselb

Copy link
Copy Markdown
Contributor

Case 1: Existing shares are sorted properly ✅
Case 2: A new federated shares is added to the external shares list ❌

Screencast.From.2025-04-25.12-42-42.webm

@kesselbkesselb 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.

nfebe added 2 commits April 25, 2025 11:59
Replaced multiple Array.filter() calls with a single loop to improve performance.
This avoids redundant iterations over the shares array and categorizes them more efficiently.
Signed-off-by: nfebe <fenn25.fn@gmail.com>
@nfebe
nfebeforce-pushed the fix/51226/show-remote-shares-as-external-2 branch from d190d59 to 6454cb5CompareApril 25, 2025 11:00
@nfebe
nfebe requested a review from kesselbApril 25, 2025 11:01
@nfebe

Copy link
Copy Markdown
ContributorAuthor

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@szaimen
szaimen removed their request for review April 27, 2025 17:54

@maximelehericymaximelehericy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @nfebe , for global scale instances, in certain cases, what is expected is having all accounts considered as internal. Can you holdthis PR until we figured out a way to handle your case and the GS one ?

@sorbaugh

Copy link
Copy Markdown
Contributor

Hi @nfebe , for global scale instances, in certain cases, what is expected is having all accounts considered as internal. Can you holdthis PR until we figured out a way to handle your case and the GS one ?

@jancborchardt an idea would be some kind of switch. Is this ok or is there maybe a better idea? Would need your input so @nfebe knows how to adjust 🙏

@jancborchardtjancborchardt 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.

@jancborchardt an idea would be some kind of switch. Is this ok or is there maybe a better idea?

@sorbaugh a switch sounds fine ("Treat remote shares as internal shares"), if it can’t be done automatically through some means like binding to another existing sharing setting or some other setup or license aspect?

And considering @maximelehericy’s comment as well:

for global scale instances, in certain cases, what is expected is having all accounts considered as internal.

There might be some other things to adjust as well. What comes to mind is removing the text "(remote)" after the name. This should then be bound to the same logic.

@susnux

Copy link
Copy Markdown
Contributor

Iirc we treat federation different with global scale anyways so we could just check if global scale is enabled and if it is then we just treat any remote user as internal at least for the sharing sidebar maybe this could be an option so we don't need any UI option.

(At least all the lookup logic is changed with GS enabled)

@nfebe

Copy link
Copy Markdown
ContributorAuthor

Hi @nfebe , for global scale instances, in certain cases, what is expected is having all accounts considered as internal. Can you holdthis PR until we figured out a way to handle your case and the GS one ?

Hi @maximelehericy thanks for the input. For the majority of use cases and users this is a "bug" starting from when the sidebar was split, a switch, or any other solution can follow via a follow up ticket but the fix of first treating remote shares as external should be pushed forward (imo).

@nfebe
nfebe merged commit 6a2c0a2 into masterApr 28, 2025
@nfebe
nfebe deleted the fix/51226/show-remote-shares-as-external-2 branch April 28, 2025 09:13
@nfebe

Copy link
Copy Markdown
ContributorAuthor

/backport to stable31

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

Labels

3. to reviewWaiting for reviewsfeature: sharing

Projects

Status: 🏗️ In progress

Development

Successfully merging this pull request may close these issues.

[Bug]: Remote shares are shown as internal shares

9 participants

@nfebe@AndyScherzinger@kesselb@sorbaugh@susnux@jancborchardt@skjnldsv@maximelehericy@nextcloud-command