HEEDLS-412 - Implement new StoreASPAssessNoSession tracker endpoint #1187
Conversation
For reference, this is a git issue, not a github issue, and can be resolved on the command line as follows: (http://git-scm.com/docs/git-diff#Documentation/git-diff.txt--Mltngt) |
| public static readonly TrackerEndpointResponse NullTutorialStatusOrTime = | ||
| new TrackerEndpointResponse(-14, nameof(NullTutorialStatusOrTime)); | ||
| public static readonly TrackerEndpointResponse StoreAspAssessException = | ||
| new TrackerEndpointResponse(-6, nameof(StoreAspAssessException)); |
There was a problem hiding this comment.
What's the deal with these negative ids?
There was a problem hiding this comment.
When the process is complete, we expect the tracker endpoint to return some text in the format of
"[#Result:1]"
If the process errors, we expect to return a similar output along the lines of
"[#Result:{errorCode}]"
where the errorCode is a specific number. These error codes are all negative to indicate the a problem occurred. Hence we save the IDs of each of these as negative so we can easily return the right code from the enum.
Why the numbers are what they are, I am unsure. But I believe these tracker endpoints are all used in Learning Hub, which will expect specific results in specific circumstances - I am unsure how they will behave if any of this changed. But what we have here more or less emulates the old code.
| .MustHaveHappenedOnceExactly(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
It'd have been easier to review this file if the GetProgressAndValidateInputsForStoreAspAssess and GetProgressAndValidateCommonInputsForStoreAspProgressEndpoints tests were grouped separately, rather than interspersed.
There was a problem hiding this comment.
Yeah I realise that mistake now. I was trying to go with "near identical tests for different methods are together" that would make the whole file easier to understand, but from a comparison perspective it ends up worse. I've left them how they were for now so comparing between commits should be simple.
|
|
||
| [Test] | ||
| public void | ||
| GetProgressAndValidateCommonInputsForStoreAspProgressEndpoints_returns_progress_record_and_null_exception_when_all_is_valid() |
There was a problem hiding this comment.
Instead of null_exception, I'd prefer either no_exception or null_validationResponse, since I immediately thought: "Is a null exception like Java's null pointer exception?"
Note: there are numerous other tests with this naming convention both in this file and in TrackerActionServiceTests
| private void StoreAspProgressServiceReturnsDefaultDetailedCourseProgressOnValidation() | ||
| [Test] | ||
| public void | ||
| StoreAspAssessNoSession_returns_non_null_exception_from_query_param_and_progress_validation() |
There was a problem hiding this comment.
"non null exception" = "an exception"?
JIRA link
https://softwiretech.atlassian.net/browse/HEEDLS-412
Description
New tracker endpoint StoreAspAssessNoSession. Some minor renaming/refactoring of related methods has also been performed.
As part of this, the StoreAspProgressService was renamed to StoreAspService as it contains methods used across StoreAspProgress and StoreAspAssess tracker endpoints. A similar renaming was performed on the test file for this service. It looks like GitHub hasn't compared the details effectively, and considers the old file deleted. So there are a few changes in each which aren't actually changed.
Screenshots
N/A
Developer checks
(Leave tasks unticked if they haven't been appropriate for your ticket.)
I have: