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

Field Actions Redesign #321

Closed spficklin closed 1 year ago

spficklin commented 1 year ago

Overview

This PR addresses Issue #310. Despite that our three complex fields were working the programming of them was a bit confusing. In a design discussion we came up with a way to make things more clear. That design is described in issue #310. However, this PR goes a bit further and makes fields even more clear. Here's what I did.

Property Actions

We had originally decided to rename our property actions for Chado as: store, store_id, store_link_id, store_fixed, join, replace, and function. The store_link_id, and store_fixed were new.

However, I made a few adjustements:

Field Property Settings

Previously, property settings were provided in three different places! This made it difficult to know where a developer should set properties? Previously, some properties would get set during field initialization (such as in the Chado Prepare step), others got set in the field class itself and others were set in parent classes. Now, property setting are only set in the property class itself. This way it's easy for a site developer to know exactly what's beeing set for each property.

Record ID property

Previously, the TripalFieldItemBase class provided to all fields a default property for the record_id. I removed this. In hindsight, I didn't think we wanted to force all fields, including those not for Chado, to have a built-in record_id property. I did not move this to the ChadoFieldItemBase because I decided to let field developers implement it. That way they cand do it how they want. It returns flexibility to the field developer.

Also, in the past every field had to have a property with a key named record_id. This is no longer the case because it didn't make sense to require the action of store_id and a key of record_id. We only need to require store_id so users are free to give the property a key of whatever they like.

Value property

Previously, every field was required to have a property with a key named value. This make naming properties a bit confusing because all of the properites are given names that correspond to what they store except this property. For example in the obi__organism field, it must store the organism_id and that property was given the key of value. But that's confusing becuase it's the property named label that is shown to the user. I discovered you can tell Drupal what property key can be used for the primary value by returing tke key in the function mainPropertyName() of the field class. So, if a complex field doesn't have a value key, that's okay as long as the user tells Drupal what it should be via the mainPropertyName() function. I think this makes creating properties more intuitive as you don't have to cram a key of value onto a property that doen't make sense for it.

tripalValuesTemplate()

The tripalValuesTemplate() function is no longer needed in a field class. It's still required by Tripal but this PR automates creation of the return value. Now, in the TripalFieldItemBase the function calls the tripalTypes() function and generates a values array that matches. This means that field developers no longer need to write it. This is great because it was too easy to accidientally forget to add a value for every property, or if you changed a key of a property in the tripalTypes function it was too easy to forget to also do it in the tripalValuesTemplate() function.

Field Properties with CV as keys

Previously, in our complex fields, the keys of properties were CV terms (e.g. schema__label). We did a similar thing in Tripal v3 because it helped web services. But, I thought this made programming fields harder for folks, and there's no reason for this. We can easily add a required term for all properties when they are instantiated. The property key does not need to be that name. You'll notice in all of our complex fields, now, the property keys are more of what is stored by the property, which I think makes it easier to program and more readable. I will create an issue to add a term to properties.

The defaultTripalTypes and defaultTripalValuesTemplate functions are gone

Now that site developers program their own field properties all within their field classes we don't have a need for providing default types. I also found these functions a bit weird because they provide default types implying they could be adjusted but yet they were required. Also, when we did make changes to the record_id property provided by these functions it was confusing as to what was going on (@laceysanderson commented on this). So, once I did all of the cleaning above, these functions weren't used anymore and I took them out. We can put them back if needed.

max_delta

This PR does not implement the max delta to keep large numbers of records from being loaded. It was so long/hard to get all of these changes done I didn't feel like doing this minor thing, but we still need it.

Testing

To test this PR, you need to create content that use each of the three complex field types. You must start with a completely fresh install of Tripal.

Type field & Organism field

  1. Case #1: Optional type:
    • Create an organism record without an infraspecific type. Save it
    • Edit the orgnaism, add an infraspecific type. Save it.
    • Edit the organism, remove the infraspecific type. Save it.
    • At each step the page should reflect the change as well as the organism record of Chado.
  2. Case #2: Fixed type and Organism
    • Create a gene page. It should have a type widget that is already set to 'gene' and is disabled (you can't change it). Save it. Note: Be sure to set the sequence lenght to a numeric value or the page can't be saved.
    • Edit the gene page. You still can't change the type.
    • The organism can be changed, saved, changed saved.

Prop field

  1. Go to Structure > Tripal Conent Types and select "manage fields" for the "Organism" content type
  2. Click "Add Field"
  3. Select the "Chado Property".
    • Give the property any name you want. I picked "Notes".
    • Use 'organism' as the base table.
    • Use "Notes" as the term name. It doesn't matter what vocab it's from.
    • Save
  4. Return to the organism content type created above
    • Edit the page add one or more notes. Save
    • Edit the page remove a note. Save
    • At each step the page should reflect the change as well as the organismprop records of Chado.
spficklin commented 1 year ago

Here's an example of the entire obi__orgnaism field. So much easier to understand!!!

<?php

namespace Drupal\tripal_chado\Plugin\Field\FieldType;

use Drupal\tripal_chado\TripalField\ChadoFieldItemBase;
use Drupal\tripal_chado\TripalStorage\ChadoVarCharStoragePropertyType;
use Drupal\tripal_chado\TripalStorage\ChadoIntStoragePropertyType;
use Drupal\tripal\TripalStorage\StoragePropertyValue;
use Drupal\core\Form\FormStateInterface;
use Drupal\core\Field\FieldDefinitionInterface;

/**
 * Plugin implementation of Tripal string field type.
 *
 * @FieldType(
 *   id = "obi__organism",
 *   label = @Translation("Chado Organism Reference"),
 *   description = @Translation("A chado organism reference"),
 *   default_widget = "obi__organism_widget",
 *   default_formatter = "obi__organism_formatter"
 * )
 */
class obi__organism extends ChadoFieldItemBase {

  public static $id = "obi__organism";

  /**
   * {@inheritdoc}
   */
  public static function mainPropertyName() {
    return 'label';
  }

  /**
   * {@inheritdoc}
   */
  public static function defaultFieldSettings() {
    $settings = parent::defaultFieldSettings();
    $settings['termIdSpace'] = 'OBI';
    $settings['termAccession'] = '0100026';
    return $settings;
  }

  /**
   * {@inheritdoc}
   */
  public static function defaultStorageSettings() {
    $settings = parent::defaultStorageSettings();
    return $settings;
  }

  /**
   * {@inheritdoc}
   */
  public static function tripalTypes($field_definition) {
    $entity_type_id = $field_definition->getTargetEntityTypeId();

    // Get the length of the database fields so we don't go over the size limit.
    $chado = \Drupal::service('tripal_chado.database');
    $schema = $chado->schema();
    $organism_def = $schema->getTableDef('organism', ['format' => 'Drupal']);
    $cvterm_def = $schema->getTableDef('cvterm', ['format' => 'Drupal']);
    $genus_len = $organism_def['fields']['genus']['size'];
    $species_len = $organism_def['fields']['species']['size'];
    $iftype_len = $cvterm_def['fields']['name']['size'];
    $ifname_len = $organism_def['fields']['infraspecific_name']['size'];
    $label_len = $genus_len + $species_len + $iftype_len + $ifname_len;

    // Get the base table columns needed for this field.
    $settings = $field_definition->getSetting('storage_plugin_settings');
    $base_table = $settings['base_table'];
    $base_schema_def = $schema->getTableDef($base_table, ['format' => 'Drupal']);
    $base_pkey_col = $base_schema_def['primary key'];
    $base_fk_col = array_keys($base_schema_def['foreign keys']['organism']['columns'])[0];

    // Return the properties for this field.
    return [
      new ChadoIntStoragePropertyType($entity_type_id, self::$id, 'record_id', [
        'action' => 'store_id',
        'drupal_store' => TRUE,
        'chado_table' => $base_table,
        'chado_column' => $base_pkey_col
      ]),
      new ChadoIntStoragePropertyType($entity_type_id, self::$id, 'organism_id', [
        'action' => 'store',
        'chado_table' => $base_table,
        'chado_column' => $base_fk_col,
      ]),
      new ChadoVarCharStoragePropertyType($entity_type_id, self::$id, 'label', $label_len, [
        'action' => 'replace',
        'template' => "<i>[genus] [species]</i> [infraspecific_type] [infraspecific_name]",
      ]),
      new ChadoVarCharStoragePropertyType($entity_type_id, self::$id, 'genus', $genus_len, [
        'action' => 'join',
        'path' => $base_table . '.organism_id>organism.organism_id',
        'chado_column' => 'genus'
      ]),
      new ChadoVarCharStoragePropertyType($entity_type_id, self::$id, 'species', $species_len, [
        'action' => 'join',
        'path' => $base_table . '.organism_id>organism.organism_id',
        'chado_column' => 'species'
      ]),
      new ChadoVarCharStoragePropertyType($entity_type_id, self::$id, 'infraspecific_name', $ifname_len, [
        'action' => 'join',
        'path' => $base_table . '.organism_id>organism.organism_id',
        'chado_column' => 'infraspecific_name',
      ]),
      new ChadoIntStoragePropertyType($entity_type_id, self::$id, 'infraspecific_type', [
        'action' => 'join',
        'path' => $base_table . '.organism_id>organism.organism_id;organism.type_id>cvterm.cvterm_id',
        'chado_column' => 'name',
        'as' => 'infraspecific_type_name'
      ])
    ];
  }
}
laceysanderson commented 1 year ago

Comments based on reading the overview:

laceysanderson commented 1 year ago

Unsurprisingly the ChadoStorage tests are failing now. I'm happy to go in and fix them as part of my understanding of this :-) It makes it so much easier to double check my understanding that way and it will let you focus on taking a look at my PRs instead. Thanks for all this work! ā¤ļø

spficklin commented 1 year ago

I think it's good to have everything implemented in the field but there are situations where the values of the field settings change depending on what content type they are attached to -how is this handled now?

So we have three types of settings: 1) field settings, 2) field storage settings and 3) field property settings. We were using the field storage settings as a way to specify the field property settings during setup. But that forced a design contraint that all fields should be aware that property settings could be specified in that way. It was one more thing a field developer had to keep track of. This change no longer expects that, but field developers are still free to use the field storage settings to provide property settings if they want. Now rather than this being an expectation of Tripal, it's up to the field developer to design their field they way they want.

My preference would be that fields don't expose property settings via the field storage settings but rather provide storage settings that give them all the info they need to behave differently depending on the content type or Chado table they are connected to. For example, the chado_linker__prop field adds a new setting called prop_table so it knows the name of the property table and can use for setting up the properties if need be.

https://github.com/tripal/t4d8/blob/b46300ca626853ada23bd5e6e9e93cacf2af7f1e/tripal_chado/src/Plugin/Field/FieldType/chado_linker__prop.php#L33-L37

That setting is unique to that field and helps it know how to setup the properties internally.

spficklin commented 1 year ago

Also, I believe this PR also fixes issue #311

spficklin commented 1 year ago

@laceysanderson I have added to this branch a fix for issue #322 which adds a term to the constructor for property objects. This finishes out adding terms to properties so they will be compatible with web services. Do you want me to merge it into this PR or wait unilt this gets merged and then submit another PR.

laceysanderson commented 1 year ago

I fixed the tests for Chado Storage and added one for property fields.

The ChadoStorageTest tests the ChadoStorage class and using the associated property type / value objects. Specifically, the mock situation is a single entity with the storage classes associated with:

The field classes are not created. Instead we create the property type and value classes independently, add the types to our chado storage, set the feature_id + featureprop_ids and then load the values using chado storage.

codeclimate[bot] commented 1 year ago

Code Climate has analyzed commit c91db57b and detected 6 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 2
Duplication 4

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

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

View more on Code Climate.

spficklin commented 1 year ago

Thanks @laceysanderson for fixing the testing! Is this PR ready for merging? Also, just pointing out my question above in case you didn't see it.

laceysanderson commented 1 year ago

New Years with my family has had me out of this for a couple of days. I did miss the question so thanks for pointing it out!

Is this PR ready for merging?

I haven't had a chance to test this functionality yet but code-wise everything looks good and the automated testing shows it works as expected. We will be doing a full test through before release so I am going to merge this :-) šŸ‘ Originally I was hoping to manual test it first but life got in the way šŸ¤·ā€ā™€ļø

I have added to this branch a fix for issue https://github.com/tripal/t4d8/issues/322 which adds a term to the constructor for property objects. This finishes out adding terms to properties so they will be compatible with web services. Do you want me to merge it into this PR or wait until this gets merged and then submit another PR.

As I will merge this one right now, feel free to make it a new PR :-)

Thanks for all this -code-wise it looks great and So Much more intuitive!