Skip to content

Add import_metadata method#145

Merged
pwildenhain merged 7 commits into
redcap-tools:masterfrom
JuliaSprenger:import_metadata
Mar 25, 2021
Merged

Add import_metadata method#145
pwildenhain merged 7 commits into
redcap-tools:masterfrom
JuliaSprenger:import_metadata

Conversation

@JuliaSprenger
Copy link
Copy Markdown
Contributor

Adding a method to import metadata into an existing RedCap project.
This allows to use PyCap to upload a DataDictionary in csv / json format.

The new code is mostly based on the import_records method.

I would be happy to add more tests, but would need some advise on the test setup used.

@pwildenhain
Copy link
Copy Markdown
Collaborator

Thanks for your contribution. I'll take a look tomorrow 👍

Copy link
Copy Markdown
Collaborator

@pwildenhain pwildenhain left a comment

Choose a reason for hiding this comment

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

Thanks for jumping in the project! 🙇🏻‍♂️

See the comments below 👁️ for potential changes.

Lastly, if you run pip install -r dev-requirements.txt and then run pytest it should point out the exact styling/linting issues you're running into here. If you can address those as well, then this will be good to merge 💪🏻

Comment thread test/test_project.py
self.assertIn("error", exc.args[0])

@responses.activate
def test_import_metadata(self):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm happy with this test 👍🏻

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have been experimenting with additional tests and found some weird behavior, of which I suspect is is due to the test setup as it works with other redcap servers. Do you have an idea why the following test might fail?

    @responses.activate
    def test_import_reduced_metadata(self):
        "Test import of a reduced set of metadata"
        self.add_normalproject_response()
        data = self.reg_proj.export_metadata()
        # reducing the metadata
        data = data[0:1]
        d = self.reg_proj.import_metadata(data)
        # this should return the reduced set of metadata, but instead returns the full set of metadata when running on the test server
        self.assertEqual(len(d), len(data))

Comment thread redcap/project.py
Comment on lines +653 to +664
if hasattr(to_import, "to_csv"):
# We'll assume it's a df
buf = StringIO()
to_import.to_csv(buf, index=False)
payload["data"] = buf.getvalue()
buf.close()
format = "csv"
elif format == "json":
payload["data"] = json.dumps(to_import, separators=(",", ":"))
else:
# don't do anything to csv/xml
payload["data"] = to_import
Copy link
Copy Markdown
Collaborator

@pwildenhain pwildenhain Mar 23, 2021

Choose a reason for hiding this comment

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

Thanks for being consistent with how the Project.import_records imports data.

Would you be open to adding a private helper method Project._add_import_data (or something to that effect) that basically accomplishes the highlighted section above? Since the code is exactly the same, it probably makes sense to abstract it at this point.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did add a utility method do initialize the payload for record and metadata. However since the code was not 100% identical i had to add the data type as another input to that helper function. What do you think about this?

@JuliaSprenger
Copy link
Copy Markdown
Contributor Author

Thanks for pointing out the next steps to resolve the test issues. One of them seems to be the number of lines in the project.py module. Would you like to split this module to adhere to the 1000 lines max rule?

@pwildenhain
Copy link
Copy Markdown
Collaborator

Would you like to split this module to adhere to the 1000 lines max rule?

If you don't mind, that would be great!

As for the test failure....this is such great work, I really wouldn't want to hold it up.

Do you know if there is a way that we can mark this test as expected to fail? Even just commenting it out for now would be fine. As long as we file an issue to address it later.

@JuliaSprenger
Copy link
Copy Markdown
Contributor Author

Would you like to split this module to adhere to the 1000 lines max rule?

If you don't mind, that would be great!

To be honest I am not sure how you would do this in a good manner as it would require splitting the Project class. Instead I now configured pylint to ignore the excess lines in project.py. I hope that is an acceptable temporary fix for you until there's a better reason to restructure the classes an another time.

@JuliaSprenger
Copy link
Copy Markdown
Contributor Author

As for the test failure....this is such great work, I really wouldn't want to hold it up.

Do you know if there is a way that we can mark this test as expected to fail? Even just commenting it out for now would be fine. As long as we file an issue to address it later.

I added the test and configured unittest to skip it as I don't think it should be marked as expected to fail.

@pwildenhain
Copy link
Copy Markdown
Collaborator

I hope that is an acceptable temporary fix for you until there's a better reason to restructure the classes an another time.

Totally acceptable 👍

I don't think it should be marked as expected to fail

🤦‍♂️ duh haha, thanks for bringing that up.

Thanks so much for this contribution. Do you also need a release to PyPi?

@pwildenhain pwildenhain self-requested a review March 25, 2021 12:55
@pwildenhain pwildenhain merged commit 8137d2c into redcap-tools:master Mar 25, 2021
@JuliaSprenger
Copy link
Copy Markdown
Contributor Author

Thanks for the quick merge.
A new release is not essential for us, but would make the version tracking much more transparent. I think it depends how much effort a new release is on your side.

@JuliaSprenger JuliaSprenger deleted the import_metadata branch March 26, 2021 08:34
@pwildenhain
Copy link
Copy Markdown
Collaborator

pwildenhain commented Mar 26, 2021 via email

@pwildenhain
Copy link
Copy Markdown
Collaborator

🚀 Check out the new release pip install PyCap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants