Fi 900 - generic ui fields - proof of concept #470

ghost · 2020-07-16T12:30:19Z

This is adds a way for sequences to specify their own prerequisites without changing the the view erb files or changing the testinginstance data model. They would just "require" whatever prerequisites they need and the UI and database entry for it would be made by Inferno. I have it working with US Core tests. The patient id's field and device codes field are not in the default.erb file, but you can see them in the UI when running the tests. I also tested with my generated ukcore code, and confirmed it was working for the new fields there as well.

To require a new field, you'll first need to add it in the module yaml file. In the uscore_v3.1.0_module.yml file, you can see the sequence_requirements section. To add a new requirement, just add one with a label. Then in a sequence file, add that requirement.

Submitter:

This pull request describes why these changes were made
Internal ticket for this PR:
Internal ticket links to this PR
Internal ticket is properly labeled (Community/Program)
Internal ticket has a justification for its Community/Program label
Code diff has been reviewed for extraneous/missing code
Tests are included and test edge cases
Tests/code quality metrics have been run locally and pass

Reviewer 1:

Name:

Code is maintainable and reusable, reuses existing code and infrastructure
where appropriate, and accomplishes the task's purpose
The tests appropriately test the new code, including edge cases
You have tried to break the code

Reviewer 2:

Name:

Code is maintainable and reusable, reuses existing code and infrastructure
where appropriate, and accomplishes the task's purpose
The tests appropriately test the new code, including edge cases
You have tried to break the code

radamson · 2020-07-30T16:36:27Z

I think adding the labels and descriptions in the module.yml is a good way to start, but I'm worried about having to manage and update the prerequisites in multiple places. Right now we have a situation where they need to be edited in the ERBs, model and test code and its a bear to deal with.

I'm not sure if its the best way to do it, but it might be nice to be able to add the label and description when defining the prerequisite so that it all happens in the same place. Otherwise I could see the prerequisites getting out of sync where maybe we forget to add a label in the config or have a label in the config, but never use it.

I don't want perfect to get in the way of good, but I think longer term we might want to be able to centralize the management of prereqs.

lib/app/sequence_base.rb

arscan · 2020-07-30T16:57:23Z

I think adding the labels and descriptions in the module.yml is a good way to start, but I'm worried about having to manage and update the prerequisites in multiple places. Right now we have a situation where they need to be edited in the ERBs, model and test code and its a bear to deal with.

Could take care of this using ruby. By default have these just be defined completely in the sequences. And if you use in multiple sequences place in a common ruby file / module and include the common file. And/or you could have it be smart enough that only 1 sequence has to include the label/description portions, and in the other just require the parameter name it is good enough.

But from a workflow perspective, maybe it does make sense to have a module file on the top where you define what global variables there are and what gets 'exported'?

lib/app/models/sequence_requirement.rb

lib/app/models/testing_instance.rb

lib/app/sequence_base.rb

lib/app/models/testing_instance.rb

Jammjammjamm · 2020-08-04T14:07:13Z

lib/app/sequence_base.rb

+
+          instance_class.define_method requirement_setter_name do
+            requirement_found = sequence_requirements.find { |requirement| requirement.name == requirement_name.to_s }
+            return unless requirement_found.present?


Should this create the requirement if it isn't found?

I added this so the rake tests pass, but it also works so if you don't have the requirement in the module file, you can still use the requirement. It just won't have a nice label or description.

…dules

…R feedback

Jammjammjamm · 2020-08-07T12:32:38Z

lib/app/models/testing_instance.rb

+        # Get the conditions that determine whether the record already exists
+        conditions = attributes.select { |key, _value| condition_attributes.include?(key) }
+
+        SequenceRequirement.first_or_create(conditions, attributes)


I think lines 287-290 are overcomplicating things.

Suggested change

SequenceRequirement.first_or_create(conditions, attributes)

SequenceRequirement.first_or_create({ name: requirement[:name], testing_instance: self }, attributes)

Jammjammjamm · 2020-08-07T12:34:23Z

lib/app/models/testing_instance.rb

