Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ask before automatically syncing a folder that was just shared. #1199

Closed
wants to merge 5 commits into from
Closed

Conversation

shaan7
Copy link

@shaan7 shaan7 commented Apr 11, 2019

Fixes: #1169

The idea, as described in the issue is to prevent users from sharing a folder with someone and keep adding files to it to use up disk space.
I've kept the default to false to maintain behavior when people upgrade.

@shaan7 shaan7 marked this pull request as ready for review April 12, 2019 10:40
@DominiqueFuchs
Copy link
Contributor

Thanks a lot @shaan7 , seems like a great addition to me! server side options regarding this topic are yet to be implemented AFAIK. Code looks fine to me, however I'd like to double-check the corresponding cases with a test instance of mine. I'll report back if I reproduced this with your changes.
@jancborchardt any further comments on this change?

@jancborchardt
Copy link
Member

Yep, the server-side issue of this is at: Require internal shares to be accepted nextcloud/server#2192

And I’d say this should mainly be done server-side. Then on the server, desktop client and mobile apps you would get a notification that something was shared with you and you can accept or reject it.

(I fear a bit that this work-around could result in some un-synced folders down the line because you clicked "No" and don’t necessarily know how to enable them back again through selective sync.)

@shaan7
Copy link
Author

shaan7 commented Sep 12, 2019

I agree with @jancborchardt . Once there is server-side support for this, the client no longer needs to worry about the specific reason why confirmation should be required from the user.

So, whether or not a client-side check should be merged now is a question of the timelines of the server-side issue. If its going to be picked soon, I'd ask that this PR be discarded in favor of that. However, if timelines are uncertain, it might make sense to have a client-side option available to users.

@DominiqueFuchs
Copy link
Contributor

I especially get your point about the danger of forgetting about this after confirming this one-time dialog. That said, given the reference to the issue on the server repo and having the current situation here explained, I'd vote to close this PR here (at least for the moment) - also to make clear, that we are not going to work at this right now for given reasons.

@jancborchardt
Copy link
Member

There is a work-in-progress pull request for the server part at nextcloud/server#16828 by @nickvergessen

So @shaan7 if you are fine with it I’d say we close this one. And if you like, reviews on the server pull request are very welcome! :)

@shaan7 shaan7 closed this Sep 12, 2019
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.

Newly shared folder gets automatically synced
4 participants