Contribute Tripal HQ Imports

[laceysanderson]

Issue #114

This PR submits a sub-module tripal_hq_imports which provides Tripal HQ support for TripalImporters. It uses the exact same strategy as core Tripal HQ to provide the ability for users to submit any single-page TripalImporter. The module then saves the $form_state in the Drupal table until approved by an administrator at which point it submits the TripalImporter::run() job. It uses the form, form_validate and form_submit from Tripal Core to ensure the same processing is done as would be if you submitted through the admin interface. It also uses the same permissions-based approach as Tripal HQ to allow configuring of which Importers users can propose.

A couple things to note: tripal_hq_imports does not yet support Chado-based by-organism permissions like Tripal HQ does not does it support email. The module has 98% test coverage and has been extensively tested by my team.

Documentation

I updated ReadtheDocs to include documentation for Tripal HQ Imports. Rather then creating a specific section, I incorporated it into the existing documentation for module continuity. The Readme for Tripal HQ Imports is still available within the PR to ensure a more targeted Readme is available.

Collaboration

I would love suggestions for improvements! As for continued development, we are using the full Tripal HQ package on KnowPulse and are committed to contributing back :-) I will also try to answer questions regarding Tripal HQ imports as they come up on the repo. The amount of future development will depend on the needs of KnowPulse though.

Testing

Automated testing

This PR contains automated testing which has been completed integrated with the existing testing. As such, simply pull this branch, enable tripal_hq_imports, run composer up (if needed) and execute all tests.

Manual testing

Before enabling Tripal HQ imports, check that all functionality is exactly the same as before 👍
After enabling Tripal HQ Imports,

1. Go to permissions and check the box beside each importer you would like users to be able to propose. You can use the same role as you do for core Tripal HQ permissions.
2. You will see a "Import Data File" action link added to the user dashboard. Use this to add TripalImporter submissions.
3. The administration dashboard will now have two tables on each page with the first being the original Tripal HQ Content Submission tables and the second will list TripalImporter submissions.
4. Approve a submission which will submit the tripal job. Run the tripal job to import the data proposed. Reject a submission to confirm the job is not submitted. Edit a submission to confirm administrators and users can update metadata or upload new files.

[laceysanderson]

I'll work on the code climate changes this morning 🤷‍♀

[almasaeed2010]

I don't know about Code Climate. I honestly don't like it. It's very demanding and I believe it's a waste of time. The goal is make the code legible and I think we generally do a good job at that and don't necessarily need a bot to tell us we missed 230 spaces ....

If you agree, I am willing to turn off the integration.

(I'll take a look at the code and perform tests next week when I have a chance)

Thanks!

[bradfordcondon]

I agree, I was experimenting with it and really liked certain things (catching dpm) but in terms of styling it’s a waste of time.

…

Sent from my iPhone

On Nov 1, 2019, at 12:50 PM, Abdullah Almsaeed ***@***.***> wrote: I don't know about Code Climate. I honestly don't like it. It's very demanding and I believe it's a waste of time. The goal is make the code legible and I think we generally do a good job at that and don't necessarily need a bot to tell us we missed 230 spaces .... If you agree, I am willing to turn off the integration. (I'll take a look at the code and perform tests next week when I have a chance) Thanks! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[laceysanderson]

I'm tempted to look into additional configuration for it rather then remove it altogether. In this case it caught that I wasn't using type hinting, some laziness in my documentation and some helpful hints regarding translating and links.

I'll see what I can come up with and report back.

[bradfordcondon]

Good luck! It is really nice to have some standards feedback.

I've switched to PHPMD which is easily configurable, i set it to only show missing variables and type hints for example but to ignore method names which i have no control over. I did some brief googling, maybe we are using DrupalPractice which probably has excessive best practice suggestions? But I dont know that the drupal standards are going to be as configurable without, say, defining our own.

[laceysanderson]

So we can't get as fine grained control as I would like 🤷‍♀ However, I found a solution I was happy with:

I agree that the large number of style issues was very frustrating to see! To address this, I've added some helpful text to the contributing documentation about how to use phpcodesniffer locally with this project. The ability to automatically fix most of the issues helped me immensely! After that, all the remaining issues were easy to fix and I feel better about my code having done so. I've included phpcodesniffer in the composer.json to make it easier to run it locally which I felt was important.

If you still feel it is too much, you can hide warnings which would greatly lessen the reporting on PRs. However, I found doing this missed some issues I felt would be good to fix 🤷‍♀

[almasaeed2010]

Tests

• Tested using the fasta importer. File uploads were successful and the importer ran as expected once accepted by the administrator.
• Permissions are also set properly.
• For some reason, the view/edit comments link shows some un-rendered HTML as shown below (notice the <em class=...)

Security Concerns

A user is able to enter any value into the local file field (labeled Server path). I was able to give it a malicious value such as /etc/passwd. If importers delete or edit such a file mistakenly, it could harm the system. We should either remove this field or validate it to allow for specific paths that are predetermined by the site administrator.

Everything else worked wonderfully! Thanks so much @laceysanderson for all the work ❤️

[almasaeed2010]

One more note: the Chado Bulk Publication Importer is showing a blank page for me and submitting is always accepted. Is this because I have not defined a bulk importer? If so, I think we can simply warn in the documentation that this importer should not be made available unless properly configured.

[laceysanderson]

Thanks for the review @almasaeed2010! I fixed

• un-rendered html for comment count (happened when fixing for code climate)
• disabled local file upload
• removed bulk publication and obo loaders which do not work well with Tripal HQ Imports.

Note: Regarding supported importers, I have tested it with custom importers and it's worked with all of them except one which was a multi-page form. It now works with all core importers which appear in the list so admin would only need to verify custom importers.

[almasaeed2010]

Confirmed everything works as expected. Thanks @laceysanderson!