+      def get_sequence_requirement(requirement)
+        return unless requirement&.dig(:name)
+
+        attributes = { name: requirement[:name],


Don't need to set name: requirement[:name] if we're then merging in requirement.

Jammjammjamm · 2020-08-07T12:35:35Z

lib/app/models/module.rb

@@ -37,6 +38,13 @@ def initialize(params)
          test_sets[test_set_key] = TestSet.new(test_set_key, test_set)
        end
      end
+      if params[:sequence_requirements].present?
+        @sequence_requirements = {}.tap do |requirements|
+          params[:sequence_requirements].each do |requirement_key, texts|


Why is this value called texts? The same thing is done in TestingInstance.

changed name to req_description.

Jammjammjamm · 2020-08-07T12:36:56Z

lib/app/models/testing_instance.rb

+
+      def set_requirement_value(requirement_name, value)
+        get_sequence_requirement(name: requirement_name.to_s, testing_instance: self).update(value: value)
+        self.patient_id = value if requirement_name == 'patient_id'


I hope we're not keeping this in the final version.

Took this out. Forgot we have the setters now so I'm back to using that in the test_set_endpoint

arscan · 2020-08-07T18:12:17Z

Not a blocking issue, but I was very specific about how we ordered the fields that come up in the form. At least now, the order is not quite right -- though that may be because this is a mix of the old and new methods... perhaps if we went to all the new method they will show up in the interface in the order that they are put in the module file?

arscan

This looks like an improvement for sure. We've got a ways to go, but this moves it in the right direction and any issues I have with it can be addressed later.

ghost added the WIP Work in progress label Jul 16, 2020

okeefm requested review from Jammjammjamm, radamson and arscan July 28, 2020 17:12

radamson reviewed Jul 30, 2020

View reviewed changes

lib/app/sequence_base.rb Outdated Show resolved Hide resolved

Jammjammjamm reviewed Aug 4, 2020

View reviewed changes

ghost removed the WIP Work in progress label Aug 4, 2020

radamson force-pushed the fi-900-generic-ui-fields branch from 55403e1 to 2265480 Compare August 5, 2020 15:19

plump-pinniped and others added 12 commits August 6, 2020 12:49

proof of concept for generic UI fields and database changes

b36a376

adding bearer token field back in

de5a4c5

fix whitespace

1f464a9

accidentally erased commas

edfb4bb

add getters/setters and labels/descriptions in the module file

1dccc6d

change new_requires to requires and fix crash when launching older mo…

f70ea96

…dules

create requirements if not set and fix rake issues

56f19cd

add unique property to name in sequencerequirement

41c8751

fix rubocop

cc72dfb

use create instead of new+push

477d0bf

update add checks to prevent errors with the unique constraint

30566bd

fix some issues

a41b870

radamson force-pushed the fi-900-generic-ui-fields branch from c25214d to a41b870 Compare August 6, 2020 16:49

Chase and others added 4 commits August 6, 2020 13:12

don't duplicate patient_id field

dc7f11b

support legacy patient_id

79e3f16

use first_or_create

d70ceeb

remove test_instance_id from first_or_create calls based on earlier P…

cf18717

…R feedback

Jammjammjamm reviewed Aug 7, 2020

View reviewed changes

radamson and others added 2 commits August 7, 2020 09:06

simply get_sequence_requirements based on PR feedback

094c478

use setter methods and change variable name

4387859

radamson approved these changes Aug 7, 2020

View reviewed changes

arscan approved these changes Aug 7, 2020

View reviewed changes

radamson merged commit 8fcfc49 into development Aug 7, 2020

radamson deleted the fi-900-generic-ui-fields branch August 7, 2020 18:14

radamson mentioned this pull request Aug 9, 2020

Version 2.10.0 #481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fi 900 - generic ui fields - proof of concept #470

Fi 900 - generic ui fields - proof of concept #470

ghost commented Jul 16, 2020 •

edited by ghost

radamson commented Jul 30, 2020

arscan commented Jul 30, 2020 •

edited

Jammjammjamm Aug 4, 2020

ghost Aug 5, 2020

Jammjammjamm Aug 7, 2020

Jammjammjamm Aug 7, 2020

Jammjammjamm Aug 7, 2020

ghost Aug 7, 2020

Jammjammjamm Aug 7, 2020

ghost Aug 7, 2020

arscan commented Aug 7, 2020

arscan left a comment

	SequenceRequirement.first_or_create(conditions, attributes)
	SequenceRequirement.first_or_create({ name: requirement[:name], testing_instance: self }, attributes)

Fi 900 - generic ui fields - proof of concept #470

Fi 900 - generic ui fields - proof of concept #470

Conversation

ghost commented Jul 16, 2020 • edited by ghost

radamson commented Jul 30, 2020

arscan commented Jul 30, 2020 • edited

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

arscan commented Aug 7, 2020

arscan left a comment

Choose a reason for hiding this comment

ghost commented Jul 16, 2020 •

edited by ghost

arscan commented Jul 30, 2020 •

edited