-
Notifications
You must be signed in to change notification settings - Fork 3
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
RAS-940: Questionnaire Completion Rate Information #288
RAS-940: Questionnaire Completion Rate Information #288
Conversation
/deploy scorfs |
Deploying to dev cluster with following parameters:
|
/deploy scorfs |
Deploying to dev cluster with following parameters:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the tests are a bit simplistic in this service, not expecting huge things, but a tweak at minimum would be good. There was a suggestion of having a trigger in the db doing the update, I don't personally think we need it, but another dev might think otherwise
@@ -61,4 +62,7 @@ public class CaseGroup implements Serializable { | |||
@Enumerated(EnumType.STRING) | |||
@Column(name = "status") | |||
private CaseGroupStatus status; | |||
|
|||
@Column(name = "change_state_timestamp") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly strange name, I would expect it to align with the column? Isn't that status, I also find it more palatable flipping the words, so something like status_change_timestamp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I've renamed it.
@@ -23,7 +22,7 @@ public void updateAuditTable(final CaseGroup caseGroup, final UUID partyId) { | |||
auditEntity.setCaseGroupFK(caseGroup.getCaseGroupPK()); | |||
auditEntity.setStatus(caseGroup.getStatus()); | |||
auditEntity.setPartyId(partyId); | |||
auditEntity.setCreatedDateTime(DateTimeUtil.nowUTC()); | |||
auditEntity.setCreatedDateTime(caseGroup.getChangeStateTimestamp()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not convinced we use this table for anything, but right to update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe it is but I thought for the sake on continuity to update it.
/deploy scorfs |
Deploying to dev cluster with following parameters:
|
/deploy robinm3 |
Deploying to dev cluster with following parameters:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good functionally, agree with the comment about adding some test changes as well.
@LJBabbage I agree the tests need adding. I have added one to cover the adding of a datetimestamp after transitioning status. |
@@ -6,6 +6,7 @@ | |||
import static org.mockito.Mockito.verify; | |||
|
|||
import java.util.*; | |||
import org.junit.Assert; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 3 brings in all the asserts
@@ -90,6 +93,7 @@ public void givenCaseGroupStatusWhenCaseGroupStatusTransitionedThenTransitionIsA | |||
caseGroupService.transitionCaseGroupStatus(caseGroup, categoryName, caseGroup.getPartyId()); | |||
|
|||
// Then | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra line?
@@ -64,6 +65,8 @@ public void givenCaseGroupStatusWhenCaseGroupStatusTransitionedThenTransitionIsS | |||
caseGroupService.transitionCaseGroupStatus(caseGroup, categoryName, caseGroup.getPartyId()); | |||
|
|||
// Then | |||
Assert.assertNotNull(caseGroup.getStatusChangeTimestamp()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change the name of this test to not be so rubbish, I know you didn't do it originally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now with the test changes
What and why?
There is a requirement to add a datetime for when a respondents status is changed and to display this in the Response Chasing Report. This PR includes the code change to add the timedate to the Casegroup table.
How to test?
Deploy this PR and update the status of a respondent (either via Frontstage or in rOps) and click the report for the associated survey. This will show the datetime of the status change within the Casegroup table.
Check the unit tests work and that the acceptance tests still work.
For completeness, deploy ONSdigital/rm-reporting#110 and follow it's test instructions.
Jira
https://jira.ons.gov.uk/browse/RAS-940