Update confirmation #2443

justinr86 · 2020-10-29T19:50:30Z

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 · 2020-10-29T19:58:59Z

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

new alerts:

5 for Unused variable, import, function or class

ying-chen-cds · 2020-10-30T13:26:55Z

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 · 2020-10-30T13:37:23Z

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

justinr86 · 2020-10-30T13:38:42Z

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 · 2020-10-30T13:40:44Z

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 · 2020-10-30T13:49:57Z

f2/src/summary/HowDidItStartSummary.js

          />
        </Flex>

-        {containsData(howdiditstart) ? (
+        {howdiditstart.howDidTheyReachYou.length ? (


Is there a reason not to use containsData? It's being used everywhere

containsData simply checks if the object contains anything. In this case if the field is flagged as edited it will render an empty confirmation summary. This way it will render "No information provided." unless a selection has been made.

sdzhangtao · 2020-10-30T13:52:51Z

f2/src/summary/WhatWasAffectedSummary.js

            label="confirmationPage.ImpactTitle.edit"
+            path="/whatwasaffected"
+            edited={impact.edited}
          />
        </Flex>
        {containsData(impact) ? (


I know containsData is overly complex. But for string operation, it makes sense to have a check that the string is not empty.
str is not undefined, is not null, size is not zero and doesn't just contain white spaces

dengcn

I would recommend to close this PR for now, it touched many files. We may not catch the Azure migration in couple of hours. All the work already in the branch which will go to Azure. Once azure is done, you can recreate PR so have more time to clean up. Rush to master may cause more request coming.

sdzhangtao · 2020-10-30T13:56:39Z

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 · 2020-10-30T14:01:37Z

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 · 2020-10-30T14:17:14Z

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 · 2020-10-30T14:23:43Z

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 · 2020-10-30T14:24:07Z

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 · 2020-10-30T14:53:25Z

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 · 2020-10-30T15:07:37Z

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.

DianeLiu2019

all pages works well except anonymous location page. the edit button don't change to purple , and the( Eddited ) don't show up after editing the page

justinr86 added 23 commits October 2, 2020 14:13

Update package-lock.json

a055e22

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

0beddd7

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

061798f

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

5a9d8f5

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

a5a5783

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

5e00778

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

2e2d58c

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

59a3d12

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

05bab20

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

1cd2034

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

d0a73b2

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

04aefb8

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

09865fa

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

243d577

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

5872c0d

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

deb29b0

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

c825585

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

adf8d98

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

d9e92ec

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

a075716

Merge branch 'master' of https://github.com/cds-snc/report-a-cybercrime

1e32f07

Indicate field has been modified in confirmation summary

0f2b10e

Indicate field has been modified in confirmation summary

11b55b3

sastels temporarily deployed to rac-prototyp-update-con-rvfubq October 29, 2020 19:50 Inactive

Merge branch 'master' into update-confirmation

cd0d4e0

sastels temporarily deployed to rac-prototyp-update-con-rvfubq October 29, 2020 19:54 Inactive

justinr86 requested review from DianeLiu2019, dengcn, kevinyang06 and ngosset October 29, 2020 19:55

justinr86 requested a review from ying-chen-cds October 29, 2020 19:55

justinr86 and others added 2 commits October 30, 2020 07:21

Remove unused import

ed116ea

Merge branch 'master' into update-confirmation

6023496

sastels temporarily deployed to rac-prototyp-update-con-rvfubq October 30, 2020 11:24 Inactive

Merge conflicts

3c038be

sastels temporarily deployed to rac-prototyp-update-con-rvfubq October 30, 2020 11:27 Inactive

sdzhangtao reviewed Oct 30, 2020

View reviewed changes

dengcn reviewed Oct 30, 2020

View reviewed changes

DianeLiu2019 reviewed Oct 30, 2020

View reviewed changes

sdzhangtao requested review from ying-chen-cds and sdzhangtao and removed request for ying-chen-cds and sdzhangtao October 31, 2020 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update confirmation #2443

Update confirmation #2443

justinr86 commented Oct 29, 2020

lgtm-com bot commented Oct 29, 2020

ying-chen-cds commented Oct 30, 2020

sdzhangtao commented Oct 30, 2020

justinr86 commented Oct 30, 2020

justinr86 commented Oct 30, 2020

sdzhangtao Oct 30, 2020

justinr86 Oct 30, 2020

sdzhangtao Oct 30, 2020

dengcn left a comment

sdzhangtao commented Oct 30, 2020

justinr86 commented Oct 30, 2020

sdzhangtao commented Oct 30, 2020

justinr86 commented Oct 30, 2020

ying-chen-cds commented Oct 30, 2020

ying-chen-cds commented Oct 30, 2020

justinr86 commented Oct 30, 2020

DianeLiu2019 left a comment

Update confirmation #2443

Are you sure you want to change the base?

Update confirmation #2443

Conversation

justinr86 commented Oct 29, 2020

Description

Any new packages installed?

Required followup work

Checklist:

lgtm-com bot commented Oct 29, 2020

ying-chen-cds commented Oct 30, 2020

sdzhangtao commented Oct 30, 2020

justinr86 commented Oct 30, 2020

justinr86 commented Oct 30, 2020

sdzhangtao Oct 30, 2020

Choose a reason for hiding this comment

justinr86 Oct 30, 2020

Choose a reason for hiding this comment

sdzhangtao Oct 30, 2020

Choose a reason for hiding this comment

dengcn left a comment

Choose a reason for hiding this comment

sdzhangtao commented Oct 30, 2020

justinr86 commented Oct 30, 2020

sdzhangtao commented Oct 30, 2020

justinr86 commented Oct 30, 2020

ying-chen-cds commented Oct 30, 2020

ying-chen-cds commented Oct 30, 2020

justinr86 commented Oct 30, 2020

DianeLiu2019 left a comment

Choose a reason for hiding this comment