Add export & import for repeating instruments and events

[JuliaSprenger]

With the new test framework I am not sure how / where to integrate a dedicated test for these functions. Any advise on this?

[JuliaSprenger]

I don't think the failing checks are related to my changes, but rather to some server token permissions not granted for tests originating from a fork. @pwildenhain Any way I can fix this or is this an issue of the GH Actions configuration?

[pwildenhain]

You are correct, it's the fact that your account can't access repo secrets

it's happening with the doctests, but what's confusing to me is that I thought I had configured these to skip on forks though, I must've messed up the logic

If you check out .github/workflows/ci.yml specifically at line 40 you'll see the skip logic I'm talking about

Looks like I'll have to tweak that to move this forward.

Does everything work when you run pytest locally?

[JuliaSprenger]

Locally using pytest I get some pylint issues, not sure if these will also pop up in the CI. I addition the test_empty_json_is_still_a_problem_for_other_methods test also fails, but I am not sure how that relates to my changes...

Regarding the conditional run of the doctests, maybe you have to use a different syntax (${{ github.repository == 'redcap-tools/PyCap' }}) like shown here.

[pwildenhain]

Yea looks like we might need to use github.ref or github.actor

As a test, can you change that line to only run if github.actor == 'pwildenhain' ?

That's a little hard coded for my usual taste but at least we can get this PR to pass. Down the road I can change this to only run if the ref starts with redcap-tools or something like that (aka don't run if PR originated from a fork)

[JuliaSprenger]

It seems the token permission issue also occurs with the main tests. I had the same issue here and solved it very indirectly via the pull_request_target in combination with tagging the the PR to be safe for testing (i.e. no leaking of secrets). If you know a better way to use secrets in PRs from other forks this would be very interesting.

[pwildenhain]

Hmmmm it's appropriately skipping the integration tests, but it looks like it's still trying to build the doctest fixture and that's where it's freaking out.

Really appreciate all your contributions, I'll hopefully be able to provide a lot of fixes once I'm in the office today/and probably bleeding into tomorrow as well

[pwildenhain]

See JuliaSprenger#1 for some proposed changes to this PR that I think will allow the CI to run 🤞🏻

[JuliaSprenger]

I thought I fixed the typing-extensions dependencies already via #212, not sure why it is still missing in the CI environment...

[codecov]

Codecov Report

Merging #210 (35e31a4) into master (8a7d169) will decrease coverage by 1.10%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##            master     #210      +/-   ##
===========================================
- Coverage   100.00%   98.89%   -1.11%     
===========================================
  Files           16       17       +1     
  Lines          531      541      +10     
===========================================
+ Hits           531      535       +4     
- Misses           0        6       +6     

Impacted Files	Coverage Δ
redcap/methods/metadata.py	100.00% <ø> (ø)
redcap/methods/records.py	100.00% <ø> (ø)
redcap/project.py	100.00% <ø> (ø)
redcap/methods/__init__.py	100.00% <100.00%> (ø)
redcap/methods/base.py	100.00% <100.00%> (ø)
redcap/methods/repeating.py	100.00% <100.00%> (ø)
redcap/conftest.py	50.00% <0.00%> (-50.00%)	⬇️

[pwildenhain]

I thought I fixed the typing-extensions dependencies already via #212, not sure why it is still missing in the CI environment...

I ended up getting rid of typing-extensions all together when dropping support for python 3.7 since the types I was using it for entered the stdlib in python 3.8

🎉 So awesome that the CI passes now 🙌🏻

Would you like to try your hand at adding some unit tests? If so I can walk you through the architecture -- if not I can add them myself.

[JuliaSprenger]

The tests fail with a KeyError: 'repeatingFormsEvents'. Maybe there's more changes to the long_project.xml needed to be able to export repeating forms? @pwildenhain Any ideas?

[pwildenhain]

