Add new share types for OCS Share API

[svierne]

Hi @luffah, I'd like to contribute the new share types introduced in the latest versions of NextCloud. 🙂
This PR is "work in progress", because I still need to write the tests for my changes. Should I create a new method or extend test_create(self) in tests/test_shares.py?

[luffah]

Hi, sorry for the delay.

Thx for you contribution.

For this case, you can extend existing tests.

You can move SHARE_WITH_ENABLED_SHARE_TYPES as a class property (see LOCAL as example) .

[svierne]

If I understand correctly, testing these changes is anything but straightforward... ☹️

• EMAIL: Testing requires a functioning mail setup, e.g. a mock mail server.
• CIRCLE, TALK_CONVERSATION: Testing requires
  • the "Circle" and "Talk" apps to be installed in the test setup and
  • an API endpoint to create circles and Talk conversations to test against.

[luffah]

Indeed, not really simple to test the apps connections.

• About mail, it shall be possible to do that in the test container with a local only postfix.
• For installing Circle and Talk app, it is mainly some wget to do (e.g. with groupfolders).
• About API endpoints :
  • for Circle app, the relative requests shall be coded in wrapper to be test. The info can be found here https://github.com/nextcloud/circles/blob/master/appinfo/routes.php
  • for Talk app, i don't know where to find the source code.

What you add don't require to test the apps, but to ensure no regression.
For understanding, this shall be documented that the functionality have to be coded.

[svierne]

Just to clarify that I've understood correctly: What you're saying is that this PR is good to merge if I include a comment documenting that tests have yet to be written for the mail, Circle and Talk share types? 🤔

[luffah]

Just to clarify that I've understood correctly: What you're saying is that this PR is good to merge if I include a comment documenting that tests have yet to be written for the mail, Circle and Talk share types? thinking

Documentation shall be in the docstring. I add a review.

But yes, if there is no regression, the PR will be merged.

Anyway, this branch is is not the main branch.
Do you need to have this included in the pip package ?

[svierne]

Do you need to have this included in the pip package ?

Actually yes, that would be awesome. But I also need a single method from the OCS Sharee API, so updating it now wouldn't help me unfortunately. I have a local implementation of this wrapper with only a single method inside (the one I need)... I don't know if that's really worth a PR. 😅

Edit: Nevermind, I just saw that the whole Sharee API has only two methods. I'll just implement them both and open up another PR. 🙂

[luffah]

Still this PR relevant ?

There is some review opened.

[svierne]

Oh, sorry for the long delay! Yes, support for the new share types would be really nice to have. Right now I'm still using my patched version of the package. What's left to do to get this merged? 🙂

Concerning the Sharee API, I've now set a reminder in my calendar and should be able to open the second PR in the next two weeks.

Edit: Unfortunately, as I've chosen the develop branch for this PR, I'm unable to open the second PR against luffah:develop... and GitHub won't allow a second fork. >_< But I'll open the PR asap when we're done with this one (and I can recreate the fork and NOT commit my changes directly to the develop branch...).

[kalikaneko]

@luffah: what is the status of this PR? I could help with any tasks that are blocking a merge