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

Cleanup federation phase 1 #32086

Merged
merged 5 commits into from
Jul 20, 2018
Merged

Cleanup federation phase 1 #32086

merged 5 commits into from
Jul 20, 2018

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Jul 18, 2018

Description

The first part of federatedfileshare code cleanup

  • Get rid of some deprecated classes in the code
  • Get rid of some unused classes in the code
  • Cleanup routing
  • Provides an ability to bypass changing /ocs/routes.php and set up absolute ocs routes in the appinfo/routes.php

Related Issue

https://github.com/owncloud/enterprise/issues/2564

Motivation and Context

Clean and readable code

How Has This Been Tested?

  1. Created an incoming share from another instance at master branch
  2. Accepting|Declining|Unsharing it

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@VicDeo VicDeo self-assigned this Jul 18, 2018
@VicDeo VicDeo added this to the development milestone Jul 18, 2018
@VicDeo VicDeo force-pushed the cleanup-federation-phase-1 branch from 238fe18 to 9350772 Compare July 18, 2018 15:38
@codecov
Copy link

codecov bot commented Jul 18, 2018

Codecov Report

Merging #32086 into master will increase coverage by 0.02%.
The diff coverage is 53.93%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #32086      +/-   ##
============================================
+ Coverage     63.85%   63.87%   +0.02%     
- Complexity    18556    18557       +1     
============================================
  Files          1169     1171       +2     
  Lines         69743    69753      +10     
  Branches       1267     1267              
============================================
+ Hits          44535    44558      +23     
+ Misses        24839    24826      -13     
  Partials        369      369
Flag Coverage Δ Complexity Δ
#javascript 52.81% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.14% <53.93%> (+0.02%) 18557 <80> (+1) ⬆️
Impacted Files Coverage Δ Complexity Δ
core/routes.php 65.51% <ø> (ø) 0 <0> (ø) ⬇️
ocs/routes.php 100% <ø> (ø) 0 <0> (ø) ⬇️
apps/dav/appinfo/v1/publicwebdav.php 0% <0%> (ø) 0 <0> (ø) ⬇️
apps/federatedfilesharing/appinfo/app.php 20.83% <0%> (ø) 0 <0> (ø) ⬇️
apps/files_sharing/ajax/shareinfo.php 0% <0%> (ø) 0 <0> (ø) ⬇️
apps/files_sharing/ajax/external.php 0% <0%> (ø) 0 <0> (ø) ⬇️
apps/federatedfilesharing/appinfo/routes.php 100% <100%> (ø) 0 <0> (?)
lib/private/Route/Router.php 68.75% <100%> (+3.12%) 54 <0> (ø) ⬇️
lib/private/AppFramework/Routing/RouteConfig.php 95.74% <100%> (+0.14%) 31 <0> (+1) ⬆️
apps/files_sharing/lib/ShareBackend/File.php 24.77% <100%> (ø) 35 <0> (ø) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f8fb04...df565a0. Read the comment docs.

@VicDeo VicDeo force-pushed the cleanup-federation-phase-1 branch 6 times, most recently from 618847f to 13bdf62 Compare July 19, 2018 17:59
@VicDeo VicDeo force-pushed the cleanup-federation-phase-1 branch from 13bdf62 to 48e87c8 Compare July 19, 2018 18:03
@VicDeo VicDeo force-pushed the cleanup-federation-phase-1 branch from 48e87c8 to 9c695cc Compare July 19, 2018 21:32
$args = \OC\Files\Filesystem::is_dir($file)
? ['dir' => $file]
: ['dir' => \dirname($file), 'scrollto' => $file];
$link = \OCP\Util::linkToAbsolute('files', 'index.php', $args);
Copy link
Contributor

Choose a reason for hiding this comment

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

note for later: we could use permalink here, see ViewController how to generate one

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Awesome!

Please adjust the exception catch block I pointed to

}
} catch (\Exception $e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

better catch Share\Exceptions\ShareNotFound then and let the other through for proper error reporting ?

}
$this->fedShareManager->declineShare($share);
} catch (\Exception $e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -32,7 +32,7 @@

$l = \OC::$server->getL10N('files_sharing');

$federatedSharingApp = new \OCA\FederatedFileSharing\AppInfo\Application('federatedfilesharing');
$federatedSharingApp = new \OCA\FederatedFileSharing\AppInfo\Application();
Copy link
Contributor

Choose a reason for hiding this comment

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

sad to see we still have ajax endpoints, oh well

@VicDeo
Copy link
Member Author

VicDeo commented Jul 20, 2018

@PVince81 updated

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Contributor

As discussed, unit tests will be added in the next round

@PVince81 PVince81 merged commit 91df00d into master Jul 20, 2018
@PVince81 PVince81 deleted the cleanup-federation-phase-1 branch July 20, 2018 16:20
@PVince81
Copy link
Contributor

no backport for now

@VicDeo VicDeo mentioned this pull request Jul 23, 2018
10 tasks
@VicDeo
Copy link
Member Author

VicDeo commented Oct 5, 2018

Stable10: #33027

@rullzer rullzer mentioned this pull request Oct 10, 2018
19 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants