Fi 601 Pass comma-separated patients to US Core #419

arscan · 2020-01-21T06:42:24Z

Pass a comma separated list of patients to US Core. A few loose ends need fixing, and unit tests need to be updated. For discussion. FYI @czh-orz @Jammjammjamm

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

Jammjammjamm

I think this is a reasonable approach.

Jammjammjamm · 2020-01-21T20:08:50Z

generator/uscore/uscore_generator.rb

+
+        revinclude_test[:test_code] += %(
+          end
+          skip 'No Provenance resources were returned from this search' unless any_provenances


It would be nice to use our skip_unless helper wherever possible.

Jammjammjamm · 2020-01-21T20:10:06Z

generator/uscore/uscore_generator.rb

+        revinclude_test[:test_code] = %(
+               any_provenances = false
+               could_not_resolve_all = []
+               resolved_one = false


These aren't always needed, for example in the AllergyIntolerance sequence.

Jammjammjamm · 2020-01-21T20:10:57Z

generator/uscore/uscore_generator.rb

@@ -250,10 +258,17 @@ def create_revinclude_test(sequence)
                assert_response_ok(reply)
                assert_bundle_response(reply)
                #{resource_variable} = fetch_all_bundled_resources(reply.resource).select { |resource| resource.resourceType == '#{resource_name}'}
-                skip 'No #{resource_name} resources were returned from this search' unless #{resource_variable}.present?
+                any_provenances ||= provenance_results.present?


Should we break out of the iteration once we've found some?

Jammjammjamm · 2020-01-21T20:14:38Z

generator/uscore/uscore_generator.rb

        search_test[:test_code] =
          if is_first_search
+            # rcs question: are comparators ever be in the first search?


They are not currently and I'd say they are unlikely to be in the future.

generator/uscore/uscore_generator.rb

Jammjammjamm · 2020-01-21T20:25:40Z

generator/uscore/uscore_generator.rb

-        revinclude_test[:test_code] = search_params
+        revinclude_test[:test_code] = %(
+               any_provenances = false
+               could_not_resolve_all = []


Could this be given a more clear name? Maybe something along the lines of required_search_params?

Jammjammjamm · 2020-01-21T20:30:56Z

generator/uscore/uscore_generator.rb

-          reply = get_resource_by_params(versioned_resource_class('#{sequence[:resource]}'), search_params)
-          assert_response_ok(reply)
-          assert_bundle_response(reply)
+          @#{sequence[:resource].underscore}_ary = {}


Since this is now a hash, I'd love it if it wasn't named *_ary. The US Core resources work out so that *s works as a plural for them all.

I was wondering if this would make more sense, and quickly scanned to see if we could just replace with 's'. But I didn't end up doing it because it is easy to drop an s in places by mistake in the generator. But I can readd.

Jammjammjamm · 2020-01-21T20:38:00Z

lib/modules/uscore_v3.1.0/test/fixtures/bmi.json

@@ -1,46 +0,0 @@
-{


These fixtures are not generated and need to be restored.

Jammjammjamm · 2020-01-21T20:40:29Z

generator/uscore/templates/sequence.rb.erb

      conformance_supports :<%=resource%><%="
      delayed_sequence" if delayed_sequence%>
 <%=search_validator%>
      details %(
        The #{title} Sequence tests `#{title.gsub(/\s+/, '')}` resources associated with the provided patient.
      )

+      def patient_ids


Should this go in SequenceBase? I know putting more stuff in SequnceBase is unfortunate, but I think it makes sense since this method seems necessary to support the new field in TestingInstance.

I don't think so -- to me it is a matter of scope. Should all tests everywhere need access to this method? I don't think so. Yeah it's unfortunate that all testing instances will have this field, but hopefully we can fix that at some point.

Jammjammjamm · 2020-01-21T20:41:49Z

generator/uscore/uscore_generator.rb

@@ -759,7 +816,7 @@ def get_search_param_hash(search_parameters, sequence, grab_first_value = false)
        search_parameters.each_with_object({}) do |param, params|
          params[param] =
            if param == 'patient'
-              '@instance.patient_id'
+              'patient'


What's this change?

Hah, we should just remove it. I didn't look too closely at the logic, was doing a replace any time we are doing @instance.patient_id because they are now in a loop block where patient is defined.

style fix for block Co-Authored-By: Stephen MacVicar <Jammjammjamm@users.noreply.github.com>

ghost · 2020-01-22T18:48:44Z

generator/uscore/uscore_generator.rb

-            reply = get_resource_by_params(versioned_resource_class('#{sequence[:resource]}'), search_params)
-            assert_response_ok(reply)
-            assert_bundle_response(reply)
+          could_not_resolve_all = []


We don't need the resolved_one logic in any of the first searches because we're not resolving the values from the resources. They're all either just patient or patient + some fixed value.

ghost · 2020-01-23T15:29:09Z

I think the only thing that needs to loop through patients are the first searches for patient-based resources. Once all the resources are in the array, we can use resolve_element_from_path to look through all the resources. Then pretty much nothing else has to change I think.

fix rubocop issues and remove unit tests

arscan · 2020-01-24T20:03:19Z

Note: temporarily removed the generated unit tests because major refactor needed. We have decided to go this route though so our plan is to merge to allow end-to-end tests with other downstream updates and backfill unit tests to catch edge cases over the coming weeks.

arscan added 4 commits January 20, 2020 15:14

Add URL into us core unit test instances.

31604da

Update method client is created in us core tests.

b25494c

Fix incorrect version setter.

3bfb77e

Initial multi-patient update to uscore.

5af598d

arscan added the WIP Work in progress label Jan 21, 2020

arscan changed the title ~~Fi 601 mutli patient uscore~~ Fi 601 Pass comma-separated patients to US Core Jan 21, 2020

Jammjammjamm reviewed Jan 21, 2020

View reviewed changes

Update generator/uscore/uscore_generator.rb

b9d171e

style fix for block Co-Authored-By: Stephen MacVicar <Jammjammjamm@users.noreply.github.com>

ghost reviewed Jan 22, 2020

View reviewed changes

arscan and others added 3 commits January 23, 2020 10:56

Readd uscore fixtures.

d24db47

fix rubocop issues and remove unit tests

8961b1b

Merge pull request #424 from onc-healthit/clean-up-multi-patient-changes

7de4448

fix rubocop issues and remove unit tests

arscan merged commit a2ffbde into development Jan 24, 2020

arscan deleted the FI-601-mutli-patient-uscore branch January 24, 2020 20:04

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 601 Pass comma-separated patients to US Core #419

Fi 601 Pass comma-separated patients to US Core #419

arscan commented Jan 21, 2020 •

edited

Jammjammjamm left a comment

Jammjammjamm Jan 21, 2020

Jammjammjamm Jan 21, 2020

Jammjammjamm Jan 21, 2020

Jammjammjamm Jan 21, 2020

Jammjammjamm Jan 21, 2020

Jammjammjamm Jan 21, 2020

arscan Jan 22, 2020

Jammjammjamm Jan 21, 2020

Jammjammjamm Jan 21, 2020

arscan Jan 23, 2020

Jammjammjamm Jan 21, 2020

arscan Jan 22, 2020

ghost Jan 22, 2020

ghost commented Jan 23, 2020

arscan commented Jan 24, 2020

Fi 601 Pass comma-separated patients to US Core #419

Fi 601 Pass comma-separated patients to US Core #419

Conversation

arscan commented Jan 21, 2020 • edited

Jammjammjamm 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

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

ghost commented Jan 23, 2020

arscan commented Jan 24, 2020

arscan commented Jan 21, 2020 •

edited