[23.0][Instrument] Fix freeze on completed instruments #7344

jesscall · 2021-02-22T16:13:23Z

Brief summary of changes

Bugfix for disabling fields when instrument is set to complete.

Testing instructions (if applicable)

Make sure that fields are disabled when setting an instrument to Data Entry = Complete

jesscall · 2021-02-22T16:14:02Z

@driusan Can you help with this?

With the changes in this PR, the page still needs to be refreshed for freeze to take effect.
I've tried implementing the change at different levels of the call stack, but same issue.
Also tried unsuccessfully to add a redirect in order to force refresh, but it was giving me some issues.

driusan · 2021-02-22T18:26:23Z

I can't test very well because the 23 branch doesn't work with php 8, but it seems to work for me when I redirect in the process function if the Data Entry status changes. It doesn't work in the handle function because of the order of operations where the NDB_BVL_InstrumentStatus_ControlPanel->save method gets called.

diff --git a/php/libraries/NDB_BVL_Instrument.class.inc b/php/libraries/NDB_BVL_Instrument.class.inc
index 44aa274a3..82d12a5c5 100644
--- a/php/libraries/NDB_BVL_Instrument.class.inc
+++ b/php/libraries/NDB_BVL_Instrument.class.inc
@@ -11,7 +11,9 @@
  * @link     https://www.github.com/aces/Loris-Trunk/
  */
 use \Psr\Http\Message\ServerRequestInterface;
+use \Psr\Http\Server\RequestHandlerInterface;
 use \Psr\Http\Message\ResponseInterface;
+
 use \Loris\StudyEntities\Candidate\CandID;
 
 /**
@@ -51,6 +53,12 @@ abstract class NDB_BVL_Instrument extends NDB_Page
     public $XINRules;
     public $XINDebug;
 
+    /**
+     * A flag determining whether the ControlPanel changed
+     * the Data_entry flag status on this page load.
+     */
+    private $DataEntryChanged;
+
     /**
      * Test name (short name), equivalent to test_names.Test_name
      *
@@ -2402,6 +2410,8 @@ abstract class NDB_BVL_Instrument extends NDB_Page
             $config = NDB_Config::singleton();
             $paths  = $config->getSetting('paths');
 
+            $this->DataEntryChanged = false;
+
             // check if the file/class exists
             $BasePath = $paths['base'] . 'project/instruments/';
             $TestName = $this->testName;
@@ -2412,7 +2422,12 @@ abstract class NDB_BVL_Instrument extends NDB_Page
                 || file_exists($BasePath . "$TestName.linst")
             ) {
                 // save possible changes from the control panel...
+                $originalstatus = $controlPanel->getDataEntryStatus();
                 $controlPanel->save();
+
+                if ($controlPanel->getDataEntryStatus() !== $originalstatus) {
+                    $this->DataEntryChanged = true;
+                }
             }
 
             // display the control panel
@@ -2822,6 +2837,33 @@ abstract class NDB_BVL_Instrument extends NDB_Page
         return $this->jsonData;
     }
 
+      public function process(
+        ServerRequestInterface $request,
+        RequestHandlerInterface $handler
+    ) : ResponseInterface {
+        // The parent middleware needs to be called in order for
+        // the instrument control panel to saved, which triggers
+        // $this->DataEntryChanged being set appropriately.
+        $response = parent::process($request, $handler);
+
+        if ($this->DataEntryChanged) {
+            $sessionID = $request->getQueryParams()["sessionID"];
+            $candID    = $request->getQueryParams()["candID"];
+
+            $baseURL = \NDB_Factory::singleton()->settings()->getBaseURL();
+
+            $url = $baseURL . "/instruments/" .
+                urlencode($this->testName) .
+                '?commentID=' . urlencode($this->getCommentID()) .
+                '&candID=' . urlencode($candID) .
+                '&sessionID=' . urlencode($sessionID);
+            return (new \LORIS\Http\Response())
+                ->withStatus(303)
+                ->withHeader("Location", $url);
+        }
+        return $response;
+      }
+
     /**
      * A form extends the basic page handler to
      *  (1) call save() on a POST request
@@ -2839,14 +2881,14 @@ abstract class NDB_BVL_Instrument extends NDB_Page
         }
 
         // Disable form if data entry is complete
-        $commentID = $request->getAttribute("pageclass")->commentID;
         $user      = $request->getAttribute("user");
         $sessionID = $request->getQueryParams()["sessionID"];
+        $candID    = $request->getQueryParams()["candID"];
         $timepoint = \Timepoint::singleton($sessionID);
 
         // create an instrument status object
         $status = new \NDB_BVL_InstrumentStatus;
-        $status->select($commentID);
+        $status->select($this->commentID);
 
         // freeze the form to prevent data entry
         if ($status->getDataEntryStatus() != 'In Progress'
@@ -2863,7 +2905,7 @@ abstract class NDB_BVL_Instrument extends NDB_Page
             }
         }
 
-        return (new \Laminas\Diactoros\Response())
+        return (new \LORIS\Http\Response())
             ->withBody(new \LORIS\Http\StringStream($this->display() ?? ""));
     }
 }

kongtiaowang · 2021-02-25T00:37:19Z

@jesscall to pass test, you need to add "$this->DataEntryType = 'DirectEntry'; " into the setup function of "test/test_instrument/NDB_BVL_Instrument_testtest.class.inc"

kongtiaowang

LGTM

test/test_instrument/NDB_BVL_Instrument_testtest.class.inc

driusan · 2021-02-26T19:02:43Z

Why is testtest a DirectEntry instrument? AFAIK it's not used for testing surveys

kongtiaowang · 2021-03-02T16:51:11Z

rebase after this #7369

jesscall · 2021-03-02T16:56:00Z

Testing suite blocked by #7369

@driusan is it possible to have that merged soon?

stale

.github/workflows/loristest.yml

ridz1208

there seems to be hanges to files that are unrelated to the PR. I think this might be the result of a bad rebase.

test/test_instrument/NDB_BVL_Instrument_testtest.class.inc

driusan · 2021-03-16T15:22:21Z

test/test_instrument/NDB_BVL_Instrument_testtest.class.inc

@@ -42,6 +42,9 @@ class NDB_BVL_Instrument_testtest extends NDB_BVL_Instrument

        // setup the form
        $this->_setupForm();
+
+        // set data entry type
+        $this->DataEntryType = 'DirectEntry';


Why is this set to DirectEntry? As far as I know this test instrument isn't used as a survey instrument

I believe it is needed because the following logic was added in this PR and it affects testing.
@kongtiaowang can maybe provide more details

if ($this->preview !== true && $this->DataEntryType !== 'DirectEntry') { $this->freeze(); }

Hi @driusan ,the integration test of testtest instrument will test the direct input information. I added DirectEntry since @jesscall have modifed the DataEntryType rule.

You have the following 5 conditions that would make the instrument be frozen

Instrument is NOT in progress

the user does not have data_entry permission

the user does not have access to the timepoint at which the instrument is instantiated

the instrument is not in preview mode

the instrument is not a direct entry

Why choose to solve the problem with option 5 ? I would say options 1, 2 and 3 are your easy logical solutions !

I think Rida means "make the instrument not be frozen". Assuming he does, I agree. "Direct entry" means survey mode and doesn't get frozen because when it's set, the participant is entering data directly (after the entry is complete they can no longer access the URL..)

If the test doing data entry on an instrument that was supposed to be frozen and is now broken because the freeze bug is fixed, the solution should be to fix the test and/or data, not fake being a survey instrument for testing non-survey functionality in order to work around the freeze call.

Thanks for the feedback.
I agree with Dave, it's likely an issue with the test doing data entry on an instrument meant to be frozen and this fix is messing with that. In that case the test case should be updated.

Pushing Rida's recent feedback and will monitor the test suite logs for indications of where the issue might be.

same as Dave's question

php/libraries/NDB_BVL_Instrument.class.inc

ridz1208 · 2021-03-18T13:27:49Z

test/test_instrument/NDB_BVL_Instrument_testtest.class.inc

@@ -42,6 +42,9 @@ class NDB_BVL_Instrument_testtest extends NDB_BVL_Instrument

        // setup the form
        $this->_setupForm();
+
+        // set data entry type
+        $this->DataEntryType = 'DirectEntry';


You have the following 5 conditions that would make the instrument be frozen

Instrument is NOT in progress

the user does not have data_entry permission

the user does not have access to the timepoint at which the instrument is instantiated

the instrument is not in preview mode

the instrument is not a direct entry

Why choose to solve the problem with option 5 ? I would say options 1, 2 and 3 are your easy logical solutions !

ridz1208 · 2021-03-18T13:29:24Z

php/libraries/NDB_BVL_Instrument.class.inc

+            && !in_array(
+                $timepoint->getData("CenterID"),
+                $user->getData("CenterIDs")


please add the third argument true to the in_array function to avoid unexpected behaviour

I think $user->hasCenter($timepoint->getCenterID()) would be better so that it doesn't need to change after #7359 (and it's more explicit what it's trying to do..)

kongtiaowang · 2021-03-19T17:30:08Z

hi @jesscall, when the user has data_entry permission or status is in_progress, the form always is frozen.

ridz1208 · 2021-03-19T18:51:56Z

php/libraries/NDB_BVL_Instrument.class.inc

+        if ($status->getDataEntryStatus() != 'In Progress'
+            || !$user->hasPermission('data_entry')
+            || (isset($timepoint)
+            && !in_array(
+                $timepoint->getData("CenterID"),
+                $user->getData("CenterIDs"),
+                true
+            ))
+            || $this->preview !== true
+            || $this->DataEntryType !== 'DirectEntry'
+        ) {


Suggested change

if ($status->getDataEntryStatus() != 'In Progress'

|| !$user->hasPermission('data_entry')

|| (isset($timepoint)

&& !in_array(

$timepoint->getData("CenterID"),

$user->getData("CenterIDs"),

true

))

|| $this->preview !== true

|| $this->DataEntryType !== 'DirectEntry'

) {

if ($status->getDataEntryStatus() != 'In Progress'

|| !$user->hasPermission('data_entry')

|| (isset($timepoint)

&& !in_array(

$timepoint->getData("CenterID"),

$user->getData("CenterIDs"),

true

))

&& $this->preview !== true

&& $this->DataEntryType !== 'DirectEntry'

) {

"Move the if into the same statement", they said.

"That'll make it simpler," they said.

kongtiaowang · 2021-03-19T20:28:00Z

@jesscall please add "'Data_entry'=> 'In Progress', " into test/test_instrument/test_instrumentTest.php at line 58, it will pass the integration test.

ridz1208 · 2021-03-22T15:29:53Z

@jesscall this is in your court

jesscall added Category: Bug PR or issue that aims to report or fix a bug Critical to release PR or issue is key for the release to which it has been assigned Priority: High PR or issue should be prioritised over others for review and testing labels Feb 22, 2021

jesscall requested a review from driusan February 22, 2021 16:13

kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Feb 25, 2021

kongtiaowang approved these changes Feb 25, 2021

View reviewed changes

ridz1208 previously requested changes Feb 26, 2021

View reviewed changes

test/test_instrument/NDB_BVL_Instrument_testtest.class.inc Outdated Show resolved Hide resolved

jesscall force-pushed the 2021_02_16_freeze_completed_instr branch from a63c8a9 to 1d9ebe7 Compare March 2, 2021 16:54

jesscall and others added 9 commits March 8, 2021 12:17

first pass

079fe83

refresh ?

f3bd037

[23.0][Instrument] Fix freeze on complete forms

bc8792e

phpcs

b6cdef4

Dave's suggestion

e8784e2

Install php-ast with setup-php config

1a2849e

formatting

3eadb6c

pass tests

3680fb6

instument test direct entry

2a8070b

jesscall force-pushed the 2021_02_16_freeze_completed_instr branch from 1d9ebe7 to 2a8070b Compare March 8, 2021 17:18

jesscall requested a review from ridz1208 March 8, 2021 17:43

ridz1208 reviewed Mar 8, 2021

View reviewed changes

.github/workflows/loristest.yml Outdated Show resolved Hide resolved

ridz1208 previously requested changes Mar 8, 2021

View reviewed changes

test/test_instrument/NDB_BVL_Instrument_testtest.class.inc Outdated Show resolved Hide resolved

jesscall and others added 2 commits March 8, 2021 13:04

cherrypick

d3e7f58

spacing

376ab16

jesscall requested a review from ridz1208 March 16, 2021 14:53

driusan reviewed Mar 16, 2021

View reviewed changes

ridz1208 requested changes Mar 18, 2021

View reviewed changes

ridz1208 reviewed Mar 18, 2021

View reviewed changes

jesscall added 3 commits March 18, 2021 12:46

Rida's feedback

cdb87e4

conflict resolution

ade1a76

hasCenter

5ff5406

kongtiaowang added State: Needs work PR awaiting additional work by the author to proceed and removed Passed manual tests PR has been successfully tested by at least one peer labels Mar 19, 2021

ridz1208 reviewed Mar 19, 2021

View reviewed changes

jesscall added 2 commits March 19, 2021 15:26

logic

7587252

fic comment

e4cf39a

kongtiaowang added Passed manual tests PR has been successfully tested by at least one peer and removed State: Needs work PR awaiting additional work by the author to proceed labels Mar 19, 2021

ridz1208 approved these changes Mar 22, 2021

View reviewed changes

integration test, shen's recommendation

a5d17e9

driusan merged commit 3d3f430 into aces:23.0-release Mar 22, 2021

ridz1208 added this to the 23.0.3 milestone Mar 30, 2021

ridz1208 mentioned this pull request Apr 9, 2021

[instrument] form frozen on first opening #7420

Closed

ridz1208 mentioned this pull request Aug 26, 2021

[Instrument] Fix freeze form logic bug #7530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[23.0][Instrument] Fix freeze on completed instruments #7344

[23.0][Instrument] Fix freeze on completed instruments #7344

jesscall commented Feb 22, 2021

jesscall commented Feb 22, 2021 •

edited

Loading

driusan commented Feb 22, 2021

kongtiaowang commented Feb 25, 2021

kongtiaowang left a comment

driusan commented Feb 26, 2021

kongtiaowang commented Mar 2, 2021 •

edited

Loading

jesscall commented Mar 2, 2021

ridz1208 left a comment

driusan Mar 16, 2021

jesscall Mar 16, 2021

kongtiaowang Mar 16, 2021

ridz1208 Mar 18, 2021

driusan Mar 18, 2021

jesscall Mar 18, 2021

ridz1208 Mar 18, 2021

ridz1208 Mar 18, 2021

driusan Mar 18, 2021

kongtiaowang commented Mar 19, 2021 •

edited

Loading

ridz1208 Mar 19, 2021 •

edited

Loading

driusan Mar 19, 2021

kongtiaowang commented Mar 19, 2021

ridz1208 commented Mar 22, 2021

[23.0][Instrument] Fix freeze on completed instruments #7344

[23.0][Instrument] Fix freeze on completed instruments #7344

Conversation

jesscall commented Feb 22, 2021

Brief summary of changes

Testing instructions (if applicable)

jesscall commented Feb 22, 2021 • edited Loading

driusan commented Feb 22, 2021

kongtiaowang commented Feb 25, 2021

kongtiaowang left a comment

Choose a reason for hiding this comment

driusan commented Feb 26, 2021

kongtiaowang commented Mar 2, 2021 • edited Loading

jesscall commented Mar 2, 2021

ridz1208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kongtiaowang commented Mar 19, 2021 • edited Loading

ridz1208 Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kongtiaowang commented Mar 19, 2021

ridz1208 commented Mar 22, 2021

jesscall commented Feb 22, 2021 •

edited

Loading

kongtiaowang commented Mar 2, 2021 •

edited

Loading

kongtiaowang commented Mar 19, 2021 •

edited

Loading

ridz1208 Mar 19, 2021 •

edited

Loading