Skip to content

Move invite-accepted capability and implementation to contacts app. #57853

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
Open
redblom wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Move invite-accepted capability and implementation to contacts app. #57853

redblom wants to merge 2 commits into from
+12 −367

Conversation

@redblom
Copy link

@redblom redblom commented Jan 27, 2026 •
edited
Loading

  • Resolves: #

Summary

Code moved to contacts app, see nextcloud/contacts#4417

TODO

  • deal with inviteAcceptedDialog code

Checklist

@redblom redblom requested a review from a team as a code owner January 27, 2026 15:43
@redblom redblom requested review from leftybournes, nfebe, salmart-dev and sorbaugh and removed request for a team January 27, 2026 15:43
@redblom redblom marked this pull request as draft January 27, 2026 15:43
@ChristophWurst
Copy link
Member

ChristophWurst commented Jan 28, 2026

@redblom why is this moved out of server and could it break OCM when the contacts app is not installed? the app is not shipped. it is optional.

@redblom redblom force-pushed the move-ocm-invite-accepted-route-impl-to-contacts-app branch from 7cb53f3 to 2d2e4a1 Compare January 28, 2026 16:58
@ArtificialOwl
Copy link
Member

ArtificialOwl commented Jan 28, 2026

@redblom why is this moved out of server and could it break OCM when the contacts app is not installed? the app is not shipped. it is optional.

This is done on my request:

  • it is moved out of server because the implementation of the feature creates its own db table and does not provide an API to manage it,
  • it will not break OCM, it will only not enable the 'invite-accepted' capability if contacts is not installed. the capability is useless without the contacts app,
  • contacts will register the capability and fully manage the db table

@ArtificialOwl
Copy link
Member

ArtificialOwl commented Jan 28, 2026

let's ping @mickenordin on this one !

@mickenordin mickenordin force-pushed the move-ocm-invite-accepted-route-impl-to-contacts-app branch from 2d2e4a1 to aea3c70 Compare January 28, 2026 18:22
@mickenordin
Copy link
Contributor

mickenordin commented Jan 28, 2026

let's ping @mickenordin on this one !

I haven't reviewed the change, but in principal I see no problem with it.

@redblom
fix(ocm): Move invite-accepted capability and implementation to conta…
254681c 
…cts app.

Signed-off-by: antoonp <antoon@redblom.com>
@redblom redblom force-pushed the move-ocm-invite-accepted-route-impl-to-contacts-app branch from aea3c70 to 254681c Compare January 30, 2026 09:26
@redblom
Copy link
Author

redblom commented Jan 30, 2026

Rebased, squashed, removed draft label.

@redblom redblom marked this pull request as ready for review January 30, 2026 10:12
@redblom redblom requested review from a team and nickvergessen as code owners January 30, 2026 10:12
nickvergessen
nickvergessen requested changes Jan 30, 2026
View reviewed changes
apps/cloud_federation_api/lib/Migration/Version1016Date202502262004.php Outdated
Copy link
Member

@nickvergessen nickvergessen Jan 30, 2026

Choose a reason for hiding this comment

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

This has to stay and instead we need a new migration file to drop the table

Copy link
Author

@redblom redblom Jan 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I see that the migration needs to stay, but an additional one that drops it? A server upgrade would delete existing data. Could you please explain.

Copy link
Member

@nickvergessen nickvergessen Jan 30, 2026

Choose a reason for hiding this comment

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

existing migration files are not rechecked. Removing this file does not remove the database table.
This migration should stay and instead you create a new migration with occ migration:generate cloud_federation_api that drops the table.
This way all Nextclouds independent from the version they got installed with will not have the database table anymore going forward.

lib/public/OCM/Events/LocalOCMDiscoveryEvent.php
*
* @return IOCMProvider
*
* @since 33.0.0
Copy link
Member

@nickvergessen nickvergessen Jan 30, 2026

Choose a reason for hiding this comment

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

Suggested change
* @since 33.0.0
* @since 34.0.0

Copy link
Author

@redblom redblom Jan 30, 2026

Choose a reason for hiding this comment

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

Even when the plan is to backport to 33?

@redblom
Restore migration.
f6d739f 
Signed-off-by: antoonp <antoon@redblom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen requested changes

@salmart-dev salmart-dev Awaiting requested review from salmart-dev salmart-dev is a code owner automatically assigned from nextcloud/server-backend

@nfebe nfebe Awaiting requested review from nfebe nfebe is a code owner automatically assigned from nextcloud/server-backend

@leftybournes leftybournes Awaiting requested review from leftybournes leftybournes is a code owner automatically assigned from nextcloud/server-backend

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh is a code owner automatically assigned from nextcloud/server-backend

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@redblom @ChristophWurst @ArtificialOwl @mickenordin @nickvergessen