X Tutup
Skip to content

Rewire notify by email#33180

Merged
VicDeo merged 6 commits intomasterfrom
fix-32494
Oct 17, 2018
Merged

Rewire notify by email#33180
VicDeo merged 6 commits intomasterfrom
fix-32494

Conversation

@VicDeo
Copy link
Member

@VicDeo VicDeo commented Oct 12, 2018 •
edited
Loading

Description

  • Get rid of legacy API::register invocations in favor of routing
  • Kill an ugly OCSShareWrapper
  • Refactor Share20OCS to be a controller
  • Move 2 endpoints from core/ajax/share.php into the new controller
  • Rewrite the implementation for these endpoints in order not to use legacy sharing methods

Related Issue

Motivation and Context

Sharing code cleanup

How Has This Been Tested?

  1. share with user

  2. click notify by email

  3. check that email has been arrived

  4. reload the page and check that there is no notify by email button any more

  5. Login as user and check that the emailed permalink works

  6. share with group

  7. click notify by email

  8. check that email for all users with emails have been arrived

  9. reload the page and check that there is no notify by email button any more

  10. Login as any user from this group and check that permalink works (TBH I didn't test them all)

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

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)
    Since it doesn't beak anything a backport would be nice

@codecov
Copy link

codecov bot commented Oct 12, 2018 •
edited
Loading

Codecov Report

Merging #33180 into master will increase coverage by 0.02%.
The diff coverage is 66.31%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #33180      +/-   ##
============================================
+ Coverage     64.18%   64.21%   +0.02%     
- Complexity    18700    18701       +1     
============================================
  Files          1181     1180       -1     
  Lines         70349    70302      -47     
  Branches       1270     1271       +1     
============================================
- Hits          45156    45143      -13     
+ Misses        24823    24789      -34     
  Partials        370      370
Flag Coverage Δ Complexity Δ
#javascript 52.88% <50%> (-0.02%) 0 <0> (ø)
#phpunit 65.5% <66.66%> (+0.03%) 18701 <50> (+1) ⬆️
Impacted Files Coverage Δ Complexity Δ
apps/files_sharing/lib/Notifier.php 100% <ø> (ø) 19 <0> (ø) ⬇️
apps/files_sharing/appinfo/routes.php 97.36% <ø> (-1.14%) 0 <0> (ø)
core/js/shareitemmodel.js 77.77% <0%> (-0.59%) 0 <0> (ø)
core/ajax/share.php 0% <0%> (ø) 0 <0> (ø) ⬇️
apps/files_sharing/lib/AppInfo/Application.php 50.98% <100%> (+9.11%) 17 <1> (+1) ⬆️
lib/private/Share20/DefaultShareProvider.php 98.33% <100%> (ø) 114 <0> (ø) ⬇️
core/js/sharedialogshareelistview.js 78.84% <100%> (ø) 0 <0> (ø) ⬇️
...es_sharing/lib/Controller/Share20OcsController.php 85.23% <61.58%> (ø) 192 <39> (?)
lib/private/Share/MailNotifications.php 97.46% <84.61%> (-2.54%) 47 <10> (ø)

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 88f87a5...129d5e1. Read the comment docs.

@VicDeo VicDeo force-pushed the fix-32494 branch 4 times, most recently from 3628b2a to 59e3478 Compare October 12, 2018 21:35
@VicDeo VicDeo mentioned this pull request Oct 15, 2018
Closed
PVince81
PVince81 reviewed Oct 15, 2018
View reviewed changes
apps/files_sharing/lib/AppInfo/Application.php
});

$container->registerService('Share20OcsController', function (SimpleContainer $c) use ($server) {
return new Share20OcsController(
Copy link
Contributor

@PVince81 PVince81 Oct 15, 2018

Choose a reason for hiding this comment

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

I guess automagic DI does not work here due to complex arguments ?

Copy link
Member Author

@VicDeo VicDeo Oct 15, 2018

Choose a reason for hiding this comment

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

@PVince81 it can't resolve \OCP\IUser to inject current user properly

@VicDeo VicDeo added this to the development milestone Oct 15, 2018
@VicDeo VicDeo self-assigned this Oct 15, 2018
@PVince81 PVince81 mentioned this pull request Oct 15, 2018
Closed
@VicDeo VicDeo force-pushed the fix-32494 branch 8 times, most recently from 33a18b9 to 746978d Compare October 16, 2018 18:40
@VicDeo VicDeo changed the title [WIP] Rewire notify by email Rewire notify by email Oct 16, 2018
@VicDeo VicDeo requested a review from DeepDiver1975 October 16, 2018 18:53
@VicDeo
Copy link
Member Author

VicDeo commented Oct 16, 2018

Ready for review

PVince81
PVince81 reviewed Oct 16, 2018
View reviewed changes
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 check why codecov is complaining (good luck getting an actual report page if it's in the right mood).

Also please state how you confirmed that it still worked as required by the issue template. Would be good to confirm that the button as clicked in the web UI still behaves as before.

@PVince81
Copy link
Contributor

PVince81 commented Oct 17, 2018

ok, so codecov issue related to the code that declares the new routes, not testable.
routes should already be covered by acceptance API tests

PVince81
PVince81 approved these changes Oct 17, 2018
View reviewed changes
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.

👍

@VicDeo VicDeo force-pushed the fix-32494 branch 2 times, most recently from 51ccc9e to c8798bc Compare October 17, 2018 19:59
@VicDeo
Copy link
Member Author

VicDeo commented Oct 17, 2018

66.31% of diff hit (target 64.18%)

Go go go!

@VicDeo VicDeo merged commit 777fa69 into master Oct 17, 2018
@VicDeo VicDeo deleted the fix-32494 branch October 17, 2018 21:25
@PVince81 PVince81 mentioned this pull request Dec 11, 2018
Merged
2 tasks
@DeepDiver1975 DeepDiver1975 mentioned this pull request Mar 7, 2019
Closed
DeepDiver1975 pushed a commit that referenced this pull request Mar 18, 2019
@VicDeo @DeepDiver1975
Merge pull request #33180 from owncloud/fix-32494
29f8dce 
Rewire notify by email
@DeepDiver1975 DeepDiver1975 mentioned this pull request Mar 18, 2019
Merged
DeepDiver1975 pushed a commit that referenced this pull request Mar 18, 2019
@VicDeo @DeepDiver1975
Merge pull request #33180 from owncloud/fix-32494
5ee2ac5 
Rewire notify by email
PVince81 pushed a commit that referenced this pull request Mar 18, 2019
Merge pull request #34786 from owncloud/stable10-777fa69a9f05c28b5e0e…
c883292 
…4a548ee395127803e8fe

[stable10] Backport of #34622 and #33180
@dpakach dpakach mentioned this pull request Apr 24, 2019
Closed
@PVince81 PVince81 mentioned this pull request May 14, 2019
Merged
11 tasks
@VicDeo VicDeo mentioned this pull request May 15, 2019
Merged
10 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Oct 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

1 more reviewer

@PVince81 PVince81 PVince81 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@VicDeo VicDeo

Labels

3 - To Review technical debt

Projects

None yet

Milestone

development

Development

Successfully merging this pull request may close these issues.

Rewire button to notify users for local shares to use notifications API

2 participants

@VicDeo @PVince81
X Tutup