tripal / t4d8

This is a temporary repository for Drupal 9 upgrade for Tripal v4. It is meant to house issues related to the upgrade.
GNU General Public License v2.0
1 stars 10 forks source link

Chado Vocabs and ID Spaces Working #220

Closed spficklin closed 2 years ago

spficklin commented 2 years ago

This PR is for @4ctrl-alt-del to merge into his tv4g1-129-TripalFields branch.

This PR contains working ChadoIdSpace and ChadoVocabulary classes along with the functional tests. All of the functions except for those dealing with terms are working and have tests.

@4ctrl-alt-del I did have to make a few adjustments to the base classes and interfaces. Mostly they are just minor tweaks, but I did have to add some checks to keep them from breaking. Check them out ot make sure I didn't break anything.

I got rid of the save() function and these classes should always return the most up-to-date data from the database. The functional tests make sure this works.

You can test them with this command:

docker run --publish=9090:80 --name=t4d8 -tid --volume=`pwd`:/var/www/drupal9/web/modules/contrib/tripal tripalproject/tripaldocker:drupal9.3.x-dev

docker exec -it t4d8 "service postgresql start"
docker exec -it t4d8 "service apache2 start"

docker exec --workdir=/var/www/drupal9/web/modules/contrib/tripal t4d8 phpunit /var/www/drupal9/web/modules/contrib/tripal/tripal_chado/tests/src/Functional/Plugin/ChadoVocabTermsTest.php
4ctrl-alt-del commented 2 years ago

I noticed you added a plugid id argument to set default vocabulary, which is incorrect. The vocabulary must exist to add it, therefore its plugin id is already set and not needed.

Then I noticed in a previous PR you added a plugin id argument to the load collection method of collection managers, which is also incorrect. I apologize for missing that on the previous pull request.

I made a commit fixing your mistake of adding a plugin id argument to the load collection method. Please merge this with this PR and remove the plugin id argument from set default vocabulary.

spficklin commented 2 years ago

Okay thanks for the correction on the plugin IDs @4ctrl-alt-del . The SQL queries weren't quite working and I didn't make the connection about the pluginID. I've merged in your fixes but I still left some of my fixes to the queries so take a look and make sure all looks okay. But the plugin ID is no longer passed in as you fixed it.

Also, I just pushed a bunch more code today to this PR for the TripalTerm class. That class is now fully functional with tests.

I had to make quite a few changes to the TripalTerm class. One reason was posted on the #core-dev channel of Slack which @laceysanderson responded to. The other changes were that there needed to be support for synonyms, alternate IDs, properties, obsolescence and relationship types. Because there was so many new things I changed the constructor as well. Take a look and make sure I didn't break anything in terms of design. In terms of functionality it all seems to work.

There are two ways to create terms. Using the setters:

$parent = new TripalTerm();
$parent->setName('biological_process');
$parent->setIdSpace('GO');
$parent->setVocabulary('biological_process');
$parent->setAccession('0008150');
$parent->setDefinition($parent_definition);
$parent->addAltId('GO', '0000004');
$parent->addAltId('GO', '0007582');
$parent->addAltId('GO', '0044699');
$parent->addSynonym('biological process');
$parent->addSynonym('physiological process');
$parent->addSynonym('single organism process');
$parent->addSynonym('single-organism process');
$parent->addProperty($comment, $parent_comment);

Or via the constructor:

$parent = new TripalTerm([
  'name' => 'biological_process',
  'idSpace' => 'GO',
  'vocabulary' => 'biological_process',
  'accession' => '0008150',
  'definition' => $parent_definition,
  'altIDs' => [
    ['GO', '0000004'], 
    ['GO', '0007582'],
    ['GO', '0044699]'
  ],
  'synonyms' => [
    'biological process',
    'physiological process',
    'single organism process',
    'single-organism process',       
  ],
  'properties' => [
    [$comment, $parent_comment],
  ],
]);

Also, I changed anywhere in the TripalTerm class where it said defaultVocabulary to just vocabulary. But we might want to put that back. It just seemed confusing to me that a term has a default vocabulary. It belongs to one vocabulary only, but can be adopted by others. @laceysanderson any thoughts on that?

The one remaining thing I haven't yet finished up are the term functions (e.g. saveTerm) in the ChadoIdSpace class. That should be the last bit left to do! But if you find all of these changes okay, there's no reason not to merge them.

spficklin commented 2 years ago

Correction. the TripalTerms::suggestTerms() function is not yet implemented either.