Replace notify user for local shares with button by PVince81 · Pull Request #29366 · owncloud/core

PVince81 · 2017-10-26T14:01:12Z

Description

Replace the checkbox with a button.
Changed logic to only send the mail_send flag if email could be sent to at least one recipient.

Related Issue

Fixes #29318

Motivation and Context

See ticket

How Has This Been Tested?

Manual testing so far

Screenshots (if appropriate):

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)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

TODOs:

BUG: fix spinner position
TODO: add/adjust unit tests
TODO: fading tickmark on success?!

codecov · 2017-10-26T14:22:19Z

Codecov Report

Merging #29366 into master will decrease coverage by <.01%.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##             master   #29366      +/-   ##
============================================
- Coverage     60.23%   60.23%   -0.01%     
- Complexity    17186    17187       +1     
============================================
  Files          1030     1030              
  Lines         57273    57274       +1     
============================================
  Hits          34500    34500              
- Misses        22773    22774       +1

Impacted Files	Coverage Δ	Complexity Δ
core/ajax/share.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Share/MailNotifications.php	`88.23% <100%> (ø)`	`15 <0> (+1)`	⬆️

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 f4b3388...52cbc09. Read the comment docs.

codecov · 2017-10-26T14:22:19Z

Codecov Report

Merging #29366 into master will increase coverage by <.01%.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##             master   #29366      +/-   ##
============================================
+ Coverage     60.45%   60.45%   +<.01%     
- Complexity    17222    17223       +1     
============================================
  Files          1032     1032              
  Lines         57360    57361       +1     
============================================
+ Hits          34677    34679       +2     
+ Misses        22683    22682       -1

Impacted Files	Coverage Δ	Complexity Δ
core/ajax/share.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Share/MailNotifications.php	`90.58% <100%> (+2.35%)`	`15 <0> (+1)`	⬆️

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 89d3432...094bc76. Read the comment docs.

PVince81 · 2017-10-30T10:31:26Z

note: cannot increase code coverage for ajax/share.php... we need to refactor that to app framework eventually if we don't manage to kill this checkbox eventually...

jvillafanez · 2017-10-30T13:20:01Z

-			\OCP\Share::setSendMailStatus($itemType, $itemSource, $shareType, $recipient, true);
+			// if we were able to send to at least one recipient, mark as sent
+			// allowing the user to resend would spam users who already got a notification
+			if (count($result) < count($recipientList)) {


count($result) could be 0. Do we still want to set the status as sent in this scenario?

Yes, because 0 means no errors. The array $result only contains a list of email addresses for which an email could not be sent.

lordelix · 2017-11-02T10:16:24Z

Well … this is still ugly a. f. but simply repositioning the button won't help.
I'll go ahead and open an issue for visual improvement.

lordelix

It's on the brink of not being okay from a UI perspective. Will be addressed in a different issue.

PVince81 · 2017-11-06T09:22:27Z

increase coverage a tiny bit more.

will not bother about the ajax/* case

PVince81 · 2017-11-06T09:22:44Z

@jvillafanez your seal of approval for the PHP code ?

PVince81 · 2017-11-06T13:47:22Z

ignoring missing coverage for ajax/* file

PVince81 · 2017-11-06T13:48:43Z

stable10: #29463

lock · 2019-08-02T01:02:31Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

PVince81 added the 3 - To Review label Oct 26, 2017

PVince81 added this to the development milestone Oct 26, 2017

PVince81 self-assigned this Oct 26, 2017

PVince81 mentioned this pull request Oct 26, 2017

Change "notify by mail" checkbox to a button with user feedback #29318

Closed

3 tasks

PVince81 force-pushed the notify-checkbox-to-button branch from 6957f2d to 52cbc09 Compare October 26, 2017 14:22

PVince81 force-pushed the notify-checkbox-to-button branch 2 times, most recently from d048355 to 6347871 Compare October 30, 2017 10:30

PVince81 requested a review from jvillafanez October 30, 2017 10:31

jvillafanez reviewed Oct 30, 2017

View reviewed changes

lordelix self-requested a review November 3, 2017 13:46

lordelix approved these changes Nov 3, 2017

View reviewed changes

Replace notify user for local shares with button

094bc76

PVince81 force-pushed the notify-checkbox-to-button branch from 6347871 to 094bc76 Compare November 6, 2017 09:21

PVince81 merged commit 497327d into master Nov 6, 2017

PVince81 deleted the notify-checkbox-to-button branch November 6, 2017 13:47

PVince81 mentioned this pull request Nov 6, 2017

[stable10] Replace notify user for local shares with button #29463

Merged

ownclouders added the status/STALE label Dec 20, 2017

lock Bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace notify user for local shares with button#29366

Replace notify user for local shares with button#29366
PVince81 merged 1 commit into
masterfrom
notify-checkbox-to-button

PVince81 commented Oct 26, 2017

Uh oh!

codecov Bot commented Oct 26, 2017

Uh oh!

codecov Bot commented Oct 26, 2017 •

edited

Loading

Uh oh!

PVince81 commented Oct 30, 2017

Uh oh!

jvillafanez Oct 30, 2017

Uh oh!

PVince81 Nov 2, 2017

Uh oh!

lordelix commented Nov 2, 2017

Uh oh!

lordelix left a comment

Uh oh!

PVince81 commented Nov 6, 2017

Uh oh!

PVince81 commented Nov 6, 2017

Uh oh!

PVince81 commented Nov 6, 2017

Uh oh!

PVince81 commented Nov 6, 2017

Uh oh!

lock Bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

PVince81 commented Oct 26, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

TODOs:

Uh oh!

codecov Bot commented Oct 26, 2017

Codecov Report

Uh oh!

codecov Bot commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PVince81 commented Oct 30, 2017

Uh oh!

jvillafanez Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 2, 2017

Choose a reason for hiding this comment

Uh oh!

lordelix commented Nov 2, 2017

Uh oh!

lordelix left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Nov 6, 2017

Uh oh!

PVince81 commented Nov 6, 2017

Uh oh!

PVince81 commented Nov 6, 2017

Uh oh!

PVince81 commented Nov 6, 2017

Uh oh!

lock Bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Oct 26, 2017 •

edited

Loading