Description

Confirmation summary will indicate which fields have been updated. Links that have been visited will now be displayed in a different colour similarly to other Government of Canada sites.

Any new packages installed?

N/A

Required followup work

N/A

Checklist:

[ ] I have updated the azurescript.sh with any new environment variables and added them to the appsettings
[ ] I have looked at my code on GitHub and it all looks good (ex: no random commented out code or console.logs)
[ ] I have added and needed tests for my changes (in particular for new screens)
[ ] I have added a comment to any confusing code
[ ] I have added documentation to any modified front-end code. (Or added missing documentation)

lgtm-com[bot] commented 3 years ago

This pull request introduces 5 alerts when merging cd0d4e088c736b287a399cb15a6ccb03995e180a into 4e3e45af9adf3565d9f1a62dee279ab7e60c8e5f - view on LGTM.com

new alerts:

5 for Unused variable, import, function or class

ying-chen-cds commented 3 years ago

If you click "Edit" and go to a page, do nothing and come back, it shows "Edited", but actually user changed nothing. I don't know if in that case, we should still display "Edited".

sdzhangtao commented 3 years ago

Here is an easy one. There are alerts. 5 for Unused variable, import, function or class

justinr86 commented 3 years ago

If you click "Edit" and go to a page, do nothing and come back, it shows "Edited", but actually user changed nothing. I don't know if in that case, we should still display "Edited".

Yes, this is something I struggled with. How much logic should we use to display this message?

@ngosset @stephaniemgauthier @TarekTraboulsi @barrhayl @Cassista Any thoughts?

Note: the field is only labeled 'Edited' if the user selects the 'Continue' button, selecting 'Go Back' will not label the field 'Edited'.

justinr86 commented 3 years ago

Here is an easy one. There are alerts. 5 for Unused variable, import, function or class

Yes, this was resolved in another commit. This causes a failed check due to linting errors. It is now resolved.

sdzhangtao commented 3 years ago

The edit link looks good. But I feel the refactor of containsData should be in a separate PR. I agree containsData is overly complex. It's overly complex because it's too generic. I am happy to see it refactored based on different data types. Or maybe only stringNotEmpty is required.

justinr86 commented 3 years ago

The edit link looks good. But I feel the refactor of containsData should be in a separate PR. I agree containsData is overly complex. It's overly complex because it's too generic. I am happy to see it refactored based on different data types. Or maybe only stringNotEmpty is required.

containsData has only been replaced where it would cause a blank summary to be displayed. I can change the code to use containsData more specifically if you think that would be more appropriate.

sdzhangtao commented 3 years ago

The edit link looks good. But I feel the refactor of containsData should be in a separate PR. I agree containsData is overly complex. It's overly complex because it's too generic. I am happy to see it refactored based on different data types. Or maybe only stringNotEmpty is required.

containsData has only been replaced where it would cause a blank summary to be displayed. I can change the code to use containsData more specifically if you think that would be more appropriate.

Tha's OK, just feel a bit rushed for a Friday and migration day I would love to have a separate PR to address this containsData. To be honest I don't like this containsData at all. I think it should be case by case. We use it mostly for string. As for checking an option exist or not. I think your code is a lot easier to read and understand {containsData(whenDidItHappen) vs {whenDidItHappen.incidentFrequency ?

The latter reads much better.

justinr86 commented 3 years ago

I will work on an update to evaluate form contents and only mark as edited if the contents are different.

This PR will be left open to collect more feedback.

ying-chen-cds commented 3 years ago

If you click "Edit" and go to a page, do nothing and come back, it shows "Edited", but actually user changed nothing. I don't know if in that case, we should still display "Edited".

Yes, this is something I struggled with. How much logic should we use to display this message?

@ngosset @stephaniemgauthier @TarekTraboulsi @barrhayl @Cassista Any thoughts?

Note: the field is only labeled 'Edited' if the user selects the 'Continue' button, selecting 'Go Back' will not label the field 'Edited'.

If making "Edited" accurately reflects if the form is actually edited or not involves too complicated logic but little business value , maybe turn the link of "Edit" to purple is good enough for now.

ying-chen-cds commented 3 years ago

The edit link looks good. But I feel the refactor of containsData should be in a separate PR. I agree containsData is overly complex. It's overly complex because it's too generic. I am happy to see it refactored based on different data types. Or maybe only stringNotEmpty is required.

containsData has only been replaced where it would cause a blank summary to be displayed. I can change the code to use containsData more specifically if you think that would be more appropriate.

Tha's OK, just feel a bit rushed for a Friday and migration day I would love to have a separate PR to address this containsData. To be honest I don't like this containsData at all. I think it should be case by case. We use it mostly for string. As for checking an option exist or not. I think your code is a lot easier to read and understand {containsData(whenDidItHappen) vs {whenDidItHappen.incidentFrequency ?

The latter reads much better.

I agree we should have a separate PR to discuss this. If this is the correct way to do things, we should change the logic in PDF report together. After all, this has nothing to with "Edited" function.

justinr86 commented 3 years ago

The edit link looks good. But I feel the refactor of containsData should be in a separate PR. I agree containsData is overly complex. It's overly complex because it's too generic. I am happy to see it refactored based on different data types. Or maybe only stringNotEmpty is required.

containsData has only been replaced where it would cause a blank summary to be displayed. I can change the code to use containsData more specifically if you think that would be more appropriate.

Tha's OK, just feel a bit rushed for a Friday and migration day I would love to have a separate PR to address this containsData. To be honest I don't like this containsData at all. I think it should be case by case. We use it mostly for string. As for checking an option exist or not. I think your code is a lot easier to read and understand {containsData(whenDidItHappen) vs {whenDidItHappen.incidentFrequency ? The latter reads much better.

I agree we should have a separate PR to discuss this. If this is the correct way to do things, we should change the logic in PDF report together. After all, this has nothing to with "Edited" function.

This is related to marking confirmation summaries as edited. containsData will evaluate the object and return true if it contains anything. If an object contains the property edited: true it will display an empty summary.

This code change is limited to the confirmation summaries that will be affected by containsData.

cds-snc / report-a-cybercrime

Update confirmation #2443

Description

Any new packages installed?

Required followup work

Checklist: