Automatic Creation of Single Valued, String Chado Fields #261

spficklin · 2022-08-13T06:24:47Z

Issues Addressed

Issue #129
Issue #236
Issue #237

Wait to review this until after the merge of PR #254 because this PR has that code in it and all of the changes in that PR are also showing up here.

Description

This PR provides new functionality for field creation. It will automatically add all single-valued string fields to all content types during the Chado Prepare step. The only exception is the 'Pub' content type. That one requires some special handling. You can also create new entities for content types where the string fields are sufficient to meet the unique constraint (e.g. Organism).

This PR does not implement the ChadoStorage::deleteValues() function (issue tripal/tripal#1350) because there is not yet a distinction between an "Unpublish" and a "Delete" so I thought it would be best to not implement a delete until we had the buttons. The ChadoStorage::findValues() function is also not yet implemented (issue tripal/tripal#1351).

This PR also does not support integer fields, text fields or other database fields other than character varying. I left a "@todo" comment in the code to finish those up. But this PR is provided now so that everyone can have access to the new code.

How to Test

Uninstall Tripal, pull this branch and reinstall Tripal (or start from a brand new installation)
Install Chado
Prepare Chado. It should indicate all of the content types are created. It's a bit slower creating content types now because it takes time to create all the fields.
Go to Admin > Structure > Tripal Content Types. Pick any content type. You will see the set of fields that have been added. They should not appear in the 'Disabled' section but should be in the Content section.
Go to Content > Tripal Content > Add new Tripal Content and pick the Organism content type. Test creation and editing of the content type.
Look in the Chado database in the organism table. A new record should exist in the organism table, and any changes you made should be there.
Return to the Admin > Structure > Tripal Content Types. You should be able to move the fields around on the manage display and manage form.
Check out the field settings and the field storage settings. You should see the controlled vocabulary term that each field is assigned and the Chado table/column that the field maps to.

Added id space and accession attributes for tripal entity CV terms.

Merged new tripal field handling code in entities to existing entities. The new tripal field interface will now be recognized by the system.

Fixed bug in annotation of tripal entities where the storage handler was not provided.

…he TripalEntity class

Added missing semicolons when creating new empty array.

…artifact.

…eld.

laceysanderson

The tests should be fixed now :-)
Code review-wise: this PR is looking good.
It works for single-value, single-property fields like those on the organism content type.

We will need to create more fields in order to ensure the ChadoStorage handles more complex fields like foreign key single- + multi-value fields. There is placeholders to test these two in the automated testing for when we get to implementing them :-)

laceysanderson · 2022-08-20T00:27:07Z

@risharde this one is also ready for you to review!

spficklin · 2022-08-21T23:44:19Z

I did a quick review of the changes @laceysanderson . All looks good. Thanks for makin these fixes.

spficklin · 2022-08-24T19:01:02Z

I had a brief moment for coding, so I added in support for text fields. These are unlimited strings (e.g. organism.description). So, now when you prepare Chado you should see the single-valued text fields appear as fields.

spficklin · 2022-08-25T16:34:05Z

In the process of working on a complex fields (obi__organism), I've discovered that we have a problem with this PR. When setting the storage settings for a field, those are persistent between instances of the field. So, if we use the same field for two different content types and link that field to a Chado table, it will always be linked to only one Chado table. For example the feature table and analysis tables of Chado both have a name column. A field of rdfs:label gets added to all content types for those tables. But, in the end, that field always only points to one table. I think the fix is to move the Chado associations into the field settings and out of the field storage settings. I don't think this should stop this PR from getting merged in, we just need to fix this. I'm working on a fix in the branch for the complex field.

If there aren't any objections I think we move forward with a functional review by @risharde and merge if all is good and I'll make a new PR afterwards with the complex field and the fix for the problem I just described.

risharde · 2022-08-29T16:59:33Z

Overall working great @spficklin! 1 warning (hopefully minor) in the tripal prepare step - screenshot attached.
@laceysanderson

[GOOD] Uninstall Tripal, pull this branch and reinstall Tripal (or start from a brand new installation)
[GOOD] Install Chado
[1 WARNING] Prepare Chado. It should indicate all of the content types are created. It's a bit slower creating content types now because it takes time to create all the fields.

@spficklin

[GOOD] Go to Admin > Structure > Tripal Content Types. Pick any content type. You will see the set of fields that have been added. They should not appear in the 'Disabled' section but should be in the Content section.
[GOOD] Go to Content > Tripal Content > Add new Tripal Content and pick the Organism content type. Test creation and editing of the content type.
[GOOD] Look in the Chado database in the organism table. A new record should exist in the organism table, and any changes you made should be there.
[GOOD] Return to the Admin > Structure > Tripal Content Types. You should be able to move the fields around on the manage display and manage form.
[GOOD] Check out the field settings and the field storage settings. You should see the controlled vocabulary term that each field is assigned and the Chado table/column that the field maps to.

risharde

Functional tests worked so we're good to go here!

codeclimate

The PR diff size of 10989 lines exceeds the maximum allowed for the inline comments feature.

spficklin · 2022-08-30T22:06:22Z

Approved by @laceysanderson and @risharde ... all tests are passing but the Drupal 10 check which is breaking on Docker. So, merging.

codeclimate · 2022-08-30T22:33:31Z

tripal/src/Plugin/Field/FieldType/TripalStringTypeItem.php

+  /**
+   * {@inheritdoc}
+   */
+  public function tripalValuesTemplate() {


Identical blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2022-08-30T22:33:31Z

tripal/src/Plugin/Field/FieldType/TripalTextTypeItem.php

+  /**
+   * {@inheritdoc}
+   */
+  public function tripalValuesTemplate() {


Identical blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2022-08-30T22:33:32Z

tripal_chado/src/Plugin/TripalStorage/ChadoStorage.php

-  public function updateValues($values) {
-    $indexed_values = $this->indexValues($values);
-
+  public function updateValues($values) : bool {


Function updateValues has a Cognitive Complexity of 23 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2022-08-30T22:33:32Z

tripal/src/Services/TripalFieldsManager.php

+   * @return bool
+   *   True if the field was added successfully. False otherwise.
+   */
+  public function addBundleField(string $bundle, array $field_def) : bool {


Function addBundleField has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2022-08-30T22:33:32Z

tripal/src/TripalField/TripalWidgetBase.php

+  /**
+   * {@inheritdoc}
+   */
+  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {


Method formElement has 5 arguments (exceeds 4 allowed). Consider refactoring.

codeclimate · 2022-08-30T22:33:34Z

Code Climate has analyzed commit 82893db and detected 23 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	19
Duplication	4

The test coverage on the diff in this pull request is 8.9% (50% is the threshold).

This pull request will bring the total coverage in the repository to 22.2% (-2.1% change).

View more on Code Climate.

4ctrl-alt-del and others added 25 commits July 18, 2022 08:10

Added CV terms.

144ca72

Added id space and accession attributes for tripal entity CV terms.

Merged tripal field solution.

88794b9

Merged new tripal field handling code in entities to existing entities. The new tripal field interface will now be recognized by the system.

Merge branch '9.x-4.x' into tv4g1-129-TripalFields

59eb5d7

Fixed tripal entities.

0ec5ead

Fixed bug in annotation of tripal entities where the storage handler was not provided.

Fixed missing semicolons in TripalEntity

1eee3f7

Fixed missing arguments in bundleFieldDefinitions function calls in t…

df796fe

…he TripalEntity class

Fixed tripal entity schema handler.

e3cc2a8

Added missing semicolons when creating new empty array.

Typo fixes and one change to TripalTypes

bc5fd2b

Typos fixed

2226a40

Merge branch 'tv4g4-212-mviews' into tv4g1-129-TripalFields-spf

99fc4b6

First field working partially

4e15623

field display settings working

403e822

Field formatter working

e9cafcc

Working on saving fields to Chado

69aacff

more work on saving fieldsto Chado

7db9d9e

Fields now saving to Chado

7f4157e

Insert, Load and Update all working with Chado single-value fields

08e9e18

Merge branch '9.x-4.x' into tv4g1-129-TripalFields-spf

c74bd11

Fixed merge conflict"

38805d0

Additional merge conflict fixes

09d762f

Additional merge conflict fixes

2192200

Removed RDFS types

72e0309

Merge branch '9.x-4.x' into tv4g1-129-TripalFields-spf

7d1f06e

Added default view and form displays for content types

082eba3

Automatic creation of single-value string fields

3884185

spficklin requested review from laceysanderson and risharde August 13, 2022 06:24

spficklin added Group 1 - Entities|Bundles|Fields Group 2 - Chado/Storage Group 7 - Display labels Aug 13, 2022

laceysanderson added 12 commits August 15, 2022 22:06

Add back in missing sequence ontology terms.

2f440f3

Literally move the test method to the top of the class.

6cb9fe2

Remove mention of T3 tripal_bundle + chado_ bundle plus chado_fields …

6256628

…artifact.

Remove ChadoStoragePropertyTypeTrait as it's not needed.

88c35bd

Add a note to go back to.

b42edc3

Ensure cardinality + required are set in the field settings.

7f87cf2

Match addTypes input to interface + add more error checking.

82e3fe7

First pass towards fixing these tests.

b5842ea

Update docblocks w/ example + fix chado query bug.

b4c3507

Get ChadoStorage tests working for a single-value, single-property fi…

a462849

…eld.

Missed a debugging statement.

7d5357e

Remove deprecated drupal_get_path('module', *) calls.

37a81a8

laceysanderson previously approved these changes Aug 20, 2022

View reviewed changes

Merge branch '9.x-4.x' into tv4g1-129-TripalFields-spf

67f5a80

Added Text field type

d17333c

spficklin dismissed laceysanderson’s stale review via d17333c August 24, 2022 18:59

Fixed merge conflicts

95f71ec

risharde approved these changes Aug 30, 2022

View reviewed changes

codeclimate bot reviewed Aug 30, 2022

View reviewed changes

Merge branch '9.x-4.x' into tv4g1-129-TripalFields-spf

82893db

codeclimate bot reviewed Aug 30, 2022

View reviewed changes

spficklin merged commit f7d2cf0 into 9.x-4.x Aug 30, 2022

spficklin deleted the tv4g1-129-TripalFields-spf branch December 3, 2022 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatic Creation of Single Valued, String Chado Fields #261

Automatic Creation of Single Valued, String Chado Fields #261

spficklin commented Aug 13, 2022 •

edited

Loading

laceysanderson left a comment

laceysanderson commented Aug 20, 2022

spficklin commented Aug 21, 2022

spficklin commented Aug 24, 2022 •

edited

Loading

spficklin commented Aug 25, 2022

risharde commented Aug 29, 2022

risharde left a comment

codeclimate bot left a comment

spficklin commented Aug 30, 2022

codeclimate bot Aug 30, 2022

codeclimate bot Aug 30, 2022

codeclimate bot Aug 30, 2022

codeclimate bot Aug 30, 2022

codeclimate bot Aug 30, 2022

codeclimate bot commented Aug 30, 2022

Automatic Creation of Single Valued, String Chado Fields #261

Automatic Creation of Single Valued, String Chado Fields #261

Conversation

spficklin commented Aug 13, 2022 • edited Loading

Issues Addressed

Description

How to Test

laceysanderson left a comment

Choose a reason for hiding this comment

laceysanderson commented Aug 20, 2022

spficklin commented Aug 21, 2022

spficklin commented Aug 24, 2022 • edited Loading

spficklin commented Aug 25, 2022

risharde commented Aug 29, 2022

risharde left a comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

spficklin commented Aug 30, 2022

codeclimate bot Aug 30, 2022

Choose a reason for hiding this comment

codeclimate bot Aug 30, 2022

Choose a reason for hiding this comment

codeclimate bot Aug 30, 2022

Choose a reason for hiding this comment

codeclimate bot Aug 30, 2022

Choose a reason for hiding this comment

codeclimate bot Aug 30, 2022

Choose a reason for hiding this comment

codeclimate bot commented Aug 30, 2022

spficklin commented Aug 13, 2022 •

edited

Loading

spficklin commented Aug 24, 2022 •

edited

Loading