Yosef dev2

[yosefdroth]

The major additions so far:

1. Seperated the protein metadata from AbundanceTable so that AbundanceTable does not need the list of uniprot id's from the metadata.
2. Added more metadata to the protein page
3. Added tooltips to Taxonomic Similarity and Chemical Similarity

[coveralls]

Pull Request Test Coverage Report for Build 946

• 91 of 100 (91.0%) changed or added relevant lines in 8 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.5%) to 91.397%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/scenes/BiochemicalEntityDetails/DataTable/DataTable.js	15	16	93.75%
src/scenes/BiochemicalEntityDetails/Protein/AbundanceDataTable.js	18	19	94.74%
src/scenes/BiochemicalEntityDetails/Reaction/RateConstantsDataTable.js	10	11	90.91%
src/scenes/BiochemicalEntityDetails/TaxonomyFilter.js	12	13	92.31%
src/scenes/BiochemicalEntityDetails/Protein/MetadataSection.js	20	22	90.91%
src/scenes/BiochemicalEntityDetails/LoadExternalData.js	11	14	78.57%

Totals
Change from base Build 920:	-0.5%
Covered Lines:	1203
Relevant Lines:	1270

---

💛 - Coveralls

[jonrkarr]

Protein metadata

• Directly loading descriptions from KEGG is good. I had intended for us to incorporate this meta data directly into our database, but this is sufficient for our current purposes.
• The contents of the "Names" section doesn't quite fit with the content.
  • Remove the name. We don't need to display this since it is already displayed in the title of the page.
  • Rename the section to "Cross references", "Identifiers", or similar.
• Provide hyperlinks from EC codes to Enzyme (or another database that you prefer), e.g. https://enzyme.expasy.org/EC/1.1.1.1

Tooltips

• The tooltips look nice
• The tooltips are not implemented as we discussed using the tooltip key provided by ag-Grid, but the way you did it is ok.
• The tooltip text needs to make sense in all of the contexts in which they are displayed. Currently, the text refers to the filters. Instead, the text should explain the attribute.
  • Change the chemical similarity tooltip to the following:
    • Tanimoto distance between the structure of the queried metabolite and the structure of each observed metabolite.
  • Change the taxonomic similarity tooltip similarly

Visual style

• To be consistent with the rest of the website, only the first character of titles should be capitalized. Change the following
  • KEGG Orthology ID --> KEGG Orthology id
  • EC Code --> EC code

HTTP requests

• Using axios.all is good. I wasn't aware of this. This makes it easier to deal with multiple concurrent requests. The error handling and cancellation handling seems to be implemented correctly.
• Add cancellation and error handling to the HTTP query in LoadExternalData.js. You can use the same method I implemented. Here's the whole code for this:

   import React, { Component } from "react";
   import PropTypes from "prop-types";
   import axios from "axios";
  
   class LoadExternalData extends Component {
     static propTypes = {
       url: PropTypes.string.isRequired,
       processor: PropTypes.func.isRequired
     };
     
     constructor(props) {
       super(props);
       this.state = { text: "" };
       this.cancelTokenSource = null;
     }
     
     componentWillUnmount() {
       if (this.cancelTokenSource) {
         this.cancelTokenSource.cancel();
       }
     }
  
     componentDidMount() {
       if (this.cancelTokenSource) {
         this.cancelTokenSource.cancel();
       } 
       this.cancelTokenSource = axios.CancelToken.source();
       axios
         .get(this.props.url, {
           headers: { "Content-Type": "application/json" },
           cancelToken: this.cancelTokenSource.token
         })
         .then(response => {
           console.log(response.data);
           const processed_data = this.props.processor(response.data);
           this.setState({ text: processed_data });
         })
         .catch(
            genApiErrorHandler(
                [url],
                "Unable to load metadata."
            )
         )
        .finally(() => {
            this.cancelTokenSource = null;
         });
     }
  
     render() {
       return <div>{this.state.text}</div>;
     }
   }
  
   export { LoadExternalData };
  

Coding style

• Class names should match file names.
  • LoadData --> LoadExternalData or vice-versa.
• Method names should be in lowerCamelCase.
  • processUniprotAPI --> processUniprotApi
  • processedData.ec_code --> processedData.ecCode`
• Remove, comment out console logging, or only making logging only occur when the environment is set to development or test*. Logging is in LoadExternalData.js and several other files.

Tests

• Fix the test failures
• If more execution time is needed for the integration tests, increase defaultCommandTimeout in cypress.json. The value is in milliseconds.

[yosefdroth]

I have made those changes. I also updated taxonomy in the metabolites page.

There is currently one integration test which is failing -- TaxonomyFilter. I made some changes to TaxonomyFilter, so this makes sense, but I cannot figure out why the test is failing. The filter appears to work normally in the actualy page. Do you think you could take a look?

[jonrkarr]

The changes look good. I push a change which adds more information about the tooltips. In particular, I expanded what the taxonomic distance measures. The previous definition was a bit circular. I also added more information about the Tanimoto coefficient for users who are not familiar with this.

I still need to look into the test failure

[jonrkarr]

Fixed integration test

I fixed the taxonomy filter integration test. The test needed to be updated for the changes that you made to setModel and getModel. These functions are necessary to implement ag-Grid's filter interface. Basically getModel returns a representation of the filter and setModel applies these representations.

Sorting of taxonomic distance has been broken

The changes that you made to the taxonomy filter largely work. However, the column no longer sorts correctly, and I think there was a simpler way to implement this. ag-Grid makes it easy to distinguish between the values of attributes are how they are displayed. This can be done with the valueFormatter method. Under this paradigm, the taxonomic distance value could have remained an integer, and a valueFormatter method could have been implemented to translate these integer values into strings for display in the table. Doing it this way, the taxonomic distance column would have continued to be sortable (based on the integer distances). Instead, the column is now sorted alphabetically by the strings "species", "genus", ... which is not sematically meaningful. I opened #207 for this.

Visual style

To be consistent with other columns, please make the taxonomic distances in the tables start with upper case letters.

[jonrkarr]

I corrected the capitalization of the taxonomic distance labels in the data tables.