Fixing Package Description truncation #3638

ryuyu · 2017-03-09T18:28:13Z

Split description truncation from extra information so we can parse html in extra info.
Fix for #3604

…tml in extra info.

joelverhagen · 2017-03-09T18:31:51Z

src/NuGetGallery/ViewModels/ListPackageItemViewModel.cs

+            {
+                if (this.Description.Length > _descriptionLengthLimit)
+                {
+                    return this.Description.Substring(1, _descriptionLengthLimit) + "...";


start index 1?

Also, this does not truncate at word boundary, as the old implementation does. Could we use that Razor helper here?

joelverhagen · 2017-03-09T18:33:40Z

src/NuGetGallery/ViewModels/ListPackageItemViewModel.cs

+        {
+            get
+            {
+                if (this.Description.Length > _descriptionLengthLimit)


Needs unit test

joelverhagen · 2017-03-09T18:34:16Z

src/NuGetGallery/Views/Shared/_ListPackage.cshtml

+                @Model.ShortDescription;
+                @if (Model.Description.Length > Model.DescriptionLengthLimit)
+                {
+                    @Html.Raw(string.Format(System.Globalization.CultureInfo.CurrentCulture, " <a href=\"{0}\">More information</a>", @Url.Package(Model)))


Consider using the helper for building links:
http://stackoverflow.com/a/7709015/52749

joelverhagen

⌚️

joelverhagen · 2017-03-09T18:34:34Z

src/NuGetGallery/ViewModels/ListPackageItemViewModel.cs

@@ -8,6 +8,8 @@ namespace NuGetGallery
 {
    public class ListPackageItemViewModel : PackageViewModel
    {
+        private int _descriptionLengthLimit = 350;


private const int

scottbommarito

I would suggest a solution for this that changes the existing TruncateAtWordBoundary function or creates a new method that doesn't HTML encode the omission string. I think it would be much more elegant and would reuse the existing TruncateAtWordBoundary code.

skofman1 · 2017-03-09T19:29:41Z

why do we need any changes to the ViewModel? You can fix this directly in the cshtml..

…ernal structure of ListPacakgeItemViewModel.

joelverhagen · 2017-03-09T20:11:21Z

src/NuGetGallery/Views/Shared/_ListPackage.cshtml

@@ -46,12 +46,12 @@
            </p>

            @if (@Model.Tags.AnySafe())
-            {
+                    {


nit: fix indentation

joelverhagen

Address nit then

joelverhagen · 2017-03-09T20:12:06Z

Could we have a unit test of the ViewModel?

joelverhagen · 2017-03-09T20:13:40Z

why do we need any changes to the ViewModel? You can fix this directly in the cshtml..

https://msdn.microsoft.com/en-us/library/hh848246.aspx

Under The Benefits of MVVM:

The developers can create unit tests for the view model and the model without using the view. The unit tests for the view model can exercise exactly the same functionality as used by the view.

It is easy to redesign the UI of the application without touching the code because the view is implemented entirely in XAML. A new version of the view should work with the existing view model.

scottbommarito · 2017-03-09T20:40:14Z

src/NuGetGallery/Helpers/StringExtensions.cs

@@ -43,13 +43,15 @@ public static string Abbreviate(this string text, int length)
            return text.Substring(0, length - 3) + "...";
        }

-        public static string TruncateAtWordBoundary(this string input, int length = 300, string ommission = "...", string moreText = "")
+        public static string TruncateAtWordBoundary(this string input, int length, string ommission, string moreText, out bool wasTruncated)


wasTruncated isn't necessary. All that needs to be done is to make string moreText not be HTML encoded. I recommend making this method return an MvcHtmlString that you encode yourself. You shouldn't need to change anything but this method for your fix.

Note that I don't mind giving the ViewModel a shortDescription property, it just shouldn't require anything but calling this method directly.

Please read the rest of the discussion before making comments like this. The new model moves logic from the view into the model, and makes this block more testable.

This is also a string extension and should return a string unless the method implies otherwise.

Regardless of whether you call TruncateAtWordBoundary in _ListPackage.cshtml or in ListPackageItemViewModel, you still don't need to add this additional parameter. The ViewModel can just call TruncateAtWordBoundary with moreText specified as Html.RouteLink("More information", RouteName.DisplayPackage, new { Model.Id, Model.Version }) and modify this method so that it doesn't HTML encode it. Splitting up this logic as you've done is unnecessary.

Additionally, this method previously returned an MvcHtmlString before it was changed to prevent XSS attacks. Its status as a string extension is irrelevant.

Also, I suggest if you want to increase the testability of the block of code with your changes, you should move the @if (Model.IsDescriptionTruncated) from the cshtml into the ViewModel. You can then test that the correct HTML link is added as well as testing that it was truncated correctly.

scottbommarito · 2017-03-09T20:43:52Z

src/NuGetGallery/Helpers/StringExtensions.cs

            if (string.IsNullOrEmpty(input) || input.Length < length)
                return input;

            int nextSpace = input.LastIndexOf(" ", length, StringComparison.Ordinal);

+            wasTruncated = true;
            return string.Format(CultureInfo.CurrentCulture, "{2}{1}{0}",


Can you fix the ordering of this so it's more readable? The parameters should be ordered so that it's {0}{1}{2}.

skofman1 · 2017-03-09T22:09:48Z

src/NuGetGallery/Helpers/StringExtensions.cs

        {
+            wasTruncated = false;
            if (string.IsNullOrEmpty(input) || input.Length < length)
                return input;


please add { }
We shouldn't continue with the old coding convention

skofman1 · 2017-03-09T22:16:10Z

tests/NuGetGallery.Facts/ViewModels/ListPackageItemViewModelFacts.cs

+        [Fact]
+        public void LongDescriptionsTruncated()
+        {
+            var description = @"A Longer description full of nonsense that will get truncated. Lorem ipsum dolor sit amet, ad nemore gubergren eam. Ea quaeque labores deseruisse his, eos munere convenire at, in eos audire persius corpora. Te his volumus detracto offendit, has ne illud choro. No illum quaestio mel, novum democritum te sea, et nam nisl officiis salutandi. Vis ut harum docendi incorrupte, nam affert putent sententiae id, mei cibo omnium id. Ea est falli graeci voluptatibus, est mollis denique ne.


You can test additional scenarios, like what if it's a single long string with no spaces, etc.

skofman1 · 2017-03-09T22:16:37Z

tests/NuGetGallery.Facts/ViewModels/ListPackageItemViewModelFacts.cs

@@ -0,0 +1,214 @@
+using System;


Like the extra coverage 💯

scottbommarito · 2017-03-09T22:02:41Z

src/NuGetGallery/ViewModels/ListPackageItemViewModel.cs

+            Owners = package.PackageRegistration?.Owners;
+
+            bool wasTruncated;
+            ShortDescription = Description.TruncateAtWordBoundary(_descriptionLengthLimit, _omissionString, out wasTruncated);


Can you pass IsDescriptionTruncated as the out bool directly?

scottbommarito · 2017-03-09T22:14:01Z

tests/NuGetGallery.Facts/ViewModels/ListPackageItemViewModelFacts.cs

+    {
+        // start with replicating the PackageViewModelFacts here since we shouldn't be breaking these
+        // ListPackageItemViewModel extends PackageViewModel
+        #region CopiedFromPackageViewModelFacts


I don't think this is a very good approach. The next time somebody changes PackageViewModelFacts, these test cases will become outdated and it won't be obvious that these need to be fixed as well. If you would really like to run these test cases on both classes, you should make each test case in PackageViewModelFacts a theory that takes in either PackageViewModel or ListPackageItemViewModel and then runs on that class.

Additionally, I was under the impression that unit tests for a subclass should test only the functionality that a subclass adds or changes rather than the entirety of the functionality of the superclass. I agree that we don't want to be breaking any of the previous functionality, but I think it's fairly safe to assume that the functionality of the subclass that hasn't been modified should be the same as the superclass.

I don't mind removing them from here. I just wanted to make sure that i wasn't interfering with any PackageViewModel functionality inadvertently.

That being said, the concern for synchronization between the locations is a valid one. Given that, we should make a decision about the general guidance for testing of derived classes.

Should derived class have tests for testing interference with base class if we aren't explicitly overriding?

Should it be assumes that all child classes are verified when modifying a parent class?

is it a good idea to separate testing by class, or should we test by hierarchy (ie PackageViewModelTests should or should not include tests for all derived classes for it's own functionality)?

This is a good discussion, but outside of the scope of the PR. If there is a particular concern, I'll remove them from here, but if a base test is updated, this one should break when run, so I'm a little less concerned about being "outdated"

I would prefer to remove them from here. Although they probably will, there isn't any guarantee that these tests will automatically break when we change the others, so maintainability is a major concern for me, as running bad unit tests unknowingly is in some ways just as bad as not testing at all. Additionally, we haven't copied our tests for each subclass anywhere else.

We can determine our process for testing derived classes on a later date and then modify these test cases as we see fit.

scottbommarito · 2017-03-09T22:15:05Z

src/NuGetGallery/ViewModels/ListPackageItemViewModel.cs

@@ -8,20 +9,29 @@ namespace NuGetGallery
 {
    public class ListPackageItemViewModel : PackageViewModel
    {
+        private const int _descriptionLengthLimit = 300;
+        private const string _omissionString = "...";


Do we want to consider using http://www.fileformat.info/info/unicode/char/2026/index.htm instead of "..."?

Not sure. On one hand, I like unicode. On the other hand, the "." character doesn't require unicode. I'm leaning towards leaving it as is.

…7.03.27 * tag 'v2017.03.27': (205 commits) Revert UpdateIsLatest optimistic concurrency changes (NuGet#3707) UpdateIsLatest concurrent unlist fix (NuGet#3695) Change telemetry time to use correct format (NuGet#3690) Fix typo of "publically" (NuGet#3636) Fix regression (NuGet#3667) Add credential to Register and RequestPasswordReset audits (NuGet#3666) Functional test for temp keys (NuGet#3664) Telemetry for temp keys (NuGet#3662) Temp keys implementation (NuGet#3563) (NuGet#3646) Extracting code: single type per file (NuGet#3644) Telemetry for package push (NuGet#3649) Upgrade to NuGet.* v4.0.0 dependencies (NuGet#3643) Fix concurrent push test by disabling search hijacking on feed (NuGet#3641) Fixing Package Description truncation (NuGet#3638) Fix Microsoft Account removal (NuGet#3639) Send e-mail when a new API key is created (NuGet#3634) IsLatest Fix: wrong connection string passed to retry context (NuGet#3632) Update WindowsAzure.Storage to 7.0.0 (NuGet#3633) Depend on signed version of Elmah (NuGet#3609) Move AzureEntityList and TableErrorLog to NuGetGallery.Core (NuGet#3607) ...

Split description truncation from extra information so we can parse h…

c7eaef1

…tml in extra info.

dnfclas added the cla-already-signed label Mar 9, 2017

joelverhagen reviewed Mar 9, 2017

View reviewed changes

joelverhagen requested changes Mar 9, 2017

View reviewed changes

joelverhagen reviewed Mar 9, 2017

View reviewed changes

scottbommarito suggested changes Mar 9, 2017

View reviewed changes

Remove some logic from view. Switch to truncate on word. Simplify int…

2c4a52c

…ernal structure of ListPacakgeItemViewModel.

joelverhagen reviewed Mar 9, 2017

View reviewed changes

joelverhagen approved these changes Mar 9, 2017

View reviewed changes

scottbommarito suggested changes Mar 9, 2017

View reviewed changes

ryuyu added 4 commits March 9, 2017 13:02

Paramter ordering. Fix indentation.

4828815

Adding unit tests.

e666cce

Missed removal of argument from a call.

464b324

Fix latestStable vs latest use version test.

88227d6

joelverhagen approved these changes Mar 9, 2017

View reviewed changes

skofman1 approved these changes Mar 9, 2017

View reviewed changes

scottbommarito suggested changes Mar 9, 2017

View reviewed changes

Add 1 test. Fix some braces.

d532251

ryuyu merged commit f3a9d60 into dev Mar 9, 2017

skofman1 mentioned this pull request Mar 28, 2017

Order Dependencies #3711

Merged

ryuyu deleted the ryuyu-fixTruncatedDescriptions branch July 6, 2017 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing Package Description truncation #3638

Fixing Package Description truncation #3638

ryuyu commented Mar 9, 2017

joelverhagen Mar 9, 2017

joelverhagen Mar 9, 2017

ryuyu Mar 9, 2017

joelverhagen Mar 9, 2017

joelverhagen Mar 9, 2017

joelverhagen left a comment

joelverhagen Mar 9, 2017

scottbommarito left a comment

skofman1 commented Mar 9, 2017

joelverhagen Mar 9, 2017

joelverhagen left a comment

joelverhagen commented Mar 9, 2017

joelverhagen commented Mar 9, 2017

scottbommarito Mar 9, 2017

scottbommarito Mar 9, 2017

ryuyu Mar 9, 2017 •

edited

scottbommarito Mar 9, 2017

scottbommarito Mar 9, 2017

skofman1 Mar 9, 2017

skofman1 Mar 9, 2017

skofman1 Mar 9, 2017

scottbommarito Mar 9, 2017

ryuyu Mar 9, 2017

scottbommarito Mar 9, 2017

ryuyu Mar 9, 2017

scottbommarito Mar 9, 2017

scottbommarito Mar 9, 2017

ryuyu Mar 9, 2017

Fixing Package Description truncation #3638

Fixing Package Description truncation #3638

Conversation

ryuyu commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito left a comment

Choose a reason for hiding this comment

skofman1 commented Mar 9, 2017

Choose a reason for hiding this comment

joelverhagen left a comment

Choose a reason for hiding this comment

joelverhagen commented Mar 9, 2017

joelverhagen commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryuyu Mar 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryuyu Mar 9, 2017 •

edited