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

VerifyPackage page with more information #2242

Closed
wants to merge 10 commits into from

Conversation

robertmuehsig
Copy link
Contributor

As reported in this issue #1570 I added a few more package details on the verify package page:

  • MinClientVersion
  • Language
  • DeveloperDependency
  • FrameworkAssemblies
  • Dependencies

(ignore the glimpse bar - I choose a poor screen capture plugin ;))

Example with this package https://www.nuget.org/packages/Microsoft.Net.Http/2.2.27-beta results in:
image

https://www.nuget.org/packages/MvvmLight/
image

And one "devdependency" package https://www.nuget.org/packages/StyleCop.MSBuild/:
image

</h4>
<div style="position: relative">
<ul>
@foreach (var frameworkAssembly in Model.FrameworkAssemblies)
Copy link
Member

Choose a reason for hiding this comment

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

Can you pivot this so that it's grouped by the Supported Framework first, with the list of framework assemblies inside those groups? That would be consistent with how the Dependencies are grouped.

<ul id="packageDetails" style="border-bottom: 1px solid gray; margin-bottom: 1em;">
<li>@ReadOnlyField("Package ID", "Id", Model.Id)</li>
<li>@ReadOnlyField("Version", "Version", Model.Version)</li>
<li>@ReadOnlyField("MinClient Version", "MinClientVersion", Model.MinClientVersion.ToStringSafe())</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change the display name for this field to "Minimum NuGet Client Version" ?

Copy link
Member

Choose a reason for hiding this comment

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

Ha - I just clicked to add this exact request and then GitHub dynamically rendered it in. :-) So, 👍

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.

@robertmuehsig
Copy link
Contributor Author

Update for the wording issue "Minium NuGet Client Version":
image

@robertmuehsig
Copy link
Contributor Author

For the language question - if nothing is specified everything "works as expected" - sample nuget package without any language specified: https://www.nuget.org/packages/this.Log-NLog.VB.Sample/

image

@analogrelay
Copy link
Contributor

Awesome stuff. I say :shipit:

@robertmuehsig
Copy link
Contributor Author

Open Issues:

  • "Can you pivot this so that it's grouped by the Supported Framework first, with the list of framework assemblies inside those groups? That would be consistent with how the Dependencies are grouped."
  • "Please model the Development Dependency field after the 'Requires License Acceptance' field, where it's a Yes/No display instead of True/False. Development Dependency is still read-only though, since our package editing doesn't support changing that value. I'll file a bug for that though, as it should be editable."

@robertmuehsig
Copy link
Contributor Author

Ok - I changed the DevelopmentDependency Property like the RequireLicenseAcceptance Property:
image

But to get this into the database I also need to change the PackageEdit Entity and the EditPackageService - but in the first step this field needs to be in the database. Any convention how to create a new migration?
With this in place the edit developent dependency issue should be resolved very quickly #2244

@robertmuehsig
Copy link
Contributor Author

"Can you pivot this so that it's grouped by the Supported Framework first, with the list of framework assemblies inside those groups? That would be consistent with how the Dependencies are grouped."
image

Still open: PackageEdit Enity with the DevelopmentDependency Property and Database migration. Maybe we could leave the DevelopmentDependency in the first step as read only and handle this in another issue?

@xavierdecoster
Copy link
Member

@robertmuehsig can't merge the PR as is as I'm getting an exception due to the missing DevelopmentDependencyProperty (exception thrown at EditPackageService.GetPendingMetadata line 27).

Would be good if you could fix that one. Even better if you could also tackle this related issue to make it editable :)

Happy to take it in once fixed.

@xavierdecoster
Copy link
Member

@robertmuehsig Also, in order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. https://cla2.dotnetfoundation.org.

@dnfclas
Copy link

dnfclas commented May 21, 2015

@robertmuehsig, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@robertmuehsig
Copy link
Contributor Author

Will look at it, but if I remember correctly the edit DevDependency needed a DB Migration, that's why it was just "read-only", but I will check it an leave a comment on the other issue. Thanks for looking into it.

@robertmuehsig
Copy link
Contributor Author

So - I had time to look again at this PR and now I remember the problem.

@xavierdecoster I guess you are seeing this exception:
image
Now my comment from above makes perfect sense:
"But to get this into the database I also need to change the PackageEdit Entity and the EditPackageService - but in the first step this field needs to be in the database. Any convention how to create a new migration?
With this in place the edit developent dependency issue should be resolved very quickly #2244"

