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

Feature/group share #141

Merged
merged 9 commits into from
Oct 24, 2023
Merged

Conversation

DavidProdinger
Copy link

Group Share

Add a group share functionality wich closes #93
Additionally to users, you can now select and share clients to groups.

Tests

  • Improved login for tests (session)
  • Update Cypress

Misc

  • Replaced / simplified some queries with named parameters
  • Use Dependency Injection instead of depreceated \OC::$server->getLogger(); functions

For the future

I have added a permission column to the shares, wich can use the following values:

  • -1 To view own (current state)
  • 0 To view all
  • 1 To view and edit all

This could solve the issues #78 #84

@te-online
Copy link
Owner

Thank you so much for your contribution! I’m not in a situation right now, where I can check out and merge your changes, but I appreciate them a lot! Let me get back to you in a week or so.

@vsn-insolit
Copy link

Hope to see this feature soon into production! Thank you. It was exactly the feature I wanted to suggest.

@te-online
Copy link
Owner

te-online commented Oct 7, 2023

I’m really sorry about the radio silence from my side. It’s just a very bad timing where I have a bunch of personal priorities in my life that have to come first. I’ll get this reviewed and merged as soon as I can 🙏

@vsn-insolit
Copy link

I don't know if this feature went forward of being implemented, but I really hope so that the group share will become available. I'm not a programmer, but is it possible that another user to mark as complete an request and push it as an update for the app? Thank you

@te-online
Copy link
Owner

te-online commented Oct 24, 2023

Hey @DavidProdinger Thanks again for your work, this looks really good ❤️

I've done a first code review today and I fully agree with the changes you made. I do have some minor stylistic changes I'd like to make and I have these changes that I plan to make (but am open for discussion):

  1. Since this version has not been released yet, I'm considering to consolidate both migrations into one file before sending a new version to the app store. ✅ (It's actually necessary to have 2 migrations to drop the column after migration, I think?)
  2. I think I would design the permissions column a bit differently. Probably taking some inspiration from other places in Nextcloud, where I believe bitmasks (similar to Unix permissions) have been more popular lately. But it doesn't look like you built UI or functionality for this yet, so I guess we can still change it. ✅ (I removed the column for now, let's add it back later)
  3. After the branch is merged I will take a look if the UI (that I initially built for simple sharing) could use a bit of love and care, or if it's acceptable how it is right now, even after adding the groups. ✅ (looking good after some basic testing)

I'll merge your work now and will continue work on the main branch within the next weeks (hopefully it will be a bit faster than this response).


@vsn-insolit Yes, this PR will add support for sharing clients with groups. However, if I understand your request correctly, I'm sorry to disappoint you for now, because the sharing feature will continue to work in a way where only the author of the entry will be able to edit it for the time being.

@te-online te-online merged commit 2b92e44 into te-online:main Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Sharing with others
3 participants