Yup the tests are a bit of a maze! I'll do a write up here (maybe some version of this should live somewhere?), let me know if anything is unclear or there is somewhere you'd prefer that I step in to contribute. Something I can definitely do in the meantime is take the modifications you've made to the tests here and add them as integration tests rather than unit tests. I'll submit a PR for that shortly (edit: JuliaSprenger#2). Hold off on pushing anything until you're able to add that into this PR

Testing Architecture

The tests are broken up into unit tests tests/unit and integration tests tests/integration.

The integration tests connect to a real redcap server over at redcapdemo.vanderbilt.edu. I have a super user token which can create new projects to be used to testing. The xml files used in creating these test projects on the real server are under tests/data.

These are really nice to ensure that the package works on the real thing. But I can't share this super user token with others, so I need rely on the unit tests for others to use to test package functionality.

Unit tests

The unit tests use the responses package to mock out a fake REDCap server that spins up locally when the tests are running. These fake projects are represented by fixtures -- in the case of tests/unit/test_long_project.py it's the long_project fixture near the top of the module.

Requests are then routed to this mock server, which chooses the response data based off a handler function. With the long_project it's handler is get_long_project_request_handler located in the test/unit/callback_utils.py module.

You'll need to add a modify the long_project handler function by adding a function that accepts repeatingFormsEvents content, which can accept both an export and import request. Something like handle_long_project_repeating_form_request. Side note: this is why you're getting the KeyError. Because the repeatingFormsEvents key is being passed to the dict inget_long_project_request_handler and it's not finding anything for it. This function will mock how a real REDCap server would handle the request (though very simplified) and return the appropriate data in the requested format.

For example the handle_long_project_form_event_mapping_request function mocks the event(s) that would be returned from the formEventMapping content request. Other functions like handle_long_project_records_request take into account request format, parameters, importing, exporting, etc. to ensure all the functionality of PyCap record importing/exporting for long projects are tested. For the purposes of this PR, you don't need to worry about multiple formats

For the purposes of these endpoints, just testing the json format is plenty since the functionality is simple and similar to other endpoints.

[JuliaSprenger]

Hi @pwildenhain I copied some of the repeatingFormsEvents tests to be unit tests and added the corresponding lines for the export in the callback_utils. However, I am not sure how these utils are distinguishing between export and import. For this reason the test_import_export_repeating_forms still fails. Any hint on how to add a valid response also for the import?

[pwildenhain]

You're so close here, that I am just gonna close the gap (see JuliaSprenger#3) and I'll add a more in-depth explanation below

However, I am not sure how these utils are distinguishing between export and import.

This points to a problem with the REDCap API -- everything is a POST request. Normally exports would fall under GET while imports would fall under POST and it would be a lot easier to distinguish.

How REDCap distinguishes instead is the presence of the data parameter in the payload body, since that is only a valid parameter for imports. So we mimic that in our callback. Example linked below:

PyCap/tests/unit/callback_utils.py

Lines 135 to 153 in 8a7d169

	def handle_simple_project_metadata_request(**kwargs) -> MockResponse:
	"""Handle metadata import/export request"""
	data = kwargs["data"]
	headers = kwargs["headers"]
	# import_metadata
	if "data" in data:
	if "csv" in data["format"]:
	# count newlines to infer number of records
	newline_count = str(data["data"][0].count("\n") - 1)
	data_len = json.loads(str.encode(newline_count))
	else:
	data_len = len(json.loads(data["data"][0]))
	resp = json.dumps(data_len)
	return (201, headers, resp)
	# exporting metadata
	if "csv" in data["format"]:
	resp = (
	"field_name,field_label,form_name,arm_num,name\n"

Basically we just check the for the "data" key in the data portion of the request body (a little confusing 😵 ). And we check for that first so that way if we don't find it, we know we're dealing with an export.

It's kinda neat, gives a little bit of insight into how a real REDCap server needs to handle these types of requests -- but definitely not intuitive 😂

[JuliaSprenger]

I am not sure in which ways the not covered lines are related to the changes. @pwildenhain Any suggestions what to readd to cover those?

[pwildenhain]

I think I understand what's happening. Since the doctests don't get run with the regular unit tests, the redcap/conftest.py is counted as untested.

I think all I need to do it add --omit=redcap/conftest.py to the addopts in pytest.ini

But that's not your problem, so I'll merge this and take care of that separately.

Beautiful work here ✨ really appreciate you contributing, especially with all the architecture changing since the last time you contributed. Hope to see you back again 😉

[JuliaSprenger]

Thanks for merging and all of the advise you gave in this PR. I learned quite a bit :)