[Instrument] Rename Data_entry_completion_status & move to its own column in flag #6876

CamilleBeau · 2020-07-29T19:32:14Z

Brief summary of changes

This PR moves Data_entry_completion_status field to its own column in flag following discussion and renames to Required_elements_completed. This is accomplished with the help of a patch and a single-use script (the patch must be run before the script).

Testing instructions (if applicable)

Run patches SQL/New_patches/2020-07-29_Required_elements_completed_flag_column.sql and SQL/Cleanup_patches/2020-12-08-remove_dataEntryCompletionStatus_conflict_resolver.sql
Run php tools/single_use/Set_Required_elements_completed_flag.php. Check that the script successfully sets the new column to Y for instruments that already have Data_entry_completion_status=Complete and to N for instruments that had Data_entry_completion_status=Incomplete. Make sure that this holds for both JSON and non JSON instruments.
Run php tools/single_use/Remove_Data_entry_completion_status_instr_column.php
Check that the Data column of flag no longer contains any Data_entry_completion_status values.
Check in the backend that Data_entry_completion_status is only set to "Complete" when all required elements have been filled & saved
Check on the front end that the functionality for being able to set the instrument to complete remains the same (e.g. that you only have the option of setting Data_entry to "Complete" if Required_elements_completed='Y'

Link(s) to related issue(s)

Resolves Mistaken field created in _nullStatus function of NDB_BVL_Instrument #6746

php/libraries/NDB_BVL_Instrument.class.inc

ridz1208

This PR is missing CHANGELOG updates as well as the application of the patch on the RB dataset

CHANGELOG.md

stale

CamilleBeau · 2020-09-11T17:22:07Z

@kongtiaowang @AlexandraLivadas Would you be able to help me figure out what's going on with travis here?

CamilleBeau · 2020-09-23T17:58:45Z

@kongtiaowang I am a little bit confused about adding back the Data_entry_completion_status to the testLoadInstanceData and setFakeTableData functions in NDB_BVL_Instrument_Test since we are removing this column from instrument tables in this PR.

kongtiaowang

In this case,
$this->_instrument->table = 'flag';
$defaults = $this->_instrument->getInstanceData();
It means that the instranceData from flag table.
you have inserted 'Data_entry_completion_status' into flag table.That is why the 'Data_entry_completion_status' is here.

test/unittests/NDB_BVL_Instrument_Test.php

CamilleBeau · 2020-11-26T15:49:16Z

Data_entry_completion_status should not be present on line 1150 of NDB_BVL_Instrument_Test since this is testing getInstanceData, which takes from the instrument table data or the Data column of flag. This PR removes Data_entry_completion_status from both of those places and puts it in its own column in flag, so getInstanceData should not be returning it.

However, removing the field from the array causes the test suite to fail. Following discussion with Shen, this may be because the unittest does not run the scripts in this PR. The field can therefore be removed from the test file in a future PR after all of the scripts from this PR are run and the deprecation of Data_entry_completion_status in instrument data is complete.

ridz1208

you forgot the necessary changes to tools script (scripts that generate instrument tables mostly)
you are missing all changes in the conflict resolver tables where Data_entry_completion_status might appear as a conflict as well as the removal of that value from the $_doubleDataEntryDiffIgnoreColumns array of the instrument classes (this array exists in multiple places you should search for it)
It's a "convoluted" implementation of a change that should have made a bunch of stuff simpler, not more complex. You are removing the Data_entry_completion_status from within instrument data and yet choose to still use the instrument save function to save it when the saving functionality is already pretty-wrapped in a _setDataEntryCompletionStatus method... seems like the intuitive way is to modify that to simply store the value in flag rather then modify the save function to handle a field that is no longer in it's scope.

modules/imaging_qc/php/imaging_qc.class.inc

php/libraries/NDB_BVL_Instrument.class.inc

CamilleBeau · 2020-12-07T20:35:54Z

3. It's a "convoluted" implementation of a change that should have made a bunch of stuff simpler, not more complex. You are removing the Data_entry_completion_status from within instrument data and yet choose to still use the instrument save function to save it when the saving functionality is already pretty-wrapped in a _setDataEntryCompletionStatus method... seems like the intuitive way is to modify that to simply store the value in flag rather then modify the save function to handle a field that is no longer in it's scope.

I addressed most of your comments in the latest commit. For this one, I put a direct update to flag in the _setDataEntryCompletionStatus method, and removed the branching for data_entry_completion_status in the _save function. Is this what you meant?

SQL/0000-00-00-schema.sql

php/libraries/NDB_BVL_Instrument.class.inc

CamilleBeau · 2020-12-17T19:57:53Z

@ridz1208 After making all of the changes in this PR & running source_rb from the branch, I get the following error:

Do you know what's causing this / what I should change?

ridz1208 · 2020-12-17T20:01:33Z

@CamilleBeau My guess would be that you have an invalid default value

test/unittests/NDB_BVL_Instrument_Test.php

php/libraries/NDB_BVL_Instrument.class.inc

cmadjar · 2021-06-08T13:13:21Z

@CamilleBeau could you rebase?
@driusan once rebase, ready for you to review.

driusan · 2021-09-24T16:41:04Z

@zaliqarosli it's not quite clear from the above thread and it's showing up as approved.. is this ready to merge or are there still changes to do?

zaliqarosli

requesting changes:

@CamilleBeau i think it's good that the value is updated if it has to be. but is there maybe a way to check inconsistencies and print them out to ask for confirmation before continuing? let me know if i can be clearer

zaliqarosli

i don't think its necessary to print out every commentID being checked in the flag table.
i think an array of mismatched results will suffice. if you find the printout useful, maybe you can add a -verbose option

(loris-mri-python) lorisadmin@zrosli-dev:/var/www/loris$ php tools/single_use/Set_Required_elements_completed_flag.php 

*** Set Data_entry_completion_satus in flag table ***

####################################################################################
This script is to be run for one time use only to update and rename the 
Data_entry_completion_status field. 

Without being run with 'confirm', the script only reports mismatching data. If run 
with 'confirm', the new Required_elements_completed flag is updated by using the 
_determineRequiredElementsCompletedFlag and _setRequiredElementsCompletedFlag 
functions. An array is printed at the end of the script that lists all the cases 
where the new Required_elements_completed flag does not match the old 
Data_entry_completion_status. If the Required_elements_completed flag is not updated
through this script or through instrument saving, it's default is 'N'.

After running this script with 'confirm', run 
Remove_Data_entry_completion_status_instr_column.php to generate a DROP COLUMN patch
to remove the old column from instrument tables.
####################################################################################


Checking mismatching flag for 300001MTL0011211468524336.
Checking mismatching flag for 300001MTL0011221465332127.
Checking mismatching flag for 300001MTL0011231524315839.
Checking mismatching flag for 300001MTL0011241522092423.
Checking mismatching flag for 300001MTL0011261522092728.
Checking mismatching flag for 300001MTL0011347211524056560.
Checking mismatching flag for 300001MTL0011347241524056560.
Checking mismatching flag for 300002MTL0021189211524318806.
Checking mismatching flag for 300002MTL0021189241524318806.
Checking mismatching flag for 300002MTL0021643231524668096.
Checking mismatching flag for 300002MTL0021643241524318806.
Checking mismatching flag for 300002MTL0021643261524668096.
Checking mismatching flag for 300002MTL0022211468524336.
Checking mismatching flag for 300002MTL0022221465351036.
Checking mismatching flag for 300002MTL0022231524315839.
Checking mismatching flag for 300002MTL0022241522092423.
Checking mismatching flag for 300002MTL0022261522092728.
Checking mismatching flag for 300003MTL0033211468524337.
Checking mismatching flag for 300003MTL0033221465351036.

i think this script is going to take a long time to complete if it prints every row

CamilleBeau · 2021-10-15T13:39:47Z

i don't think its necessary to print out every commentID being checked in the flag table. i think an array of mismatched results will suffice. if you find the printout useful, maybe you can add a -verbose option

Thanks @zaliqarosli ! Printout of each data point removed, ready for re-review

zaliqarosli

something seems off with determining the Data entry completion status:

    [DDE_475906DCC4222142121524502652] => Array
        (
            [Data entry completion status] => Complete
            [New Required elements completed flag] => N
        )

mysql> select Data_entry_completion_status from bmi where CommentID='DDE_475906DCC4222142121524502652';
+------------------------------+
| Data_entry_completion_status |
+------------------------------+
| Incomplete                   |
+------------------------------+
1 row in set (2.07 sec)

SQL/Cleanup_patches/2020-12-08-remove_dataEntryCompletionStatus_conflict_resolver.sql

CamilleBeau · 2021-10-15T16:59:08Z

something seems off with determining the Data entry completion status:

Thanks for the catch @zaliqarosli .. I tried to replicate this and debug but I wasn't able to. I noticed though that I wasn't resetting the $instrDECS value for each iteration of the for loop. That could have been causing the problem

zaliqarosli


    [DDE_587630DCC0901053121524238782] => Array
        (
            [Data entry completion status] => 
            [New Required elements completed flag] => N
        )

    [DDE_676061DCC2922145221524505477] => Array
        (
            [Data entry completion status] => 
            [New Required elements completed flag] => N
        )

    [DDE_947103DCC7022155121524664375] => Array
        (
            [Data entry completion status] => 
            [New Required elements completed flag] => N
        )

i think its your logic between and around lines 80-90. DDE_947103DCC7022155121524664375 in raisin bread has NULL Data column in flag table and is linst instrument which i believe by default uses jsonData but i could be wrong here.
so i think you're missing the case for when the instrument uses json data but the data column is empty. so you would still have to grab the value from the instrument table, or default the value to 'incomplete' if the data column is empty

CamilleBeau · 2021-10-18T16:02:20Z

so i think you're missing the case for when the instrument uses json data but the data column is empty. so you would still have to grab the value from the instrument table, or default the value to 'incomplete' if the data column is empty

@zaliqarosli Thank you Zaliqa! I changed it so that a null value is considered as 'Incomplete'. We can not take from the instrument table in the case of json instruments because the table may not exist.

zaliqarosli

looks like the logic is fixed! i would just ask before approving this PR that the script documentation clearly states that running the script with 'confirm' argument will over write the DECS with the determined completed flag in the case where there is a mismatch, i.e. making it clear that there will be a loss / modification of data for projects

CamilleBeau · 2021-10-20T14:46:03Z

@zaliqarosli Edited the info printed by the script to specify this

driusan · 2021-10-29T15:40:15Z

CHANGELOG.md

@@ -45,6 +46,8 @@ requesting a new account and will be displayed in the User Accounts module (PR #
 ### Notes For Existing Projects
 - New function Candidate::getSubjectForMostRecentVisit replaces Utility::getSubprojectIDUsingCandID, adding ability to determine which subproject a candidate belongs to given their most recent visit.
 - LINST instrument class was modified to implement the getFullName() and getSubtestList() functions thus making entries in the test_names and instrument_subtests tables respectively unnecessary for LINST instruments (PR #7169)
+- The `Data_entry_completion_status` column of instrument tables has been migrated to its own column in flag, and renamed to `Required_elements_completed`. After script `Set_Required_elements_completed_flag.php` is run, projects will need to delete the `Data_entry_completion_status` column of instrument tables. This can be accomplished by running `Remove_Data_entry_completion_status_instr_column.php`, and then sourcing the patch generated by this script.
+- Make sure to replace all instances of `_setDataEntryCompletionStatus`, `_determineDataEntryCompletionStatus`, and `updateDataEntryCompletionStatus` functions with their newly named counterparts, `_setRequiredElementsCompletedFlag`, `_determineRequiredElementsCompletedFlag`, `updateRequiredElementsCompletedFlag`


I don't understand this. Replace them where?

This is meant to be a note for projects to replace these functions in case they are calling them in any overrides

I re-worded it to be more clear

tools/single_use/Remove_Data_entry_completion_status_instr_column.php

driusan

approved once the typo in suggested changes is fixed

…umn.php Co-authored-by: Dave MacFarlane <[email protected]>

ridz1208 · 2022-02-05T19:46:17Z

tools/single_use/Set_Required_elements_completed_flag.php

@@ -0,0 +1,134 @@
+<?php


driusan reviewed Aug 11, 2020

View reviewed changes

php/libraries/NDB_BVL_Instrument.class.inc Outdated Show resolved Hide resolved

ridz1208 previously requested changes Aug 12, 2020

View reviewed changes

CamilleBeau added the State: Needs work PR awaiting additional work by the author to proceed label Aug 13, 2020

driusan reviewed Aug 17, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CamilleBeau removed the State: Needs work PR awaiting additional work by the author to proceed label Aug 25, 2020

CamilleBeau added the Blocking PR should be prioritized because it is blocking the progress of another task label Sep 1, 2020

CamilleBeau mentioned this pull request Sep 1, 2020

JSON data toolkit #6748

Closed

CamilleBeau added the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Sep 11, 2020

kongtiaowang removed the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Sep 22, 2020

kongtiaowang reviewed Nov 25, 2020

View reviewed changes

test/unittests/NDB_BVL_Instrument_Test.php Outdated Show resolved Hide resolved

ridz1208 previously requested changes Dec 2, 2020

View reviewed changes

CamilleBeau added the State: Needs work PR awaiting additional work by the author to proceed label Dec 7, 2020

driusan reviewed Dec 7, 2020

View reviewed changes

SQL/0000-00-00-schema.sql Outdated Show resolved Hide resolved

ridz1208 self-assigned this Dec 7, 2020

CamilleBeau force-pushed the 2020_07_29_Data_entry_completion_status_flag_column branch from a4594bf to 270bf44 Compare December 17, 2020 18:59

CamilleBeau changed the title ~~[Instrument] Move Data_entry_completion_status to its own column in flag~~ [Instrument] Rename Data_entry_completion_status & move to its own column in flag Dec 17, 2020

CamilleBeau commented Dec 17, 2020

View reviewed changes

php/libraries/NDB_BVL_Instrument.class.inc Outdated Show resolved Hide resolved

CamilleBeau commented Dec 17, 2020

View reviewed changes

test/unittests/NDB_BVL_Instrument_Test.php Outdated Show resolved Hide resolved

ridz1208 reviewed Dec 18, 2020

View reviewed changes

php/libraries/NDB_BVL_Instrument.class.inc Outdated Show resolved Hide resolved

cmadjar added State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed and removed State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed labels Jun 8, 2021

cmadjar unassigned ridz1208 Jun 8, 2021

CamilleBeau added 3 commits September 8, 2021 11:28

update pt 1

715cbf0

Add confirm argument to script

bdcbe5c

Fix fn name

42964ee

CamilleBeau removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Sep 8, 2021

driusan added the Critical to release PR or issue is key for the release to which it has been assigned label Sep 14, 2021

zaliqarosli requested changes Sep 28, 2021

View reviewed changes

zaliqarosli self-requested a review October 5, 2021 15:26

zaliqarosli requested changes Oct 13, 2021

View reviewed changes

Rm printout for each data point

738f26c

zaliqarosli requested changes Oct 15, 2021

View reviewed changes

SQL/Cleanup_patches/2020-12-08-remove_dataEntryCompletionStatus_conflict_resolver.sql Show resolved Hide resolved

Set instrDECS to null

64e7f2b

zaliqarosli requested changes Oct 15, 2021

View reviewed changes

Consider null as incomplete

3c110ce

zaliqarosli requested changes Oct 18, 2021

View reviewed changes

More specific script doc

d58a856

zaliqarosli approved these changes Oct 20, 2021

View reviewed changes

zaliqarosli added the Passed manual tests PR has been successfully tested by at least one peer label Oct 20, 2021

driusan reviewed Oct 29, 2021

View reviewed changes

tools/single_use/Remove_Data_entry_completion_status_instr_column.php Outdated Show resolved Hide resolved

driusan approved these changes Oct 29, 2021

View reviewed changes

CamilleBeau and others added 2 commits October 29, 2021 11:49

Update tools/single_use/Remove_Data_entry_completion_status_instr_col…

fe6a0c3

…umn.php Co-authored-by: Dave MacFarlane <[email protected]>

Clearer note

be1144a

driusan merged commit 1b5699b into aces:main Oct 29, 2021

ridz1208 added this to the 24.0.0 milestone Nov 4, 2021

ridz1208 reviewed Feb 5, 2022

View reviewed changes

tools/single_use/Set_Required_elements_completed_flag.php

@@ -0,0 +1,134 @@

<?php

Copy link

Collaborator

ridz1208 Feb 5, 2022

Choose a reason for hiding this comment

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

after SQL

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Instrument] Rename Data_entry_completion_status & move to its own column in flag #6876

[Instrument] Rename Data_entry_completion_status & move to its own column in flag #6876

CamilleBeau commented Jul 29, 2020 •

edited

Loading

ridz1208 left a comment

CamilleBeau commented Sep 11, 2020

CamilleBeau commented Sep 23, 2020

kongtiaowang left a comment

CamilleBeau commented Nov 26, 2020

ridz1208 left a comment

CamilleBeau commented Dec 7, 2020 •

edited

Loading

CamilleBeau commented Dec 17, 2020

ridz1208 commented Dec 17, 2020

cmadjar commented Jun 8, 2021

driusan commented Sep 24, 2021

zaliqarosli left a comment •

edited

Loading

zaliqarosli left a comment

CamilleBeau commented Oct 15, 2021

zaliqarosli left a comment

CamilleBeau commented Oct 15, 2021

zaliqarosli left a comment •

edited

Loading

CamilleBeau commented Oct 18, 2021

zaliqarosli left a comment

CamilleBeau commented Oct 20, 2021

driusan Oct 29, 2021

CamilleBeau Oct 29, 2021

CamilleBeau Oct 29, 2021

driusan left a comment

ridz1208 Feb 5, 2022

[Instrument] Rename Data_entry_completion_status & move to its own column in flag #6876

[Instrument] Rename Data_entry_completion_status & move to its own column in flag #6876

Conversation

CamilleBeau commented Jul 29, 2020 • edited Loading

Brief summary of changes

Testing instructions (if applicable)

Link(s) to related issue(s)

ridz1208 left a comment

Choose a reason for hiding this comment

CamilleBeau commented Sep 11, 2020

CamilleBeau commented Sep 23, 2020

kongtiaowang left a comment

Choose a reason for hiding this comment

CamilleBeau commented Nov 26, 2020

ridz1208 left a comment

Choose a reason for hiding this comment

CamilleBeau commented Dec 7, 2020 • edited Loading

CamilleBeau commented Dec 17, 2020

ridz1208 commented Dec 17, 2020

cmadjar commented Jun 8, 2021

driusan commented Sep 24, 2021

zaliqarosli left a comment • edited Loading

Choose a reason for hiding this comment

zaliqarosli left a comment

Choose a reason for hiding this comment

CamilleBeau commented Oct 15, 2021

zaliqarosli left a comment

Choose a reason for hiding this comment

CamilleBeau commented Oct 15, 2021

zaliqarosli left a comment • edited Loading

Choose a reason for hiding this comment

CamilleBeau commented Oct 18, 2021

zaliqarosli left a comment

Choose a reason for hiding this comment

CamilleBeau commented Oct 20, 2021

driusan Oct 29, 2021

Choose a reason for hiding this comment

CamilleBeau Oct 29, 2021

Choose a reason for hiding this comment

CamilleBeau Oct 29, 2021

Choose a reason for hiding this comment

driusan left a comment

Choose a reason for hiding this comment

ridz1208 Feb 5, 2022

Choose a reason for hiding this comment

CamilleBeau commented Jul 29, 2020 •

edited

Loading

CamilleBeau commented Dec 7, 2020 •

edited

Loading

zaliqarosli left a comment •

edited

Loading

zaliqarosli left a comment •

edited

Loading