search

statonlab / tripal_hq

provides a user and administrative dashboard for Chado content creation
GNU General Public License v3.0
2 stars 0 forks source link

Contribute Tripal HQ Imports #116

Closed laceysanderson closed 4 years ago

laceysanderson commented 4 years ago

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 commented 4 years ago

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

almasaeed2010 commented 4 years ago

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 commented 4 years ago

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 notifications@github.com 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Tests

Screen Shot 2019-11-06 at 8 51 31 AM

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 commented 4 years ago

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.

Screen Shot 2019-11-06 at 8 57 01 AM
laceysanderson commented 4 years ago

Thanks for the review @almasaeed2010! I fixed

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.