Skip to content
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

Fields don't follow design requirements #407

Open
spficklin opened this issue Mar 2, 2022 · 2 comments
Open

Fields don't follow design requirements #407

spficklin opened this issue Mar 2, 2022 · 2 comments

Comments

@spficklin
Copy link
Member

spficklin commented Mar 2, 2022

It seems many of the fields in both the expression and biomaterial modules don't quite follow proper design requirements for Tripal fields. The problem is that data provided by this module cannot be integrated properly into other forms of data access such as Drupal views and web services.

The major issue is that the load() function of many of the fields are not properly formatted. For example, the load() function of the sep__biological_sample field returns an array where keys are analysis_ids and values are count of the number of biomaterials per analysis. But this is not the data that is presented on the page. Instead the formatter class provides a table showing the list of biomaterials as well as some metadata about each biomaterial. This results in data that is available via the web page but not available to web services nor to Drupal views. And it results in data in web services that is not discoverable/findable. To make this work the formatter classes must use SQL to retrieve data that was not provided by the load() function. This is against the recommendation in the Developer's Guide that formatter classes should not perform any SQL (https://tripal.readthedocs.io/en/latest/dev_guide/custom_field/custom_formatter.html). They should simply format what has been given to them.

I did notice this problem with the data__gene_expression_data field. It too did not provide via the load() function what was presented on the page. I fixed that field with PR #403. But now that I've seen PR #406 it seems the problem is present in other fields as well. So, this issue is just to make note t hat the problem needs fixing.

To fix the problem the following needs to occur.

  1. All SQL statements for generating data need to be moved out of the formatter class and into the load() function of the base field class.
  2. The load() function needs to be structured such that the keys of key/value pairs are controlled vocabulary IDs. This tells Tripal what their data type is.
  3. An elementInfo() function needs to be added that describes the values of each element in the load() function. This is what tells Tripal how to use the data (e.g. sorting, filtering, querying etc.).
  4. The formatter view() function needs to be updated to use the data that is given to it rather than make queries.
@spficklin
Copy link
Member Author

spficklin commented Mar 2, 2022

I should mention that the exception to this rule that data provided by the load() function should match what is on the web page are visualization tools. For example the local__gene_expression_bargraph field that I added as part of PR #403 provides that bargraph of expression (I didn't add the bagraph, it was already there. I just created a new field for it). That obviously can't be provided by the load() function. So instead the field type was set to use a term that indicates a visualization is provided and the value provided by the load() function is the URL to the visualization. This way if someone pulls data via web services they can find the visualization if they need to.

@dsenalik
Copy link
Collaborator

dsenalik commented Mar 11, 2022

The widget on this field is just a stub, and these changes in #408 seem to have introduced a problem when trying to edit an analysis page, for example

Could not save the TripalEntity: SQLSTATE[42703]: Undefined column: 7 ERROR: column "biomaterial_id" of relation "analysis" does not exist LINE 1: UPDATE chado.analysis SET analysis_id = '4', biomaterial_i... ^
Cannot save entity

This is because I was saving information for the widget, but there is no functional widget, and the information doesn't get interpreted correctly. I'll submit a pull request for this soon. #410

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants