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

Make remote sharee search configurable #40577

Closed

Conversation

michielbdejong
Copy link
Contributor

@michielbdejong michielbdejong commented Jan 13, 2023

Description

When the 'sciencemesh' app is installed, use it instead of the federatedfilesharing app to
find sharee matches for OCM sharing.

Related Issue

Motivation and Context

This is needed for OC-10 instances that want to join https://sciencemesh.io

How Has This Been Tested?

  • test environment:
    • GitPod from https://github.com/cs3org/ocm-test-suite
    • run ./gitpod-init.sh
    • run ./scripts/clean.sh
    • run ./scripts/orro-testing.sh
    • go the Firefox browser-in-a-browser that is exposed to port 5800
    • in there, navigate to https://oc1.docker (einstein / relativity)
    • create a local user 'john'
    • using the ScienceMesh app, generate an invite
    • accept the invite on oc2.docker (marie / radioactivity)
    • confirm that marie is now a ScienceMesh contact of einstein on oc1.docker
  • test case 1: sharing via ScienceMesh
    • as einstein on oc1.docker, share a file and start typing "m..." as the sharee
    • as marie on oc2.docker, accept the share
    • check the revaoc1.docker logs and see that the sharing happened via oc1 -> revaoc1 -> revaoc2 -> oc2
  • test case 2: sharing via local
    • as einstein on oc1.docker, share a file and start typing "jo..." as the sharee
    • it should offer to share with John as a local user

Screenshots (if appropriate):

Screenshot 2023-01-13 at 13 53 37

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised
  • Changelog item, see TEMPLATE

@michielbdejong
Copy link
Contributor Author

I'll try to add some tests!

@michielbdejong
Copy link
Contributor Author

phpstan complains: "Instantiated class OCA\ScienceMesh\Plugins\ScienceMeshSearchPlugin not found."
It would be nice if phpstan would support a way to disable this warning at this specific line.
Not sure how to deal with this, will try to think of a solution.

@michielbdejong
Copy link
Contributor Author

The ShareesController doesn't have any unit or acceptance tests, so I'm unable to add them for this PR.

@michielbdejong michielbdejong marked this pull request as ready for review January 16, 2023 13:38
@michielbdejong michielbdejong marked this pull request as draft January 16, 2023 13:44
@michielbdejong michielbdejong changed the title OCM via ScienceMesh Make remote sharee search configurable Jan 16, 2023
@michielbdejong
Copy link
Contributor Author

phpstan fixed by copying the pattern from https://github.com/owncloud/core/blob/v10.11.0/lib/private/Server.php#L896

@michielbdejong michielbdejong marked this pull request as ready for review January 16, 2023 13:56
@michielbdejong
Copy link
Contributor Author

I got my dev env into a state where I can run bash tests/drone/test-phpunit.sh - checking it now.

@michielbdejong
Copy link
Contributor Author

Can someone re-run "apiShareManagementToRoot-mariadb10.2-php7.4"?
It looks like it might be an intermittent failure: "Step is killed (canceled)"

@phil-davis
Copy link
Contributor

Can someone re-run "apiShareManagementToRoot-mariadb10.2-php7.4"? It looks like it might be an intermittent failure: "Step is killed (canceled)"

The pipeline step hung for more than 4 hours. I guess drone eventually killed it. It looks like some drone agent problem happened.

I restarted the whole drone CI.

@michielbdejong
Copy link
Contributor Author

That helped, thanks a lot!

@phil-davis can you also do the review maybe, or should we try to find someone else available?

@phil-davis
Copy link
Contributor

@michielbdejong see #40587 - we can actually use a default call to getSystemValue. The problem was "just" that the mocking in the unit tests needs to know about config value sharing.remoteShareesSearch

Maybe you could cherry-pick the commit from there, and squash all the commits in this PR, and we can get it reviewed.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

One tiny thing!

changelog/unreleased/40577 Outdated Show resolved Hide resolved
@phil-davis phil-davis self-requested a review January 19, 2023 12:40
@jvillafanez
Copy link
Member

Technically, I'm ok with the solution. My worry is that neither core nor any ownCloud app uses that feature, so it's possible that we see it as dead code in the future. Any idea how we can make it more visible to prevent the feature from being removed? Maybe adding a comment saying that it's being used in the 3rd party sciencemesh app could be enough.

Another thing that worries me is that there is no documentation available about the expectation of the plugin class. If I (or anyone else) want to provide a custom implementation, I simply can't without seeing your code. I also don't know if your code works as expected. I mean, it could return some results, but I don't know if it should return more results or not.

It might be better if ownCloud core provides some kind of interface that the sciencemesh app can implement, and take advantage of the interface to document the expected behavior.
As far as I know, this should be only creating the interface somewhere in the "lib/public" folder (need to find a good place there), and the sciencemesh app just needs to implement the interface. You might want to adjust this PR a bit in order to ensure that the class being loaded implements the interface.

@michielbdejong
Copy link
Contributor Author

neither core nor any ownCloud app uses that feature, so it's possible that we see it as dead code in the future

Ah no, but it's used by https://github.com/pondersource/oc-sciencemesh which will be installed on many production systems in various universities and research institutes across Europe.

$pluginClass = $this->config->getSystemValue('sharing.remoteShareesSearch');
if ($pluginClass !== '') {
$this->result['remotes'] = [];
$plugin = new $pluginClass($this->config, $this->userManager, $this->userSession);
Copy link
Member

Choose a reason for hiding this comment

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

Ctor args might change over time.
Can we use dependency injection here?

@michielbdejong
Copy link
Contributor Author

It might be better if ownCloud core provides some kind of interface that the sciencemesh app can implement, and take advantage of the interface to document the expected behavior.

Good idea! I can add that.

@jvillafanez
Copy link
Member

It might be better if ownCloud core provides some kind of interface that the sciencemesh app can implement, and take advantage of the interface to document the expected behavior.

Good idea! I can add that.

In addition, make sure there is some kind of reference that the interface is being used and implemented by 3rd party apps so we know that we need to be careful with it. We might want to change the interface at some point and that would break your app, so a warning would help to prevent that problem in the future.

Signed-off-by: Michiel de Jong <michiel@unhosted.org>
@phil-davis
Copy link
Contributor

code-style is complaining. It needs:
make test-php-style-fix
and then commit and push the changes.

I tried to do it, but I can't push to this branch in pondersource repo.

Signed-off-by: Michiel de Jong <michiel@unhosted.org>
@michielbdejong
Copy link
Contributor Author

Ah yes, b8f9fdf broke it! Sorry.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

25.0% 25.0% Coverage
0.0% 0.0% Duplication

@michielbdejong
Copy link
Contributor Author

In https://drone.owncloud.com/owncloud/core/37988/28/2 the 'skip-if-unchanged' step of Sonar failed, apparently unrelated to this PR. I'll try what happens if I push a whitespace change.

Signed-off-by: Michiel de Jong <michiel@unhosted.org>
@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiFederationToShares1-latest-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/37990/100

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiFederationToShares1-git-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/37990/99

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiSharingNotificationsToRoot-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/37990/91

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiWebdavUpload2-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/37990/89

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline cliBackground-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/37990/106

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiWebdavUpload1-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/37990/88

@michielbdejong
Copy link
Contributor Author

@phil-davis what is the next step now?

@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/core/37990/88/6
Error response from daemon: No such image: owncloudci/php:7.4
Stupid drone+docker gets it wrong. I have restarted CI.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM

@phil-davis
Copy link
Contributor

@DeepDiver1975 please review again. IMO this is ready to merge.

@michielbdejong
Copy link
Contributor Author

@DeepDiver1975 Please merge this today if possible.

@phil-davis
Copy link
Contributor

The CI fail is https://drone.owncloud.com/owncloud/core/38013/28/2
sonar-analysis

latest: Pulling from owncloudci/drone-skip-pipeline
Digest: sha256:78d133bf897c26ab34c17839d1ffe86b15174f12370745cbab8b949098620174
Status: Image is up to date for owncloudci/drone-skip-pipeline:latest
DRONE_COMMIT_BEFORE:  13d0a217480ceba2846037ad782624e6645bb391
time="2023-02-24T04:51:17Z" level=error msg="execution failed: commit from DRONE_COMMIT_BEFORE not found: object not found"

I suppose that will be something to do with the fork that this branch comes from and???

But all tests passed.

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Mar 2, 2023

Yes, Sonar wants you to put the { on the next line. I fixed that in a commit last week, but it turned out that when you fix Sonar then make test-php-style-fix breaks and vice versa, so then I reverted that.

@michielbdejong
Copy link
Contributor Author

@DeepDiver1975 ? Anyone else?

@michielbdejong michielbdejong mentioned this pull request Mar 3, 2023
11 tasks
@jnweiger
Copy link
Contributor

jnweiger commented Mar 3, 2023

https://drone.owncloud.com/owncloud/core/38013
explodes in sonar analysis with level=error msg="execution failed: commit from DRONE_COMMIT_BEFORE not found: object not found"
Never seen that before.

For inclusion in 10.12.0, please redirect this PR to the release-10.12.0 branch (instead of master)

@phil-davis phil-davis mentioned this pull request Mar 4, 2023
11 tasks
@phil-davis
Copy link
Contributor

For inclusion in 10.12.0, please redirect this PR to the release-10.12.0 branch (instead of master)

See PR #40667

@phil-davis
Copy link
Contributor

Merged in #40667

@phil-davis phil-davis closed this Mar 6, 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.

Support sharee search for the sciencemesh app
7 participants