Skip to content

Commit

Permalink
Merge revert email retry from master to dev
Browse files Browse the repository at this point in the history 
* Revert "Don't throw when SmtpUri is not set (#6278)"

This reverts commit e60e75f.

* Revert "Fix logging exceptions from background thread (#6273)"

This reverts commit d1610bb.

* Revert "Improve email sending (#6154)"

This reverts commit 2616a99.
  • Loading branch information
@zivkan
zivkan authored Aug 10, 2018
1 parent 3805e2e commit 9b00ef8
Show file tree
Hide file tree
Showing 29 changed files with 440 additions and 695 deletions.
80 changes: 26 additions & 54 deletions src/NuGetGallery.Core/Services/CoreMessageService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.ObjectModel;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Net.Mail;
using System.Text;
using System.Threading.Tasks;
using AnglicanGeek.MarkdownMailer;
using NuGet.Services.Validation;
using NuGet.Services.Validation.Issues;
Expand All @@ -17,11 +15,9 @@ namespace NuGetGallery.Services
{
public class CoreMessageService : ICoreMessageService
{
private static readonly ReadOnlyCollection<TimeSpan> RetryDelays = Array.AsReadOnly(new[] {
TimeSpan.FromSeconds(0.1),
TimeSpan.FromSeconds(1),
TimeSpan.FromSeconds(10)
});
protected CoreMessageService()
{
}

public CoreMessageService(IMailSender mailSender, ICoreMessageServiceConfiguration coreConfiguration)
{
Expand All @@ -32,7 +28,7 @@ public CoreMessageService(IMailSender mailSender, ICoreMessageServiceConfigurati
public IMailSender MailSender { get; protected set; }
public ICoreMessageServiceConfiguration CoreConfiguration { get; protected set; }

public async Task SendPackageAddedNoticeAsync(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl)
public void SendPackageAddedNotice(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl)
{
string subject = $"[{CoreConfiguration.GalleryOwner.DisplayName}] Package published - {package.PackageRegistration.Id} {package.Version}";
string body = $@"The package [{package.PackageRegistration.Id} {package.Version}]({packageUrl}) was recently published on {CoreConfiguration.GalleryOwner.DisplayName} by {package.User.Username}. If this was not intended, please [contact support]({packageSupportUrl}).
Expand All @@ -53,12 +49,12 @@ [change your email notification settings]({emailSettingsUrl}).

if (mailMessage.To.Any())
{
await SendMessageAsync(mailMessage);
SendMessage(mailMessage);
}
}
}

public async Task SendPackageValidationFailedNoticeAsync(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl)
public void SendPackageValidationFailedNotice(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl)
{
var validationIssues = validationSet.GetValidationIssues();

Expand Down Expand Up @@ -99,7 +95,7 @@ public async Task SendPackageValidationFailedNoticeAsync(Package package, Packag

if (mailMessage.To.Any())
{
await SendMessageAsync(mailMessage);
SendMessage(mailMessage, copySender: false);
}
}
}
Expand Down Expand Up @@ -135,7 +131,7 @@ private static string ParseValidationIssue(ValidationIssue validationIssue, stri
}
}

public async Task SendValidationTakingTooLongNoticeAsync(Package package, string packageUrl)
public void SendValidationTakingTooLongNotice(Package package, string packageUrl)
{
string subject = "[{0}] Package validation taking longer than expected - {1} {2}";
string body = "It is taking longer than expected for your package [{1} {2}]({3}) to get published.\n\n" +
Expand Down Expand Up @@ -167,7 +163,7 @@ public async Task SendValidationTakingTooLongNoticeAsync(Package package, string

if (mailMessage.To.Any())
{
await SendMessageAsync(mailMessage);
SendMessage(mailMessage, copySender: false);
}
}
}
Expand Down Expand Up @@ -197,54 +193,30 @@ protected static void AddOwnersSubscribedToPackagePushedNotification(PackageRegi
}
}

protected virtual async Task SendMessageAsync(MailMessage mailMessage)
protected void SendMessage(MailMessage mailMessage)
{
int attempt = 0;
bool success = false;
while (!success)
{
try
{
await AttemptSendMessageAsync(mailMessage, attempt + 1);
success = true;
}
catch (SmtpException)
{
if (attempt < RetryDelays.Count)
{
await Task.Delay(RetryDelays[attempt]);
attempt++;
}
else
{
throw;
}
}
}
SendMessage(mailMessage, copySender: false);
}

protected virtual Task AttemptSendMessageAsync(MailMessage mailMessage, int attemptNumber)
virtual protected void SendMessage(MailMessage mailMessage, bool copySender)
{
// AnglicanGeek.MarkdownMailer doesn't have an async overload
MailSender.Send(mailMessage);
return Task.CompletedTask;
}

protected async Task SendMessageToSenderAsync(MailMessage mailMessage)
{
using (var senderCopy = new MailMessage(
CoreConfiguration.GalleryOwner,
mailMessage.ReplyToList.First()))
if (copySender)
{
senderCopy.Subject = mailMessage.Subject + " [Sender Copy]";
senderCopy.Body = string.Format(
CultureInfo.CurrentCulture,
"You sent the following message via {0}: {1}{1}{2}",
CoreConfiguration.GalleryOwner.DisplayName,
Environment.NewLine,
mailMessage.Body);
var senderCopy = new MailMessage(
CoreConfiguration.GalleryOwner,
mailMessage.ReplyToList.First())
{
Subject = mailMessage.Subject + " [Sender Copy]",
Body = string.Format(
CultureInfo.CurrentCulture,
"You sent the following message via {0}: {1}{1}{2}",
CoreConfiguration.GalleryOwner.DisplayName,
Environment.NewLine,
mailMessage.Body),
};
senderCopy.ReplyToList.Add(mailMessage.ReplyToList.First());
await SendMessageAsync(senderCopy);
MailSender.Send(senderCopy);
}
}
}
Expand Down
7 changes: 3 additions & 4 deletions src/NuGetGallery.Core/Services/ICoreMessageService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using NuGet.Services.Validation;
using System.Threading.Tasks;

