Creates rawphenotypes germplasm field #88

reynoldtan · 2021-12-22T20:02:06Z

This PR will create a Custom Tripal Field that will add raw phenotypes download functionality to germplasm page.

Metadata

Issue #

I feel this PR is ready to be merged.
This PR is dependent upon [PR #/ nothing]

Documentation:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

Description

Dependencies

Testing?

I tested on a generic Tripal Site
I tested on a KnowPulse Clone
This PR includes automated testing

Setup

On a KP clone, update rawphenotypes module and switch branch to rawphenotypes-germplasm-field
Clear cache (optional)
Setup Tripal Field in Structure -> Tripal Content Types and click on Manage Fields operation for Germplasm Accession content type.
Once in this page click +Check for new fields. This will ensure that the rawphenotypes germplasm field will be discovered.
Click Manage Display tab and set up the following items:
A. A group titled Raw Phenotypes and set the format to Tripal Pane
B. Under this group add an item clicking the + symbol preceding Germplasm Raw Phenotypes Field

NOTE: at the moment the cog icon to update id, classes and visibility option is causing an AJAX error leaving this field to be hidden by default (hide = yes).

Search a germplasm - you can use the germplasm search in homepage for example CDC Robin AGL.

The Interface

Test accessibility by changing the screen size of the browser while on germplasm page for CDC Robin AGL. Ensure that the field elements are still accessible.
Test controls work - click on Raw Phenotypes definition and link to Download Page.
Export Test: Select a Trait of interest and expand the select option for Location/Experiment.
Note some experiments are disabled when experiment is not assigned to the current user as specified in the warning on screen.
When selecting an active location/experiment the document icon activates and clicking it will launch a new window to begin data export identical to the download features seen in Download Page of rawphenotypes.
Allow module to fetch dataset and prepare export file.
Click file link to view result. The resulting phenotypes should be filtered to match the germplasm, experiment, location and trait.

ruobinLiu · 2022-01-17T21:39:01Z

For step #7: downloaded file contains germplasm, location and trait applied, but misses experiment. An additional trait, # of Seeds Planted (count), is always included by default.

.

reynoldtan · 2022-02-22T15:33:22Z

Experiment column added to results

ruobinLiu · 2022-03-01T19:47:10Z

I am now able to see the experiment column on my new dev fresh, PR works as expected. Great job 👍

includes/TripalFields/ncit__raw_data/ncit__raw_data.inc

laceysanderson · 2022-03-04T22:03:46Z

includes/TripalFields/ncit__raw_data/ncit__raw_data.inc

+      $summary['experiments'] = count(array_unique($cache_exp));
+      $summary['locations'] = count(array_unique($cache_loc));
+
+      $entity->{$field_name}['und'][0]['value']['NCIT:Raw Data'] = array(


I don't think the value should not be keyed by the field name. Also, you should always use the new array notation: [ ] rather than the old style for PHP compatibility reasons.

Suggested change

$entity->{$field_name}['und'][0]['value']['NCIT:Raw Data'] = array(

$entity->{$field_name}['und'][0]['value'] = array(

All of the keys exposed through the value should be cvterms and should only represent data. The icons feel out of place here as it relates to the format, not the data. All sub-keys as well so the counts in the summary should also have cvterms as keys.

laceysanderson · 2022-03-04T22:09:47Z

includes/TripalFields/ncit__raw_data/ncit__raw_data.inc

+        if ($trait_set) {
+          foreach($trait_set as $trait_id => $trait_name) {
+            // Trait id, project id and name + location:
+            $trait_experiment_location[ $trait_id . '_' . $trait_name ][] = $trait_id . '#' . $item->project_id . '#' . $item->name . '#' . $item->location;            


This feels very pre-formatted for what you want in the formatter. It should likely be broken out into a nested array with cvterm keys and then compiled to this format in the formatter.

laceysanderson · 2022-03-04T22:14:12Z

includes/TripalFields/ncit__raw_data/ncit__raw_data_formatter.inc

+      // Create markup.
+      // Refer to this ID for CSS styling.
+      $id = 'rawphenotypes-germplasm-raw-phenotypes-field';
+      $markup = '


It feels very weird to see this much HTML in PHP strings... perhaps this should be in a template?

laceysanderson · 2022-03-04T22:19:51Z

rawpheno.module

+ * Implements hook_field_group_build_pre_render_alter().
+ * Apply css and js.
+ */
+function rawpheno_field_group_build_pre_render_alter(&$element) {


won't this add these resources indiscriminately?

excellent documentation Co-authored-by: Lacey-Anne Sanderson <las166@mail.usask.ca>

reynoldtan · 2022-03-21T20:39:51Z

Revised field formatter to use template file for the markup. I have kept all array construct - array() since it is used throughout the module and I have checked off item My code follows the code style of this project above. In my recent work of the module for Drupal 8/9 all instances of this array have been converted to the short array syntax.

laceysanderson

Code Review

hook_field_group_build_pre_render_alter() removed and resources added in formatter
html moved into template file
formatting removed from the field class
Keys of values array should all be cvterms

The keys are still incorrect…

  // # SUMMARY COUNT:
  $summary = array();
  $summary['traits'] = count($trait_experiment_location);
  $summary['experiments'] = count(array_unique($cache_exp));
  $summary['locations'] = count(array_unique($cache_loc));

  $entity->{$field_name}['und'][0]['value'] = array(
    'user'      => $current_user,
    'germplasm' => $germplasm,
    'summary'   => $summary,
    'traits'    => $trait_experiment_location,
  );

All of the array keys should be cvterms! Even the nested ones. This is needed to work with web services.

Here is an example of what I mean:
https://github.com/tripal/trpfancy_fields/blob/master/includes/TripalFields/ChadoChartField.inc#L234-L237

      $entity->{$field_name}['und'][0]['value']['hydra:member'][] = array(
        'rdfs:label' => $r->{$label_column},
        'local:count' => $r->{$count_column},
      );

We use the hydra:member term to indicate an array of data such as your trait locations. This array should not be associative (i.e. keys should be simple iterative numbers) but that should not be a problem in your case as you don't access them by key anyway?

Then you need to create local terms for each associative key. Specifically, the following keys should all be cvterms:

user
- permission
- experiments
germplasm
- id
- name
summary
- traits
- experiments
- locations
traits
- trait_id
- project_id
- project_name
- location

reynoldtan · 2022-04-18T21:22:00Z

Revised the field instance and formatter to use cvterms as array keys. I have setup a cv name and named it in the same way as other cvs used by rawphenotypes (ie phenotype_measurement_units, phenotype_measurement_types etc.). I chose the name phenotype_customfield_terms and created the cv terms experiment, id, name, location, summary, experiment, user_experiment and trait to be used as keys. This cv will hold other terms in the future should raw phenotype custom field require other more terms.

laceysanderson

✅ Code looks good with the most recent changes 🎉

On to testing! The field seems to work as intended except for the download functionality. I select a location/experiment for the # Nodes on Primary Stem at R1 (1st; count) and then click download but the resulting file does not have that trait. I used CDC Robin AGL to test.

I'm also wondering a bit about the design... I'm worried that Kirstin would be most likely to want all locations for a given experiment or even all data for that trait in general. Maybe you add some "ALL" options in the select list?

reynoldtan · 2022-04-21T22:51:01Z

On it, I will setup the ALL option in the next commit 👍

reynoldtan · 2022-05-02T17:41:49Z

I have added a filter by switch to allow export of phenotypes by either experiment or location + experiment. In addition to this option, users can download everything (all experiment user has permission to) in a trait + germplasm using the download all button preceding each trait. I also add expand table view and search trait to make finding a trait of interest less painful 😄 .

Tested

Export all phenotypes (1 experiment where I don't have permission to and this got excluded in the result)
By Location + Experiment
Location + Experiment where the trait was not measured (no stock id - plant id) but is a trait in an experiment (the missing trait error in the previous review)
By experiment
Raw Phenotypes Download page and works just as is without experiment column seen in export coming from raw data field.

laceysanderson · 2022-06-24T22:32:29Z

I'm loving the new "Export everything in a trait + germplasm" button! Did a quick review on the new code since the last review and I think this is good :-)

I'm going to merge now but we won't update KP until July so nothing goes wrong right before the workshop ;-p

reynoldtan added 4 commits December 22, 2021 14:00

Creates rawphenotypes germplasm field

d5f9ff4

Filters resultset to match stock id

fe6bc13

Filters resultset to match stock id

5541a67

Filters resultset to match stock id

158c27e

Adds experiment column header

6437f82

laceysanderson requested changes Mar 4, 2022

View reviewed changes

reynoldtan and others added 5 commits March 9, 2022 14:41

Update includes/TripalFields/ncit__raw_data/ncit__raw_data.inc

6fa0ccc

excellent documentation Co-authored-by: Lacey-Anne Sanderson <las166@mail.usask.ca>

@todo: replace markup with a template file

5dfbe83

Implements hook_theme

1f872cc

Finally, add css works

6dc7913

Revises rawphenotypes germplasm field formatter to use template file

b640548

laceysanderson requested changes Apr 13, 2022

View reviewed changes

reynoldtan added 2 commits April 15, 2022 16:34

Revises field array keys to use cvterms @todo: test

5699d36

Revises field instance and formatter to use cvterms as array keys

c666964

laceysanderson requested changes Apr 21, 2022

View reviewed changes

reynoldtan added 4 commits April 22, 2022 17:04

Revises raw data field - allow more options to export data

3af2452

Adds more download options. @todo: prepare export data

3ef128b

More download options @todo: test all components

a7061c6

Add more export option

755be53

laceysanderson approved these changes Jun 24, 2022

View reviewed changes

laceysanderson merged commit 75addd0 into master Jun 24, 2022

laceysanderson deleted the rawphenotypes-germplasm-field branch June 24, 2022 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creates rawphenotypes germplasm field #88

Creates rawphenotypes germplasm field #88

reynoldtan commented Dec 22, 2021 •

edited

Loading

ruobinLiu commented Jan 17, 2022

reynoldtan commented Feb 22, 2022

ruobinLiu commented Mar 1, 2022

laceysanderson Mar 4, 2022

laceysanderson Mar 4, 2022

laceysanderson Mar 4, 2022

laceysanderson Mar 4, 2022

laceysanderson Mar 4, 2022

reynoldtan commented Mar 21, 2022

laceysanderson left a comment

reynoldtan commented Apr 18, 2022

laceysanderson left a comment

reynoldtan commented Apr 21, 2022 •

edited

Loading

reynoldtan commented May 2, 2022

laceysanderson commented Jun 24, 2022

	$entity->{$field_name}['und'][0]['value']['NCIT:Raw Data'] = array(
	$entity->{$field_name}['und'][0]['value'] = array(

Creates rawphenotypes germplasm field #88

Creates rawphenotypes germplasm field #88

Conversation

reynoldtan commented Dec 22, 2021 • edited Loading

Metadata

Description

Dependencies

Testing?

ruobinLiu commented Jan 17, 2022

reynoldtan commented Feb 22, 2022

ruobinLiu commented Mar 1, 2022

laceysanderson Mar 4, 2022

Choose a reason for hiding this comment

laceysanderson Mar 4, 2022

Choose a reason for hiding this comment

laceysanderson Mar 4, 2022

Choose a reason for hiding this comment

laceysanderson Mar 4, 2022

Choose a reason for hiding this comment

laceysanderson Mar 4, 2022

Choose a reason for hiding this comment

reynoldtan commented Mar 21, 2022

laceysanderson left a comment

Choose a reason for hiding this comment

reynoldtan commented Apr 18, 2022

laceysanderson left a comment

Choose a reason for hiding this comment

reynoldtan commented Apr 21, 2022 • edited Loading

reynoldtan commented May 2, 2022

laceysanderson commented Jun 24, 2022

reynoldtan commented Dec 22, 2021 •

edited

Loading

reynoldtan commented Apr 21, 2022 •

edited

Loading