Skip to content

Add email validation#28507

Merged
PVince81 merged 3 commits into
masterfrom
validate_public_link_mail
Aug 3, 2017
Merged

Add email validation#28507
PVince81 merged 3 commits into
masterfrom
validate_public_link_mail

Conversation

@lordelix

@lordelix lordelix commented Jul 27, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Add email validation for public link creation
  • show email field even if the model !isNew

Related Issue

Fixes #28451

How was this tested

  • Manually tested (visual test) on latest Firefox, Chrome on MacOS.
  • Manually tested (visual test) on latest Firefox, Chrome, IE1 and Edge on Windows.
  • Changed and added to Jasmine test.

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.

@lordelix
Add email validation,
e5b10e1 
show email field even if the model !isNew
@lordelix lordelix requested a review from PVince81 July 27, 2017 10:13
@lordelix
execute done() no matter the promise outcome;
e613f76 
(Creating share link is done even if mail fails)
@lordelix lordelix changed the title Add email validation, Add email validation Jul 27, 2017
@lordelix

lordelix commented Jul 27, 2017

Copy link
Copy Markdown
Contributor Author

@PVince81 pls have a look.

@PVince81

PVince81 commented Jul 31, 2017

Copy link
Copy Markdown
Contributor

@felixheidecke can you list the test cases you tested manually ? (it's the "How was this tested" section in the PR template)

Code looks good

@lordelix

lordelix commented Aug 3, 2017

Copy link
Copy Markdown
Contributor Author

@PVince81 updated and added tests :-)

PVince81
PVince81 approved these changes Aug 3, 2017
View reviewed changes

@PVince81 PVince81 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@PVince81

PVince81 commented Aug 3, 2017

Copy link
Copy Markdown
Contributor

restarted jenkins build

@PVince81

PVince81 commented Aug 3, 2017

Copy link
Copy Markdown
Contributor

Forget about Jenkins... git fetch failures...

I ran the JS tests locally and they all passed, merging.

@felixheidecke please backport to stable10

@PVince81 PVince81 merged commit b5a2f86 into master Aug 3, 2017
@PVince81 PVince81 deleted the validate_public_link_mail branch August 3, 2017 09:49
@DeepDiver1975

DeepDiver1975 commented Sep 18, 2017

Copy link
Copy Markdown
Member

this got lost because no labels and no milestone was set ... damn ... is it that hard?

@PVince81

PVince81 commented Sep 18, 2017

Copy link
Copy Markdown
Contributor

@felixheidecke please backport as soon as possible

@lordelix lordelix mentioned this pull request Sep 18, 2017
Merged
PVince81
PVince81 reviewed Nov 28, 2017
View reviewed changes
Comment thread core/js/sharedialogmailview.js
var itemSource = this.itemModel.get('itemSource');

if (!this.validateEmail(recipientEmail)) {
deferred.reject();

@PVince81 PVince81 Nov 28, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should return here to avoid sending the ajax request below

@lock

lock Bot commented Aug 1, 2019

Copy link
Copy Markdown

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.

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@PVince81 PVince81 PVince81 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

3 - To Review 4 - To release comp:core comp:core-js feature:update status/STALE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Validate email before sending out public link from dialog

4 participants

@lordelix @PVince81 @DeepDiver1975 @ownclouders