namespace NuGetGallery.Services
{
public interface ICoreMessageService
{
Task SendPackageAddedNoticeAsync(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl);
Task SendPackageValidationFailedNoticeAsync(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl);
Task SendValidationTakingTooLongNoticeAsync(Package package, string packageUrl);
void SendPackageAddedNotice(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl);
void SendPackageValidationFailedNotice(Package package, PackageValidationSet validationSet, string packageUrl, string packageSupportUrl, string announcementsUrl, string twitterUrl);
void SendValidationTakingTooLongNotice(Package package, string packageUrl);
}
}
3 changes: 1 addition & 2 deletions src/NuGetGallery/App_Start/DefaultDependenciesModule.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
using NuGetGallery.Infrastructure.Authentication;
using NuGetGallery.Infrastructure.Lucene;
using NuGetGallery.Security;
using NuGetGallery.Services;
using SecretReaderFactory = NuGetGallery.Configuration.SecretReader.SecretReaderFactory;

namespace NuGetGallery
Expand Down Expand Up @@ -365,7 +364,7 @@ protected override void Load(ContainerBuilder builder)
.As<IMailSender>()
.InstancePerLifetimeScope();

builder.RegisterType<BackgroundMessageService>()
builder.RegisterType<MessageService>()
.AsSelf()
.As<IMessageService>()
.InstancePerLifetimeScope();
Expand Down
12 changes: 6 additions & 6 deletions src/NuGetGallery/Controllers/AccountsController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public virtual ActionResult ConfirmationRequired(string accountName = null)
[HttpPost]
[ActionName("ConfirmationRequired")]
[ValidateAntiForgeryToken]
public virtual async Task<ActionResult> ConfirmationRequiredPost(string accountName = null)
public virtual ActionResult ConfirmationRequiredPost(string accountName = null)
{
var account = GetAccount(accountName);

Expand All @@ -108,7 +108,7 @@ public virtual async Task<ActionResult> ConfirmationRequiredPost(string accountN
ConfirmationViewModel model;
if (!alreadyConfirmed)
{
await SendNewAccountEmailAsync(account);
SendNewAccountEmail(account);

model = new ConfirmationViewModel(account)
{
Expand All @@ -122,7 +122,7 @@ public virtual async Task<ActionResult> ConfirmationRequiredPost(string accountN
return View(model);
}

protected abstract Task SendNewAccountEmailAsync(User account);
protected abstract void SendNewAccountEmail(User account);

[UIAuthorize(allowDiscontinuedLogins: true)]
public virtual async Task<ActionResult> Confirm(string accountName, string token)
Expand Down Expand Up @@ -163,7 +163,7 @@ public virtual async Task<ActionResult> Confirm(string accountName, string token
// Change notice not required for new accounts.
if (model.SuccessfulConfirmation && !model.ConfirmingNewAccount)
{
await MessageService.SendEmailChangeNoticeToPreviousEmailAddressAsync(account, existingEmail);
MessageService.SendEmailChangeNoticeToPreviousEmailAddress(account, existingEmail);

string returnUrl = HttpContext.GetConfirmationReturnUrl();
if (!String.IsNullOrEmpty(returnUrl))
Expand Down Expand Up @@ -254,13 +254,13 @@ public virtual async Task<ActionResult> ChangeEmail(TAccountViewModel model)

if (account.Confirmed)
{
await SendEmailChangedConfirmationNoticeAsync(account);
SendEmailChangedConfirmationNotice(account);
}

return RedirectToAction(AccountAction);
}

protected abstract Task SendEmailChangedConfirmationNoticeAsync(User account);
protected abstract void SendEmailChangedConfirmationNotice(User account);

[HttpPost]
[UIAuthorize]
Expand Down
2 changes: 1 addition & 1 deletion src/NuGetGallery/Controllers/ApiController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,7 @@ await AuditingService.SaveAuditRecordAsync(
if (!(ConfigurationService.Current.AsynchronousPackageValidationEnabled && ConfigurationService.Current.BlockingAsynchronousPackageValidationEnabled))
{
// Notify user of push unless async validation in blocking mode is used
await MessageService.SendPackageAddedNoticeAsync(package,
MessageService.SendPackageAddedNotice(package,
Url.Package(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false),
Url.ReportPackage(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false),
Url.AccountSettings(relativeUrl: false));
Expand Down
8 changes: 4 additions & 4 deletions src/NuGetGallery/Controllers/AuthenticationController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ public virtual async Task<ActionResult> Register(LogOnViewModel model, string re
// Send a new account email
if (NuGetContext.Config.Current.ConfirmEmailAddresses && !string.IsNullOrEmpty(user.User.UnconfirmedEmailAddress))
{
await _messageService.SendNewAccountEmailAsync(
_messageService.SendNewAccountEmail(
user.User,
Url.ConfirmEmail(
user.User.Username,
Expand Down Expand Up @@ -325,7 +325,7 @@ public virtual ActionResult LogOff(string returnUrl)

[HttpPost]
[ValidateAntiForgeryToken]
public virtual async Task<JsonResult> SignInAssistance(string username, string providedEmailAddress)
public virtual JsonResult SignInAssistance(string username, string providedEmailAddress)
{
// If provided email address is empty or null, return the result with a formatted
// email address, otherwise send sign-in assistance email to the associated mail address.
Expand All @@ -352,7 +352,7 @@ public virtual async Task<JsonResult> SignInAssistance(string username, string p
else
{
var externalCredentials = user.Credentials.Where(cred => cred.IsExternal());
await _messageService.SendSigninAssistanceEmailAsync(new MailAddress(email, user.Username), externalCredentials);
_messageService.SendSigninAssistanceEmail(new MailAddress(email, user.Username), externalCredentials);
return Json(new { success = true });
}
}
Expand Down Expand Up @@ -674,7 +674,7 @@ private async Task<LoginUserDetails> AssociateCredential(AuthenticatedUser user)
await RemovePasswordCredential(user.User);

// Notify the user of the change
await _messageService.SendCredentialAddedNoticeAsync(user.User, _authService.DescribeCredential(result.Credential));
_messageService.SendCredentialAddedNotice(user.User, _authService.DescribeCredential(result.Credential));

return new LoginUserDetails
{
Expand Down
10 changes: 5 additions & 5 deletions src/NuGetGallery/Controllers/JsonApiController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public async Task<JsonResult> AddPackageOwner(string id, string username, string

foreach (var owner in model.Package.Owners)
{
await _messageService.SendPackageOwnerAddedNoticeAsync(owner, model.User, model.Package, packageUrl);
_messageService.SendPackageOwnerAddedNotice(owner, model.User, model.Package, packageUrl);
}
}
else
Expand Down Expand Up @@ -147,12 +147,12 @@ public async Task<JsonResult> AddPackageOwner(string id, string username, string
model.User.Username,
relativeUrl: false);

await _messageService.SendPackageOwnerRequestAsync(model.CurrentUser, model.User, model.Package, packageUrl,
_messageService.SendPackageOwnerRequest(model.CurrentUser, model.User, model.Package, packageUrl,
confirmationUrl, rejectionUrl, encodedMessage, policyMessage: string.Empty);

foreach (var owner in model.Package.Owners)
{
await _messageService.SendPackageOwnerRequestInitiatedNoticeAsync(model.CurrentUser, owner, model.User, model.Package, cancellationUrl);
_messageService.SendPackageOwnerRequestInitiatedNotice(model.CurrentUser, owner, model.User, model.Package, cancellationUrl);
}
}

Expand Down Expand Up @@ -190,12 +190,12 @@ public async Task<JsonResult> RemovePackageOwner(string id, string username)
throw new InvalidOperationException("You can't remove the only owner from a package.");
}
await _packageOwnershipManagementService.RemovePackageOwnerAsync(model.Package, model.CurrentUser, model.User, commitAsTransaction:true);
await _messageService.SendPackageOwnerRemovedNoticeAsync(model.CurrentUser, model.User, model.Package);
_messageService.SendPackageOwnerRemovedNotice(model.CurrentUser, model.User, model.Package);
}
else
{
await _packageOwnershipManagementService.DeletePackageOwnershipRequestAsync(model.Package, model.User);
await _messageService.SendPackageOwnerRequestCancellationNoticeAsync(model.CurrentUser, model.User, model.Package);
_messageService.SendPackageOwnerRequestCancellationNotice(model.CurrentUser, model.User, model.Package);
}

return Json(new { success = true });
Expand Down
Loading

0 comments on commit 9b00ef8

Please sign in to comment.