I added a new property "DeploymentDependency" like "RequireLicence" but I need a to apply a database migration, because the actual package is not changed (as far as I know) - the edit stuff is saved in the database.

As questioned above (and with the new NuGet.org already in the making) : Should I just make a new database migration for the new property or should I leave the property as "read only"?
I would create a new database migration in the same programming style as the other db migrations.

@xavierdecoster
Copy link
Member

@robertmuehsig that is exactly the issue I'm seeing :)

I guess you mean "DevelopmentDependency" instead of "DeploymentDependency", right?
Will get back to you on the db migration question.

@robertmuehsig
Copy link
Contributor Author

Woops - yes - DevelopmentDependency ;)

@maartenba
Copy link
Contributor

(fyi) @robertmuehsig Just in case you want to create a migration: from the package manager console, just run Add-Migration DevelopmentDependencyEditable. EF will make the necessary files. No need for handcrafted migration, unless not possible otherwise :)

@robertmuehsig
Copy link
Contributor Author

Ay! Just give me a hint if I should create the migration and finish this PR + the fix for the editable DevDependency and I try my best. Database changes are sometimes nasty for the deployment-team, so I didn't want to rush too quickly.

@maartenba
Copy link
Contributor

@robertmuehsig I'll probably take care of the deployment, thanks for thinking of me :)

@robertmuehsig
Copy link
Contributor Author

👍 Then I will try my best not to screw up the NuGet.org database ;)

@robertmuehsig
Copy link
Contributor Author

Ok, it seems I have found all missing pieces and it seems to work in my branch:

Edit Page:
image

Detail Page:
image

Verify Page:
image

Tested with the LibLog (DevDependency == true) package https://www.nuget.org/packages/LibLog/4.1.1

I needed two migrations, because in the first run I didn't had the DevelopmentDependency in the Package Entity, but I guess this could be merged.

@maartenba
Copy link
Contributor

Any chance you can squash your commits and rebase on dev branch?

@robertmuehsig
Copy link
Contributor Author

Sure, but my Git-Voodoo is too low. Could you give me a hint how to do that?

@maartenba
Copy link
Contributor

@yishaigalatzer
Copy link

Is the migration required only for the 'DevelopmentDependency'? Can we leave it out (sorry being super risk averse).

@robertmuehsig
Copy link
Contributor Author

@yishaigalatzer Yes, as far as I know you track the "editable" fields (and the values) in the database. In the original PR (or from another issue? It's a long time ago) my first try was just to leave it as read-only, but - just like the other fields - it should be editable, that's why I created the DB migrations.
Without the DB migrations the DevDependecy would only be "read-only". I guess I could rebase the Pull Request to this point in time #2242 (comment) and just finished the PR without the DB migration and leave this field as read-only (in the first step).
Just let me know how you would like to handle the PR.

@robertmuehsig
Copy link
Contributor Author

Woops... wrong button ;)

@maartenba
Copy link
Contributor

Let's make it read-only for now? If we'd make it editable we'd need a fix on the jobs-side of things as well. We can revisit making it editable later.

@robertmuehsig
Copy link
Contributor Author

Ok - I will try my best.

@maartenba
Copy link
Contributor

Thanks @robertmuehsig - you rock!

@304NotModified
Copy link
Contributor

@robertmuehsig I like this PR. I would be great if the last things get fixed! Need help on this?

@robertmuehsig
Copy link
Contributor Author

Hi @304NotModified - as you might have seen: I didn't found any time to work on this.

Because my Git Skills are very low, my plan was to create a new PR with the newest dev-source and make all changes except for the part with the needed Database Migration.

Idea:

  • Show all dependencies & meta data
  • Show DevDependency as Read-Only (so no DB upgrade is needed)

I think you could reuse most of the code - there are only a couple of changes I made for the DevDependency database stuff.

If you would like to work on this - go ahead :)

@304NotModified
Copy link
Contributor

@robertmuehsig, @maartenba

FYI, It's really easy to squash commits in tortoisegit!

image

http://stackoverflow.com/a/21306306

@robertmuehsig
Copy link
Contributor Author

With the recent merge of #2834 I think this PR can be closed. The "editable" DevDependency is a greater issue, because it needs database changes.
I would try to show the "DevelopmentDependency" (like this https://github.com/robertmuehsig/NuGetGallery/blob/1570-verifypage/src/NuGetGallery/Views/Packages/DisplayPackage.cshtml#L282 ) status via a new PR.

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.

8 participants