Issue 407 changes just to sep__biological_sample field

[dsenalik]

This is the current status of my work to fix the problem noted in issue #407

So far there are only changes on the sep__biological_sample field, I have yet to look at others in this module.

It appears to work properly also in web services.

I have added support for assay content.

Live example
https://carrotomics.org/bio_data/172606
and web services
https://carrotomics.org/web-services/content/v0.1/Assay/172606

[dsenalik]

The other field in the tripal_biomaterial module sio__primary_cross_reference is a non-functional stub, so this possibly completes the issue #407 changes for fields in this module. Some of the tripal_analysis_expression fields, however, need work.

[spficklin]

Hey @dsenalik thanks for helping with this! It's much appreciated. My week is a but full but I'm going to try and squeeze in testing as I go and give comments here. So you might get requests for fixes from me as I spot things rather than in one big response.

First, when I look at the field on an analysis page I get this error:

DOException: SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "biomaterial_project" does not exist 

I think the biomaterial_project table must be a custom table you added? It's not added by this module. I think if you want to add it with this module so that the table gets added on install of the module then it would be perfectly reasonable. Do you want to submit a different PR that adds that table and any other custom linker tables for biomaterials?

[dsenalik]

That table comes from the tripal_eutils module. I don't think it would be necessary to make that module a dependency just for this table, so I will do as you said and update the installer. But is this proper, to have the same table installed by two different modules? The comment in tripal_eutils says "These tables will be in the Chado 1.4 release so let's add them here for now".

[dsenalik]

That last commit I just added adds the table to the installer. It's in this same pull request, that seems appropriate since the table and new version of the field go together.

[spficklin]

Great! I'll test it again. As long as the table definition is identical to the one in the tripal_eutils module then it shouldn't be a problem. The function for creating custom should recognize if the table already exists.

[spficklin]

Can you adjust the biomaterial_property table creation so that it occurs in a function rather than directly in the update hook? Then call the function in the tripal_biomaterial_install() function so that brand new installs will get the table, and then call the function in the update hook so that existing installations get it too.

[dsenalik]

I forgot about that, moved to a function.
update 7300 is not called on initial install, I debated whether to add it, on an initial install it would not have anything to do, though, so I left that as is.
Various typos and formatting.

[spficklin]

Hey @dsenalik rather than having a back and forth on the install file, I made some fixes and made a pull request to the branch on your fork. That way when you merge it, it should show up on this PR. See the comment on the PR on your fork for what I changed.

[spficklin]

Two issues here:

1. I don't think we should be hard-coding some attributes to appear for the biomaterial over others. Not all NCBI biomaterials have the tissue and treatment and there's nothing in this module that forces the user to provide these attributes for every biomaterial. I think these were here from before but I don't know why. I think we should think about removing them.
2. Also, we shouldn't be using vocabulary terms from a tripal vocabulary because tripal doesn't have a vocabulary. They should come from a 'local' vocabulary, established by the site admins, or from a community derived vocabulary. I believe I fixed all of these in this PR, Updated BioSample Importer #392, so no need to do anything about it here. Just pointing out.

[spficklin]

Hey @dsenalik I tested it and it seems to work great! If you could just merge in that PR I made to your fork, then I think we can merge this in. Also, the question about the hardcoded 'tissue' and 'treatment' could be added as a separate issue and dealt with later. We don't have to fix it for this PR. I'll go ahead and add the issue.

[spficklin]

PR Merged! Thanks for the fix @dsenalik