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

Issue 4258 - ContactOwners e-mails contain the package version numer next to package id #5116

Merged
merged 14 commits into from
Jan 19, 2018
Merged

Conversation

dvkudr
Copy link
Contributor

@dvkudr dvkudr commented Dec 4, 2017

No description provided.

Copy link
Contributor

@scottbommarito scottbommarito left a comment

Choose a reason for hiding this comment

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

That was quick!

Good job!


if (package == null || package.PackageRegistration == null)
var package = _packageService.FindPackageByIdAndVersionStrict(id, version);
if (package == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you removed the package.PackageRegistration check?

It's probably redundant, but we do access package.PackageRegistration.Owners directly below, so if package.PackageRegistration is null, this will throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

{
string subject = "[{0}] Message for owners of the package '{1}'";
string body = @"_User {0} <{1}> sends the following message to the owners of Package '{2}'._
string body = @"_User {0} <{1}> sends the following message to the owners of Package '{2}' {3}._
Copy link
Contributor

@scottbommarito scottbommarito Dec 4, 2017

Choose a reason for hiding this comment

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

Package shouldn't be capitalized. Also, it's a little weird to have the ID in quotes but the version not.

E.g.

Package 'Newtonsoft.Json' 6.0.3

I would suggest wrapping the entire thing in quotes.

...sends the following message to the owners of package '{2} {3}'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

};

var packageService = new Mock<IPackageService>();
packageService.Setup(p => p.FindPackageByIdAndVersion("pkgid", null, null, true)).Returns(package);
packageService.Setup(p => p.FindPackageByIdAndVersionStrict("pkgid", "1.0.0")).Returns(package);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest saving pkgid and 1.0.0 in variables so you don't need to declare them by hand every time you use them.

var id = "pkgid";
var version = "1.0.0";
...
packageService.Setup(p => p.FindPackageByIdAndVersionStrict(id, version)).Returns(package);

same with the tests below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed & some new tests

@ghost ghost removed the cla-signed label Dec 6, 2017
@ghost ghost deleted a comment from dnfclas Dec 6, 2017
@ghost ghost deleted a comment from dnfclas Dec 6, 2017
@dvkudr
Copy link
Contributor Author

dvkudr commented Dec 11, 2017

Hello!
I'm wondering whether there's any chance to push the PR through.

@skofman1
Copy link
Contributor

Thanks for the PR @dvkudr ! We will merge this PR and deploy to nuget.org this sprint (the next 3 weeks).

Copy link
Contributor

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

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

Looks great!

@scottbommarito scottbommarito merged commit 0a897f7 into NuGet:dev Jan 19, 2018
@scottbommarito
Copy link
Contributor

@dvkudr , sorry for taking so long to take care of this. I've just merged this PR and it'll appear in our next deployment. Thanks again for the contribution!

@dvkudr
Copy link
Contributor Author

dvkudr commented Jan 21, 2018

Great! Thank you guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants