Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
namespace DigitalLearningSolutions.Web.Tests.Controllers.TrackingSystem.Centre.Configuration
{
using System.Collections.Generic;
using System.Linq;
using DigitalLearningSolutions.Data.DataServices.UserDataService;
using DigitalLearningSolutions.Data.Services;
using DigitalLearningSolutions.Web.Controllers.TrackingSystem.Centre.Configuration;
Expand All @@ -23,6 +25,30 @@ public class RegistrationPromptsControllerTests
private RegistrationPromptsController registrationPromptsControllerWithMockHttpContext = null!;
private IUserDataService userDataService = null!;

private static IEnumerable<TestCaseData> AddAnswerModelErrorTestData
{
get
{
yield return new TestCaseData(
new string('x', 4000),
"xx",
"The complete list of answers must be 4000 characters or fewer (0 characters remaining for the new answer, 2 characters were entered)"
).SetName("Error_message_shows_zero_characters_remaining_if_options_string_is_at_max_length");
yield return new TestCaseData(
new string('x', 3998),
"xx",
"The complete list of answers must be 4000 characters or fewer (0 characters remaining for the new answer, 2 characters were entered)"
).SetName(
"Error_message_shows_zero_characters_remaining_if_options_string_is_two_less_than_max_length"
);
yield return new TestCaseData(
new string('x', 3996),
"xxxx",
"The complete list of answers must be 4000 characters or fewer (2 characters remaining for the new answer, 4 characters were entered)"
).SetName("Error_message_shows_two_less_than_number_of_characters_remaining_if_possible_to_add_answer");
}
}

[SetUp]
public void Setup()
{
Expand Down Expand Up @@ -238,6 +264,41 @@ public void AddRegistrationPromptConfigureAnswers_add_configures_new_answer_and_
}
}

[Test]
[TestCaseSource(
typeof(RegistrationPromptsControllerTests),
nameof(AddAnswerModelErrorTestData)
)]
public void
AddRegistrationPromptConfigureAnswers_adds_correct_model_error_if_new_answer_surpasses_character_limit(
string optionsString,
string newAnswerInput,
string expectedErrorMessage
)
{
// Given
var initialSelectPromptModel = new AddRegistrationPromptSelectPromptViewModel(1, true);

var inputAnswersViewModel = new RegistrationPromptAnswersViewModel(optionsString, newAnswerInput);

var initialTempData = new AddRegistrationPromptData
{ SelectPromptViewModel = initialSelectPromptModel, ConfigureAnswersViewModel = inputAnswersViewModel };
registrationPromptsController.TempData.Set(initialTempData);

const string action = "addPrompt";

// When
var result =
registrationPromptsController.AddRegistrationPromptConfigureAnswers(inputAnswersViewModel, action);

// Then
using (new AssertionScope())
{
result.As<ViewResult>().Model.Should().BeOfType<RegistrationPromptAnswersViewModel>();
AssertModelStateErrorIsExpected(result, expectedErrorMessage);
}
}

