Skip to content

Add a transfer ownership background job #17736

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.

Sign up for GitHub

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

Jump to bottom
Merged
rullzer merged 2 commits into from
Dec 2, 2019
Merged

Add a transfer ownership background job #17736

rullzer merged 2 commits into from
Dec 2, 2019

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Oct 29, 2019 •
edited
Loading

Allow users to initiate transfer ownership

We need to find some way to initiate this.

  • Once it is initiated the recipieint should get a notification to accept or decline the transfer
  • Initiator is noticed about accept/decline
  • If accepted transfer is scheduled
  • Transfer happens
  • Both parties are notified when transfer completes

TODO:

  • Requires Extract transfer ownership logic into a reusable service #18025
  • The actual background job
    • Triggers notification on completion
  • Find some UI that works to send a file/folder to another user
    • For now it will just be in the settings somewhere
  • Notification ping pong
    • Send notification on initiating transfer
    • Accepting notifications
      • Rights check
      • Clears notification
      • Inserts background job
    • Rejecting notification
      • Display rejection
      • Clear notification
      • Send notification
    • Send notifications on completion

@rullzer rullzer added this to the Nextcloud 18 milestone Oct 29, 2019
nickvergessen
nickvergessen requested changes Oct 30, 2019
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Blocking merge as per private chat

This was referenced Nov 20, 2019
Merged
Merged
@rullzer rullzer force-pushed the enh/transfer_ownership branch from b47072e to 473021f Compare November 26, 2019 09:11
@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 26, 2019
skjnldsv
skjnldsv reviewed Nov 26, 2019
View reviewed changes
skjnldsv
skjnldsv reviewed Nov 26, 2019
View reviewed changes
skjnldsv
skjnldsv reviewed Nov 26, 2019
View reviewed changes
skjnldsv
skjnldsv reviewed Nov 26, 2019
View reviewed changes
skjnldsv
skjnldsv reviewed Nov 26, 2019
View reviewed changes
apps/files/src/components/TransferOwnershipDialogue.vue
const url = generateOcsUrl('/apps/files/api/v1/', 2) + 'transferownership'
axios.post(url, data)
Copy link
Member

@skjnldsv skjnldsv Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Use async await and try...catch please :)
Not then...else

Copy link
Member Author

@rullzer rullzer Nov 26, 2019

Choose a reason for hiding this comment

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

@ChristophWurst 😉

Copy link
Member

@ChristophWurst ChristophWurst Nov 26, 2019

Choose a reason for hiding this comment

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

@skjnldsv fix

Copy link
Member

@skjnldsv skjnldsv Nov 26, 2019

Choose a reason for hiding this comment

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

No time

@rullzer rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 26, 2019
@rullzer
Copy link
Member Author

rullzer commented Nov 26, 2019

Ah mmm let me actually finish my todos...

@rullzer rullzer force-pushed the enh/transfer_ownership branch from 962e241 to 07501fa Compare November 27, 2019 07:57
@rullzer
Copy link
Member Author

rullzer commented Nov 27, 2019

Maybe instead of the path store the file id + name of the node? That makes it less error prone.

@rullzer
Copy link
Member Author

rullzer commented Nov 27, 2019

We should probably have autocomplete on the user search?

ChristophWurst
ChristophWurst reviewed Nov 27, 2019
View reviewed changes
ChristophWurst
ChristophWurst requested changes Nov 27, 2019
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Otherwise good to go

@rullzer
Copy link
Member Author

rullzer commented Nov 27, 2019

Good to go now form my PoV

@rullzer rullzer added the 3. to review Waiting for reviews label Nov 27, 2019
juliusknorr
juliusknorr requested changes Nov 27, 2019
View reviewed changes
nickvergessen
nickvergessen reviewed Nov 27, 2019
View reviewed changes
nickvergessen
nickvergessen requested changes Nov 27, 2019
View reviewed changes
@juliusknorr
Copy link
Member

juliusknorr commented Nov 27, 2019 •
edited by rullzer
Loading

ToDo for later polishing:

  • The UI should have some visual feedback

nickvergessen
nickvergessen reviewed Nov 27, 2019
View reviewed changes
@ChristophWurst
Copy link
Member

ChristophWurst commented Nov 27, 2019

The UI should have some visual feedback

Partly done with 24178cf but not pretty.

@rullzer
Copy link
Member Author

rullzer commented Nov 27, 2019

All resolved.

juliusknorr
juliusknorr reviewed Nov 27, 2019
View reviewed changes
apps/files/src/components/TransferOwnershipDialogue.vue Outdated
const picker = getFilePickerBuilder(t('files', 'Select directory to transfer'))
.setMultiSelect(false)
.addMimeTypeFilter('httpd/unix-directory')
Copy link
Member

@juliusknorr juliusknorr Nov 27, 2019

Choose a reason for hiding this comment

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

Intended? Now directories cannot be selected anymore.

Copy link
Member Author

@rullzer rullzer Nov 27, 2019

Choose a reason for hiding this comment

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

Yes it is the great joy of the selector. We want to be able tos elect

  • files
  • folders
  • root folder

Copy link
Member

@juliusknorr juliusknorr Nov 27, 2019

Choose a reason for hiding this comment

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

Ah ok, just was thinking that it would only transfer directories accoring to the UI. Anyway transfering files always fails for me. Let me check where that goes wrong.

Copy link
Member

@ChristophWurst ChristophWurst Dec 2, 2019

Choose a reason for hiding this comment

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

There was something in the transer ownership service that checked if the path is a directory. I changed it to also allow files.

juliusknorr
juliusknorr reviewed Nov 27, 2019
View reviewed changes
juliusknorr
juliusknorr reviewed Nov 27, 2019
View reviewed changes
@rullzer
Copy link
Member Author

rullzer commented Nov 27, 2019

Ok review time. lets get this in

juliusknorr
juliusknorr approved these changes Nov 27, 2019
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@rullzer rullzer force-pushed the enh/transfer_ownership branch 2 times, most recently from cf09f19 to a676b7f Compare December 2, 2019 09:22
ChristophWurst
ChristophWurst approved these changes Dec 2, 2019
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 2, 2019
rullzer and others added 2 commits December 2, 2019 15:28
@rullzer
Add a transfer ownership background job
5274c54 
This job can be initiated by a user to transfer a file/folder to a
target user.

The target user will have to accept the job.
Once that is done the transfers is initiated in the background.

Both parties get notified when the job is done.

Signed-off-by: Roeland Jago Douma <[email protected]>
@ChristophWurst @rullzer
Also allow transfer ownership of files
08a1b92 
Signed-off-by: Christoph Wurst <[email protected]>
@rullzer rullzer force-pushed the enh/transfer_ownership branch from a6c5e58 to 08a1b92 Compare December 2, 2019 15:25
@rullzer rullzer merged commit 60c0596 into master Dec 2, 2019
@rullzer rullzer deleted the enh/transfer_ownership branch December 2, 2019 18:06
@nickvergessen nickvergessen mentioned this pull request Dec 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@skjnldsv skjnldsv Awaiting requested review from skjnldsv skjnldsv is a code owner

Assignees

@rullzer rullzer

@nickvergessen nickvergessen

@icewind1991 icewind1991

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement

Projects

None yet

Milestone

Nextcloud 18.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@rullzer @juliusknorr @ChristophWurst @nickvergessen @skjnldsv @icewind1991