Skip to content

Commit ea46070

Browse files
author
Scott Bommarito
authored
Custom message should be required if deprecating package due to "Other" reason (#7108)
1 parent 784a6d0 commit ea46070

6 files changed

Lines changed: 187 additions & 33 deletions

File tree

‎src/NuGetGallery/Controllers/ManageDeprecationJsonApiController.cs‎

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,28 @@ public virtual async Task<JsonResult> Deprecate(
5858
string alternatePackageVersion,
5959
string customMessage)
6060
{
61+
var status = PackageDeprecationStatus.NotDeprecated;
62+
63+
if (isLegacy)
64+
{
65+
status |= PackageDeprecationStatus.Legacy;
66+
}
67+
68+
if (hasCriticalBugs)
69+
{
70+
status |= PackageDeprecationStatus.CriticalBugs;
71+
}
72+
73+
if (isOther)
74+
{
75+
if (string.IsNullOrWhiteSpace(customMessage))
76+
{
77+
return DeprecateErrorResponse(HttpStatusCode.BadRequest, Strings.DeprecatePackage_CustomMessageRequired);
78+
}
79+
80+
status |= PackageDeprecationStatus.Other;
81+
}
82+
6183
var currentUser = GetCurrentUser();
6284
if (!_featureFlagService.IsManageDeprecationEnabled(GetCurrentUser()))
6385
{
@@ -135,23 +157,6 @@ public virtual async Task<JsonResult> Deprecate(
135157
}
136158
}
137159

138-
var status = PackageDeprecationStatus.NotDeprecated;
139-
140-
if (isLegacy)
141-
{
142-
status |= PackageDeprecationStatus.Legacy;
143-
}
144-
145-
if (hasCriticalBugs)
146-
{
147-
status |= PackageDeprecationStatus.CriticalBugs;
148-
}
149-
150-
if (isOther)
151-
{
152-
status |= PackageDeprecationStatus.Other;
153-
}
154-
155160
await _deprecationService.UpdateDeprecation(
156161
packagesToUpdate,
157162
status,

‎src/NuGetGallery/Scripts/gallery/page-manage-deprecation.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,9 @@ function ManageDeprecationViewModel(id, versionDeprecationStateDictionary, defau
155155

156156
// The custom message to submit with the form.
157157
this.customMessage = ko.observable('');
158+
this.requiresCustomMessage = ko.pureComputed(function () {
159+
return self.isOther() && !self.customMessage();
160+
}, this);
158161

159162
this.submitError = ko.observable();
160163
this.submit = function () {

‎src/NuGetGallery/Strings.Designer.cs‎

Lines changed: 9 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/NuGetGallery/Strings.resx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1112,6 +1112,9 @@ The {1} Team</value>
11121112
<data name="DeprecatePackage_MissingRegistration" xml:space="preserve">
11131113
<value>Package '{0}' could not be found.</value>
11141114
</data>
1115+
<data name="DeprecatePackage_CustomMessageRequired" xml:space="preserve">
1116+
<value>You must add a custom message if you are deprecating a package due to another reason!</value>
1117+
</data>
11151118
<data name="SymbolsPackage_NoSymbols" xml:space="preserve">
11161119
<value>The package does not contain any symbol (.pdb) files.</value>
11171120
</data>

‎src/NuGetGallery/Views/Packages/_ManageDeprecation.cshtml‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
<div class="form-group unbolded-label">
5353
<label>
5454
<input name="isOther" type="checkbox" value="true" data-bind="checked: isOther" />
55-
This package should be deprecated for <b>other</b> reasons
55+
<b>Other</b>
5656
</label>
5757
</div>
5858
</div>
@@ -77,13 +77,16 @@
7777
<label for="customMessage" class="deprecation-section-header">Provide custom message</label>
7878
<textarea id="customMessage" name="customMessage" class="form-control full-line" data-bind="textInput: customMessage" rows="2"></textarea>
7979
</div>
80+
<div data-bind="visible: requiresCustomMessage()">
81+
@ViewHelpers.AlertDanger(@<text>You must add a custom message if <b>Other</b> is selected.</text>)
82+
</div>
8083
</div>
8184

8285
<div data-bind="visible: submitError">
8386
@ViewHelpers.AlertDanger(@<text><span data-bind="text: submitError"></span></text>)
8487
</div>
8588

8689
<div class="deprecation-section form-group">
87-
<button class="btn btn-primary full-line" type="submit" aria-label="Save package deprecation information" data-bind="click: submit">Save</button>
90+
<button class="btn btn-primary full-line" type="submit" aria-label="Save package deprecation information" data-bind="click: submit, disable: requiresCustomMessage">Save</button>
8891
</div>
8992
</script>

‎tests/NuGetGallery.Facts/Controllers/ManageDeprecationJsonApiControllerFacts.cs‎

Lines changed: 145 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,27 @@ public void ReturnsAllAvailableVersionsInReverseVersionOrder()
108108

109109
public class TheDeprecateMethod : TestContainer
110110
{
111+
[Fact]
112+
public async Task ReturnsBadRequestIfOtherAndNoCustomMessage()
113+
{
114+
// Arrange
115+
var controller = GetController<ManageDeprecationJsonApiController>();
116+
117+
// Act
118+
var result = await controller.Deprecate(
119+
id: "id",
120+
versions: null,
121+
isLegacy: false,
122+
hasCriticalBugs: false,
123+
isOther: true,
124+
alternatePackageId: null,
125+
alternatePackageVersion: null,
126+
customMessage: null);
127+
128+
// Assert
129+
AssertErrorResponse(controller, result, HttpStatusCode.BadRequest, Strings.DeprecatePackage_CustomMessageRequired);
130+
}
131+
111132
[Fact]
112133
public async Task ReturnsForbiddenIfFeatureFlagDisabled()
113134
{
@@ -125,7 +146,14 @@ public async Task ReturnsForbiddenIfFeatureFlagDisabled()
125146

126147
// Act
127148
var result = await controller.Deprecate(
128-
"id", null, false, false, false, null, null, null);
149+
id: "id",
150+
versions: null,
151+
isLegacy: false,
152+
hasCriticalBugs: false,
153+
isOther: false,
154+
alternatePackageId: null,
155+
alternatePackageVersion: null,
156+
customMessage: null);
129157

130158
// Assert
131159
AssertErrorResponse(controller, result, HttpStatusCode.Forbidden, Strings.DeprecatePackage_Forbidden);
@@ -153,7 +181,14 @@ public async Task ReturnsBadRequestIfNoVersions(IEnumerable<string> versions)
153181

154182
// Act
155183
var result = await controller.Deprecate(
156-
"id", versions, false, false, false, null, null, null);
184+
id: "id",
185+
versions: versions,
186+
isLegacy: false,
187+
hasCriticalBugs: false,
188+
isOther: false,
189+
alternatePackageId: null,
190+
alternatePackageVersion: null,
191+
customMessage: null);
157192

158193
// Assert
159194
AssertErrorResponse(controller, result, HttpStatusCode.BadRequest, Strings.DeprecatePackage_NoVersions);
@@ -196,7 +231,14 @@ public async Task ReturnsNotFoundIfNoPackagesOrRegistrationMissing(IEnumerable<P
196231

197232
// Act
198233
var result = await controller.Deprecate(
199-
id, new[] { "1.0.0" }, false, false, false, null, null, null);
234+
id: id,
235+
versions: new[] { "1.0.0" },
236+
isLegacy: false,
237+
hasCriticalBugs: false,
238+
isOther: false,
239+
alternatePackageId: null,
240+
alternatePackageVersion: null,
241+
customMessage: null);
200242

201243
// Assert
202244
AssertErrorResponse(controller, result, HttpStatusCode.NotFound, string.Format(Strings.DeprecatePackage_MissingRegistration, id));
@@ -258,7 +300,14 @@ public async Task ReturnsForbiddenIfNotOwner(User currentUser, User owner)
258300

259301
// Act
260302
var result = await controller.Deprecate(
261-
id, new[] { "1.0.0" }, false, false, false, null, null, null);
303+
id: id,
304+
versions: new[] { "1.0.0" },
305+
isLegacy: false,
306+
hasCriticalBugs: false,
307+
isOther: false,
308+
alternatePackageId: null,
309+
alternatePackageVersion: null,
310+
customMessage: null);
262311

263312
// Assert
264313
AssertErrorResponse(controller, result, HttpStatusCode.Forbidden, Strings.DeprecatePackage_Forbidden);
@@ -333,7 +382,14 @@ public async Task ReturnsForbiddenIfLocked(User currentUser, User owner)
333382

334383
// Act
335384
var result = await controller.Deprecate(
336-
id, new[] { "1.0.0" }, false, false, false, null, null, null);
385+
id: id,
386+
versions: new[] { "1.0.0" },
387+
isLegacy: false,
388+
hasCriticalBugs: false,
389+
isOther: false,
390+
alternatePackageId: null,
391+
alternatePackageVersion: null,
392+
customMessage: null);
337393

338394
// Assert
339395
AssertErrorResponse(
@@ -387,7 +443,14 @@ public async Task ReturnsNotFoundIfAlternatePackageRegistrationMissing(User curr
387443

388444
// Act
389445
var result = await controller.Deprecate(
390-
id, new[] { "1.0.0" }, false, false, false, alternatePackageId, null, null);
446+
id: id,
447+
versions: new[] { "1.0.0" },
448+
isLegacy: false,
449+
hasCriticalBugs: false,
450+
isOther: false,
451+
alternatePackageId: alternatePackageId,
452+
alternatePackageVersion: null,
453+
customMessage: null);
391454

392455
// Assert
393456
AssertErrorResponse(
@@ -442,7 +505,14 @@ public async Task ReturnsNotFoundIfAlternatePackageVersionMissing(User currentUs
442505

443506
// Act
444507
var result = await controller.Deprecate(
445-
id, new[] { "1.0.0" }, false, false, false, alternatePackageId, alternatePackageVersion, null);
508+
id: id,
509+
versions: new[] { "1.0.0" },
510+
isLegacy: false,
511+
hasCriticalBugs: false,
512+
isOther: false,
513+
alternatePackageId: alternatePackageId,
514+
alternatePackageVersion: alternatePackageVersion,
515+
customMessage: null);
446516

447517
// Assert
448518
AssertErrorResponse(
@@ -491,7 +561,14 @@ public async Task ReturnsNotFoundIfVersionMissing(User currentUser, User owner)
491561

492562
// Act
493563
var result = await controller.Deprecate(
494-
id, new[] { "1.0.0" }, false, false, false, null, null, null);
564+
id: id,
565+
versions: new[] { "1.0.0" },
566+
isLegacy: false,
567+
hasCriticalBugs: false,
568+
isOther: false,
569+
alternatePackageId: null,
570+
alternatePackageVersion: null,
571+
customMessage: null);
495572

496573
// Assert
497574
AssertErrorResponse(
@@ -540,7 +617,14 @@ public async Task ReturnsNotFoundIfSomeVersionMissing(User currentUser, User own
540617

541618
// Act
542619
var result = await controller.Deprecate(
543-
id, new[] { package.NormalizedVersion, "1.0.0" }, false, false, false, null, null, null);
620+
id: id,
621+
versions: new[] { "1.0.0" },
622+
isLegacy: false,
623+
hasCriticalBugs: false,
624+
isOther: false,
625+
alternatePackageId: null,
626+
alternatePackageVersion: null,
627+
customMessage: null);
544628

545629
// Assert
546630
AssertErrorResponse(
@@ -600,20 +684,67 @@ public enum ReturnsSuccessful_AlternatePackage_State
600684
MemberDataHelper.Combine(
601685
Owner_Data,
602686
PackageDeprecationStates_Data,
603-
MemberDataHelper.EnumDataSet<ReturnsSuccessful_AlternatePackage_State>(),
604-
MemberDataHelper.BooleanDataSet()).ToList();
687+
MemberDataHelper.EnumDataSet<ReturnsSuccessful_AlternatePackage_State>()).ToList();
605688

606689
[Theory]
607690
[MemberData(nameof(ReturnsSuccessful_Data))]
608-
public async Task ReturnsSuccessful(
691+
public Task ReturnsSuccessfulWithCustomMessage(
692+
User currentUser,
693+
User owner,
694+
bool isLegacy,
695+
bool hasCriticalBugs,
696+
bool isOther,
697+
PackageDeprecationStatus expectedStatus,
698+
ReturnsSuccessful_AlternatePackage_State alternatePackageState)
699+
{
700+
return AssertSuccessful(
701+
currentUser,
702+
owner,
703+
isLegacy,
704+
hasCriticalBugs,
705+
isOther,
706+
expectedStatus,
707+
alternatePackageState,
708+
true);
709+
}
710+
711+
/// <remarks>
712+
/// Deprecations where the only reason is "other" must have a custom message.
713+
/// </remarks>
714+
public static IEnumerable<object[]> ReturnsSuccessfulWithoutCustomMessage_Data =
715+
ReturnsSuccessful_Data.Where(x => !(bool)x[4]);
716+
717+
[Theory]
718+
[MemberData(nameof(ReturnsSuccessfulWithoutCustomMessage_Data))]
719+
public Task ReturnsSuccessfulWithoutCustomMessage(
720+
User currentUser,
721+
User owner,
722+
bool isLegacy,
723+
bool hasCriticalBugs,
724+
bool isOther,
725+
PackageDeprecationStatus expectedStatus,
726+
ReturnsSuccessful_AlternatePackage_State alternatePackageState)
727+
{
728+
return AssertSuccessful(
729+
currentUser,
730+
owner,
731+
isLegacy,
732+
hasCriticalBugs,
733+
isOther,
734+
expectedStatus,
735+
alternatePackageState,
736+
false);
737+
}
738+
739+
private async Task AssertSuccessful(
609740
User currentUser,
610741
User owner,
611742
bool isLegacy,
612743
bool hasCriticalBugs,
613744
bool isOther,
614745
PackageDeprecationStatus expectedStatus,
615746
ReturnsSuccessful_AlternatePackage_State alternatePackageState,
616-
bool hasAdditionalData)
747+
bool hasCustomMessage)
617748
{
618749
// Arrange
619750
var id = "id";
@@ -686,7 +817,7 @@ public async Task ReturnsSuccessful(
686817

687818
var deprecationService = GetMock<IPackageDeprecationService>();
688819

689-
var customMessage = hasAdditionalData ? "message" : null;
820+
var customMessage = hasCustomMessage ? "message" : null;
690821

691822
deprecationService
692823
.Setup(x => x.UpdateDeprecation(

0 commit comments

Comments
 (0)