[Test]
public void AddRegistrationPromptConfigureAnswers_delete_removes_configured_answer()
{
Expand Down Expand Up @@ -385,7 +446,8 @@ public void RegistrationPromptBulkPost_updates_temp_data_and_redirects_to_config

var initialTempData = new AddRegistrationPromptData
{
SelectPromptViewModel = initialPromptModel, ConfigureAnswersViewModel = initialConfigureAnswersViewModel
SelectPromptViewModel = initialPromptModel,
ConfigureAnswersViewModel = initialConfigureAnswersViewModel,
};
registrationPromptsController.TempData.Set(initialTempData);

Expand Down Expand Up @@ -418,5 +480,12 @@ private void AssertEditTempDataIsExpected(EditRegistrationPromptViewModel expect
registrationPromptsController.TempData.Peek<EditRegistrationPromptData>()!.EditModel.Should()
.BeEquivalentTo(expectedData);
}

private static void AssertModelStateErrorIsExpected(IActionResult result, string expectedErrorMessage)
{
var errorMessage = result.As<ViewResult>().ViewData.ModelState.Select(x => x.Value.Errors)
.Where(y => y.Count > 0).ToList().First().First().ErrorMessage;
errorMessage.Should().BeEquivalentTo(expectedErrorMessage);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
namespace DigitalLearningSolutions.Web.Tests.Controllers.TrackingSystem.CourseSetup
{
using System.Collections.Generic;
using System.Linq;
using DigitalLearningSolutions.Data.DataServices;
using DigitalLearningSolutions.Data.Models.CustomPrompts;
using DigitalLearningSolutions.Data.Services;
Expand All @@ -26,6 +27,30 @@ public class AdminFieldsControllerTests
private readonly ICourseService courseService = A.Fake<ICourseService>();
private AdminFieldsController controller = null!;

private static IEnumerable<TestCaseData> AddAnswerModelErrorTestData
{
get
{
yield return new TestCaseData(
new string('x', 1000),
"xx",
"The complete list of answers must be 1000 characters or fewer (0 characters remaining for the new answer, 2 characters were entered)"
).SetName("Error_message_shows_zero_characters_remaining_if_options_string_is_at_max_length");
yield return new TestCaseData(
new string('x', 998),
"xx",
"The complete list of answers must be 1000 characters or fewer (0 characters remaining for the new answer, 2 characters were entered)"
).SetName(
"Error_message_shows_zero_characters_remaining_if_options_string_is_two_less_than_max_length"
);
yield return new TestCaseData(
new string('x', 996),
"xxxx",
"The complete list of answers must be 1000 characters or fewer (2 characters remaining for the new answer, 4 characters were entered)"
).SetName("Error_message_shows_two_less_than_number_of_characters_remaining_if_possible_to_add_answer");
}
}

[SetUp]
public void Setup()
{
Expand Down Expand Up @@ -339,6 +364,35 @@ public void AddAdminField_adds_answer_without_admin_field_selected()
}
}

[Test]
[TestCaseSource(
typeof(AdminFieldsControllerTests),
nameof(AddAnswerModelErrorTestData)
)]
public void AddAdminField_adds_model_error_if_new_answer_surpasses_character_limit(
string optionsString,
string newAnswerInput,
string expectedErrorMessage
)
{
// Given
var initialViewModel = new AddAdminFieldViewModel(1, optionsString, newAnswerInput);
var initialTempData = new AddAdminFieldData(initialViewModel);
controller.TempData.Set(initialTempData);
const string action = "addPrompt";

// When
var result =
controller.AddAdminField(1, initialViewModel, action);

// Then
using (new AssertionScope())
{
result.As<ViewResult>().Model.Should().BeOfType<AddAdminFieldViewModel>();
AssertModelStateErrorIsExpected(result, expectedErrorMessage);
}
}

[Test]
public void AddAdminField_delete_removes_configured_answer()
{
Expand Down Expand Up @@ -528,5 +582,12 @@ private void AssertAddTempDataIsExpected(AddAdminFieldViewModel expectedData)
controller.TempData.Peek<AddAdminFieldData>()!.AddModel.Should()
.BeEquivalentTo(expectedData);
}

private static void AssertModelStateErrorIsExpected(IActionResult result, string expectedErrorMessage)
{
var errorMessage = result.As<ViewResult>().ViewData.ModelState.Select(x => x.Value.Errors)
.Where(y => y.Count > 0).ToList().First().First().ErrorMessage;
errorMessage.Should().BeEquivalentTo(expectedErrorMessage);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public IActionResult EditRegistrationPrompt(EditRegistrationPromptViewModel mode
SaveAction => EditRegistrationPromptPostSave(model),
AddPromptAction => RegistrationPromptAnswersPostAddPrompt(model),
BulkAction => EditRegistrationPromptBulk(model),
_ => new StatusCodeResult(500)
_ => new StatusCodeResult(500),
};
}

Expand Down Expand Up @@ -145,7 +145,7 @@ public IActionResult AddRegistrationPromptNew()
id.ToString(),
new CookieOptions
{
Expires = CookieExpiry
Expires = CookieExpiry,
}
);
TempData.Set(addRegistrationPromptData);
Expand Down Expand Up @@ -209,7 +209,7 @@ string action
NextAction => AddRegistrationPromptConfigureAnswersPostNext(model),
AddPromptAction => RegistrationPromptAnswersPostAddPrompt(model, true),
BulkAction => AddRegistrationPromptBulk(model),
_ => new StatusCodeResult(500)
_ => new StatusCodeResult(500),
};
}

Expand Down Expand Up @@ -413,7 +413,7 @@ private void SetEditRegistrationPromptTempData(EditRegistrationPromptViewModel m
id.ToString(),
new CookieOptions
{
Expires = CookieExpiry
Expires = CookieExpiry,
}
);
TempData.Set(data);
Expand All @@ -439,15 +439,23 @@ string optionsString

