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

Re-enable sharing API for recent NC versions #214

Merged
merged 3 commits into from
Aug 12, 2024

Conversation

DanRiess
Copy link
Contributor

@DanRiess DanRiess commented Aug 8, 2024

As discussed in #197, here is a PR that addresses the latest share API issues and re-enables sharing for recent NC versions, while keeping the ability to retrieve webapppassword tokens on all NC versions that I have tested.

…ns while keeping the functionality to retrieve a password in any NC version.
@pbek
Copy link
Member

pbek commented Aug 8, 2024

@aleixq, could you please test this?

Copy link
Member

Choose a reason for hiding this comment

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

What did you change here, or was this an accident?

Copy link
Member

Choose a reason for hiding this comment

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

If yes, can you please remove the change from your commit. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Git was yelling at me that there were 2 files, crop.svg and Crop.svg. It made this change automatically and I couldn't revert it. So yeah, let's call it an accident

Copy link
Member

Choose a reason for hiding this comment

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

Could you please remove the change and force push? 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it should be gone now.

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to squash your commits? Otherwise, I will while merging...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you could do it while merging, I'd prefer that. Haven't done it before and chances are I'll break something

Copy link
Member

Choose a reason for hiding this comment

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

Life got a lot easier when I started using lazygit and learned about fixups.

Copy link
Member

Choose a reason for hiding this comment

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

…C versions while keeping the functionality to retrieve a password in any NC version."

This reverts commit bb4f0df.
pbek added a commit that referenced this pull request Aug 8, 2024
Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>
@pbek
Copy link
Member

pbek commented Aug 8, 2024

What I tested:

  • Test if Pull Request does not break app on latest Nextcloud 27 (27.1.11.3)
  • Test if Pull Request does not break app on latest Nextcloud 28 (28.0.8.1)
  • Test if Pull Request does not break app on latest Nextcloud 29 (29.0.4)

What's left is to test if the file sharing API works, @aleixq.

@aleixq
Copy link
Contributor

aleixq commented Aug 11, 2024

Sorry to be late I am on vacation righ tnow. It works! using nextcloud 29.0.2

I would like to keep the class with minimal changes, but redefining the constructor seems a way to make it work, by now...
Because it seems like this is a short workaround solution because I am seeing changes in nc 30 shareapicontroller constructor... :
https://github.com/nextcloud/server/blob/v30.0.0beta5/apps/files_sharing/lib/Controller/ShareAPIController.php#L73
So I suppose that there will be problems in 30... we can push now, but the list of classes uses will grow to cover versions, and I think that at some point some use class sentence will fail because of deprecation or so... we need to test for 30. Maybe a simple functional tests must be done to cover these changes.

When I get back to work in september I'll try again to claim for a native solution in nextcloud/server side... @DanReiss , your use case must be shown in #197 (comment) ! Just to show to nc server team that allowing fine-tuning of what's exposed (such as filesharing or preview api) will be useful.

@pbek
Copy link
Member

pbek commented Aug 12, 2024

Thank you for testing!

@pbek pbek merged commit cdb9f7f into digital-blueprint:main Aug 12, 2024
11 checks passed
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.

3 participants