transfer ownership of a form

[hamza221]

[codecov]

Codecov Report

Merging #1416 (817847b) into main (601becf) will decrease coverage by 0.34%.
Report is 2 commits behind head on main.
The diff coverage is 0.00%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1416      +/-   ##
============================================
- Coverage     44.92%   44.59%   -0.34%     
- Complexity      649      653       +4     
============================================
  Files            58       58              
  Lines          2544     2563      +19     
============================================
  Hits           1143     1143              
- Misses         1401     1420      +19     

[Chartman123]

I get an error when I actually transfer the form to another user. Probably because you're staying in the same route after the transfer and therefore on a form for which the current user doesn't have edit permissions anymore.

Second, a little loading indicator would be nice between selecting a user and showing the modal. It takes some time (around 3 to 5 seconds for me) between choosing the user and the modal being shown.

[jancborchardt]

Nice! Some design details:

• This setting should be all the way on the bottom of the settings, since it’s the least common one
• It would be better as a Button than a switch
• The label "Transfer ownership" is clearer
• I’d expect the modal to open on button press also for the name input directly, like GitHub does on repository transfers. Then we also only have one modal
  
• The modal needs a heading, like "Transfer [formname]"
• The buttons are wrongly colored and labeled – "Yes/No" is too inconcrete. Similar to Github, we only need a confirmation button, which is red, and labeled "Transfer [formname] to [username]". For canceling we have the x in the top right
• The words "ownership", "form" and "cancelled" need to be lowercase when part of a sentence

[hamza221]

This last commit addresses all the mentioned issues but loading because I didn't find a straightforward way to address it since the event is triggered in one component "SettingsSidebarTab" and handled in another one "Forms".

I'll get back to it. But currently I'm writing my thesis ( I Have to submit it on the 10th) so I'm trying to gain time by getting all PRs to a state where I can write documentation about them. and I'll get all of them to a merging state after the 10th.

[jotoeri]

Can you create a separate component for the transfer? Should be good, if it just contains the Button and the Modal, including all corresponding procedures. Then you can just load the new component on the sidebar.

But currently writing my thesis

Good luck! 🤞 💪

[hamza221]

Per @jotoeri recommendation I created a separate component for ownership transfer. I'll add the missing tests later this week

[Chartman123]

@hamza221 The current solution works much smoother than before. Nice job 👍🏻

[jotoeri]

Nice @hamza221 :) I still have a bunch of comments here, but that's more since i now commented many small things. Design-wise and css-wise there is also a bit to do still, but maybe @jancborchardt can also help you on this. But in general, it feels like this is on a good way now. :)

A rebase (& squash) would also be good, when you're on it.

[jospoortvliet]

Hi, just curious - what's still on the todo here? Does it 'just' need another review & approval?

[Chartman123]

@jospoortvliet I'll have another look at it :)

[Chartman123]

@hamza221 @jospoortvliet I think there is still a little work to do. On loading the modal the input box has a little glitch:

When you closse the recommendation by clicking somewhere else in the modal and then click back into the user selection, everything looks fine.

Another thing that I noticed: When you select a user, the input field should lose the focus so that you don't have to click anywhere else to close the lower part.

And the button to start the transfer only reacts to the "security" input of the form name. It doesn't matter if you have selected a user or not and then of course throws an error in the console when you didn't select a user first.

[jospoortvliet]

awesome! Now, with those fixed, what's missing? The reviews are all implemented, correct? So a final thumbs up and merge?

[Chartman123]

There are still some Lint errors. I would also like to just have another look at the code (mostly naming related).

[Chartman123]

This is still open

[hamza221]

still working on the changes will push my changes in a bit the previous commit was just a rebase

[hamza221]

I couldn't fix the input glitch, @Chartman123 do you think it might be a bug in NcSelect itself?

[hamza221]

A few more comments. The animation problem seems to be fixed now, too. But the modal changes size when you have selected a user and click back into the field. The cursor looks misplaced, too.

I can't reproduce after the nextcloud/vue bump , @Chartman123 does it still happen on your end?

[Chartman123]

I can't reproduce after the nextcloud/vue bump , @Chartman123 does it still happen on your end?

@hamza221 seems fixed upstream, yes 👍🏻 no problems here anymore

[Chartman123]

could not invoke event listener TypeError: this.sharedForms[sharedFormIndex] is undefined
    onLastUpdatedByEventBus Forms.vue:279
    mounted Forms.vue:111
    emit index.mjs:43
    emit index.mjs:41
    emit index.mjs:25
    emit index.mjs:105
    onOwnershipTransfer TransferOwnership.vue:69
    VueJS 4
    click NcButton.mjs:185
    VueJS 33
index.mjs:46:24
    emit index.mjs:46
    emit index.mjs:41
    emit index.mjs:25
    emit index.mjs:105
    onOwnershipTransfer TransferOwnership.vue:69
    VueJS 4
    click NcButton.mjs:185
    VueJS 33

I get the above error after clicking the transfer button in the modal. I think you have to emit the lastUpdated before you really do the owner transfer because otherwise the current user no longer has access to the form.

[susnux]

Nice works! But a few comments.

[hamza221]

Requested changes are implemented

[hbiering]

I tried to transfer ownership from myself (as admin) to a group. The Group-name was found when I started typing it in the upper field - and the transfer option was enabled when I entered the required (bold) phrase in the second field. But when continuing, I got the error message: "An error occurred while transfering ownership"

Is the transfer option intended to support transfer to User Groups (as the group shows up when you start to type) or is it limited to individual users. Although you can share final results in a spreadsheet the latter is a problem for collaboration in teams.

[susnux]

Ownership is bound to users so the bug is that the search does not limit to users.