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

feat: transfer ownership via webgui #741

Merged
merged 14 commits into from
Jan 11, 2024
Merged

feat: transfer ownership via webgui #741

merged 14 commits into from
Jan 11, 2024

Conversation

enjeck
Copy link
Contributor

@enjeck enjeck commented Dec 14, 2023

This addresses #711
The goal is to add a way to transfer table ownershi using the webgui

@enjeck
Copy link
Contributor Author

enjeck commented Dec 14, 2023

I forgot to sign my commit messages. Can ammend that later

Copy link
Contributor Author

@enjeck enjeck left a comment

Choose a reason for hiding this comment

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

I do not yet understand how the backend and frontend connect to each other. I plan to spend more time on more tutorials to understand this. Since the associated issue for this was marked as a "good first issue", I thought I could easily handle this after following the introductory tutorial but I couldn't. So it's either I'm overcomplicating this and there's an easier way to go about it, or it's more complex than it seems. Would appreciate hints on what's the right direction to follow

appinfo/routes.php Outdated Show resolved Hide resolved
lib/Controller/Api1Controller.php Outdated Show resolved Hide resolved
src/modules/sidebar/partials/TransferForm.vue Outdated Show resolved Hide resolved
src/store/store.js Outdated Show resolved Hide resolved
@datenangebot
Copy link
Collaborator

I'm still very confused about what the UI should look like

The "edit table" modal was meant, that one is not crowded?!
Cloud look like this:
image

And if you click on the "change owner" button, your modal gets opened.

Btw: I guess the word "share" is not correct here, we have sharing option elsewhere for real sharing.

@enjeck
Copy link
Contributor Author

enjeck commented Dec 14, 2023

I'm still very confused about what the UI should look like

The "edit table" modal was meant, that one is not crowded?! Cloud look like this: image

And if you click on the "change owner" button, your modal gets opened.

Btw: I guess the word "share" is not correct here, we have sharing option elsewhere for real sharing.

@datenangebot Ah, I see what you mean. Makes sense. Should have asked clarified earlier. Thanks

@datenangebot
Copy link
Collaborator

datenangebot commented Dec 14, 2023

As a reminder for us, please make sure that we have this before merging:

  • CI is green
  • integration tests
  • end2end test
  • ensure that only allowed user can change ownership of tables

@juliusknorr juliusknorr added enhancement New feature or request 2. developing Work in progress labels Dec 18, 2023
src/modules/modals/TransferTable.vue Outdated Show resolved Hide resolved
src/modules/modals/TransferTable.vue Outdated Show resolved Hide resolved
@enjeck enjeck force-pushed the feature/711 branch 2 times, most recently from 59be853 to 720c8be Compare December 27, 2023 08:44
@enjeck enjeck marked this pull request as ready for review December 27, 2023 08:45
@enjeck enjeck changed the title (WIP) feat: transfer ownership via webgui feat: transfer ownership via webgui Dec 27, 2023
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.

Thanks @enjeck. I left a few inline comments regarding smaller code suggestions, but apart from that this looks quite nice already and works well :)

cypress/e2e/tables-table.cy.js Outdated Show resolved Hide resolved
cypress/e2e/tables-table.cy.js Outdated Show resolved Hide resolved
cypress/e2e/tables-table.cy.js Show resolved Hide resolved
src/modules/modals/TransferTable.vue Outdated Show resolved Hide resolved
@juliusknorr
Copy link
Member

Regarding the CI failure, you might want to check the details at https://github.com/nextcloud/tables/pull/741/checks?check_run_id=19976327121 on how to solve it and make sure to signoff commits in the future:

Screenshot 2023-12-27 at 15 35 36

@enjeck
Copy link
Contributor Author

enjeck commented Dec 27, 2023

Regarding the CI failure, you might want to check the details at https://github.com/nextcloud/tables/pull/741/checks?check_run_id=19976327121 on how to solve it and make sure to signoff commits in the future:

For some reason, trying to sign from VS Code doesn't work. I thought I set this up already, but apparently not :/. I'll try again

@juliusknorr juliusknorr linked an issue Dec 28, 2023 that may be closed by this pull request
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the reviewing process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR reviewing process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 4, 2024
@juliusknorr juliusknorr self-requested a review January 4, 2024 09:34
@datenangebot
Copy link
Collaborator

@enjeck Hey, do you want to rebase, force push and merge into main here? I mean feature seems to be good, CI is green and this PR is approved... 😁

@juliusknorr
Copy link
Member

juliusknorr commented Jan 11, 2024

Could you rebase instead of adding a merge commit? Let me know if you need some guidance on that :)

enjeck and others added 14 commits January 11, 2024 13:06
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Florian Steffens <florian.steffens@nextcloud.com>
- add suggestion for UI
- move user lookup to a shared component as it could be used elsewhere
- logic to request users and groups is missing, should be exchanged with new api endpoint

Signed-off-by: Florian Steffens <florian.steffens@nextcloud.com>
- make use of API v2 structure
- add route
- add controller method and API description
- update API docs

Signed-off-by: Florian Steffens <florian.steffens@nextcloud.com>
- reuse and adjust from occ command
- adjust occ command logic to fit to the new business
- adjust permission service to allow those changes

Signed-off-by: Florian Steffens <florian.steffens@nextcloud.com>
Signed-off-by: Florian Steffens <florian.steffens@nextcloud.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
Signed-off-by: Cleopatra Enjeck M <patrathewhiz@gmail.com>
@enjeck enjeck merged commit 173534d into main Jan 11, 2024
43 checks passed
@enjeck enjeck deleted the feature/711 branch January 11, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feedback-requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

transfer ownership via webgui
3 participants