private void SetTotalAnswersLengthTooLongError(RegistrationPromptAnswersViewModel model)
{
if (model.OptionsString == null || model.OptionsString.Length < 2)
if (model.OptionsString == null)
{
return;
}

var remainingLength = 4000 - (model.OptionsString?.Length - 2 ?? 0);
var remainingLength = 4000 - model.OptionsString.Length;
var remainingLengthShownToUser = remainingLength <= 2 ? 0 : remainingLength - 2;
var answerLength = model.Answer!.Length;
var remainingLengthPluralitySuffix = DisplayStringHelper.GetPluralitySuffix(remainingLengthShownToUser);
var answerLengthPluralitySuffix = DisplayStringHelper.GetPluralitySuffix(answerLength);
var verb = answerLength == 1 ? "was" : "were";

ModelState.AddModelError(
nameof(RegistrationPromptAnswersViewModel.Answer),
$"The complete list of answers must be 4000 characters or fewer ({remainingLength} characters remaining for the new answer)"
"The complete list of answers must be 4000 characters or fewer " +
$"({remainingLengthShownToUser} character{remainingLengthPluralitySuffix} remaining for the new answer, " +
$"{answerLength} character{answerLengthPluralitySuffix} {verb} entered)"
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ string action
SaveAction => EditAdminFieldPostSave(customisationId, model),
AddPromptAction => AdminFieldAnswersPostAddPrompt(model),
BulkAction => EditAdminFieldBulk(customisationId, model),
_ => new StatusCodeResult(500)
_ => new StatusCodeResult(500),
};
}

Expand Down Expand Up @@ -192,7 +192,7 @@ public IActionResult AddAdminField(int customisationId, AddAdminFieldViewModel m
SaveAction => AddAdminFieldPostSave(customisationId, model),
AddPromptAction => AdminFieldAnswersPostAddPrompt(model),
BulkAction => AddAdminFieldBulk(customisationId, model),
_ => new StatusCodeResult(500)
_ => new StatusCodeResult(500),
};
}

Expand Down Expand Up @@ -307,7 +307,7 @@ private void SetEditAdminFieldTempData(EditAdminFieldViewModel model)
id.ToString(),
new CookieOptions
{
Expires = CookieExpiry
Expires = CookieExpiry,
}
);
TempData.Set(data);
Expand Down Expand Up @@ -359,7 +359,7 @@ private void SetAddAdminFieldTempData(AddAdminFieldViewModel model)
id.ToString(),
new CookieOptions
{
Expires = CookieExpiry
Expires = CookieExpiry,
}
);
TempData.Set(data);
Expand Down Expand Up @@ -470,15 +470,23 @@ string optionsString

private void SetTotalAnswersLengthTooLongError(AdminFieldAnswersViewModel model)
{
if (model.OptionsString == null || model.OptionsString.Length < 2)
if (model.OptionsString == null)
{
return;
}

var remainingLength = 1000 - (model.OptionsString?.Length - 2 ?? 0);
var remainingLength = 1000 - model.OptionsString.Length;
var remainingLengthShownToUser = remainingLength <= 2 ? 0 : remainingLength - 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we think it is clear enough to just show zero if the answer is over the limit?

I'm wondering about showing how many characters are over the limit so the user knows how much they need to shorten their answer by. But then that might introduce confusion if they then want to add another answer which will mysteriously be another 2 characters shorter.

Do we think its not worth the bother?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm right. I think it would be useful to know how many characters you've entered so you can compare it against the number remaining, and I think it would be more useful than confusing. This error will probably not be encountered very often, and I think it would be more useful than confusing. Added in the number of characters entered to the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just need to fix the tests accordingly, will do after lunch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests are passing now, and the new error message looks like this, let me know what you think:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this looks pretty good. Minor suggestion, I think it would be better if it said "x characters were entered" to make it slightly clearer that those characters are the ones the user just tried to enter. Does that sound reasonable?

var answerLength = model.Answer!.Length;
var remainingLengthPluralitySuffix = DisplayStringHelper.GetPluralitySuffix(remainingLengthShownToUser);
var answerLengthPluralitySuffix = DisplayStringHelper.GetPluralitySuffix(answerLength);
var verb = answerLength == 1 ? "was" : "were";

ModelState.AddModelError(
nameof(AdminFieldAnswersViewModel.Answer),
$"The complete list of answers must be 1000 characters or fewer ({remainingLength} characters remaining for the new answer)"
"The complete list of answers must be 1000 characters or fewer " +
$"({remainingLengthShownToUser} character{remainingLengthPluralitySuffix} remaining for the new answer, " +
$"{answerLength} character{answerLengthPluralitySuffix} {verb} entered)"
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

@{
var errorHasOccurred = !ViewData.ModelState.IsValid;
ViewData["Title"] = "Add course admin field";
ViewData["Title"] = errorHasOccurred ? "Error: Add course admin field" : "Add course admin field";
ViewData["Application"] = "Tracking System";
ViewData["HeaderPath"] = $"{configuration["AppRootPath"]}/TrackingSystem/Centre/Dashboard";
ViewData["HeaderPathName"] = "Tracking System";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

@{
var errorHasOccurred = !ViewData.ModelState.IsValid;
ViewData["Title"] = "Edit course admin field";
ViewData["Title"] = errorHasOccurred ? "Error: Edit course admin field" : "Edit course admin field";
ViewData["Application"] = "Tracking System";
ViewData["HeaderPath"] = $"{configuration["AppRootPath"]}/TrackingSystem/Centre/Dashboard";
ViewData["HeaderPathName"] = "Tracking System";
Expand Down