-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for Chado to Term Mapping #254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone through a done a code review and all looks good now :-)
These are the changes I made:
- The ID for config entity yml do not have the config entity prefix in them
- There was still some "Example" placeholder comments so I updated them
- The config entity listings should be a in ListBuilders directory as was done for the other tripal entites
- hook_rebuild hadn't been updated to some previous changes + thus wasn't realizing views/config entities already existed
- There were WSOD errors when you tried to save the config entities as it was using old example code. I just commented out save functionality and added a message that there was nothing to save.
Now it just needs a functional review from @risharde. Thanks @spficklin!
Hi @risharde, this is still waiting on a functional review from you :-) |
I was only able to reach reinstalling drupal and tripal. However when performing a chado install, I encountered a WSOD which breaks all of the admin. I'm tagging @spficklin here - I'm not exactly sure if I can get logs to determine why this is breaking but I'll try my best but as such, I can't continue at the moment. |
This is what I was able to extract from watchdog.
|
@laceysanderson nevermind this one, @spficklin helped me out here, I forgot to run a drush cr which resolved this but I will come back to checking why this happened in the first place later on after completing the functional test. |
Another issue on my environment has arose so working on figuring this one out: |
Worked as intended on my new setup, great going @spficklin ! New environment setup was Drupal 9.4.5 + PHP 7.4 [GOOD] You must start with a new installation of Tripal and Chado. |
@laceysanderson this seems good to go for your review |
Since @risharde approved and @laceysanderson this is ready to merge. But there was a merge conflict which I resolved. Once all the tests have a chance to run again I'll mege. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional tests worked as intended!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR diff size of 7092 lines exceeds the maximum allowed for the inline comments feature.
Code Climate has analyzed commit 9ebdc52 and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 0.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 24.3% (0.9% change). View more on Code Climate. |
All tests pass but the Drupal 10 test which seems to be failing at Docker construction and was failing before. So I think it's safe to merge. |
This PR adds the mapping of terms to chado table columns that occurs during the Chado prepare step. In Tripal v3 these mappings were referred to as "semweb" but that is a bit of a misnaming as these mappings are needed for more than just semantic web. They are needed for fields.
Issue
Issue #212
This PR should not be merged until after PR #244 and PR #251 as it includes both of those already.
Changes:
addOntology<idSpace>()
. I realized this would be easier to maintain if we used a Configuration Entity and stored all of the terms we wanted to add in a YAML file. So, this PR removes those functions from theChadoPrepare
task and moves the definitions in a configuration YAML file. See the example below.tripal_content_terms
andchado_term_mapping
. The first is for managing the terms that we need Tripal to know about for creating entities. The second is for managing the mapping of terms that Tripal knows about to the columns of Chado. This is needed for creating fields.chado_semweb
table because Drupal manages the mapping for us with the configuration entity. So that table is not present in the install file.Screenshots
Example
config/install/tripal.tripal_content_terms.tripal_chado.yml
file.This file shows how the default vocabularies and terms needed by Tripal are stored in YAML files. These are dynamic content entities so anyone can add new ones by creating a file named
config/install/tripal.tripal_content_terms.<id>.yml
in their own custom modules (replacing<id>
with their unique identifier).Example
config/install/tripal_chado.chado_term_mapping.core.yml
fileThis is the YAML file mapping chado table columns to ontology terms.
Example of Term Mapping Edit Page
Currently this page only provides a viewing of the mapping of each table's columns. But in the future we can add functionality for editing and adding new ones.
What's Missing
How to Test
cv
,db
andcvterm
tables of both chado instances you should see that cvterms have been added to both.drush cache-rebuild
. Return to the listing and the configuration entities should return. This means that someone could potentially make changes (via a future interface) and decide to reset to default by deleting the configuration and rebuilding the cache.That's it! This PR has a lot of code changes but functionally there's not a lot to test.