tripal / tripal_file

Manages site-wide files and associates them with content in a Tripal site.
1 stars 2 forks source link

Add optional file name column, admin interface, and help #20

Closed dsenalik closed 2 years ago

dsenalik commented 3 years ago

For Issue #1

This pull request adds an additional optional column to display file name, and adds an administrative page to enable this column. Most of the changes are the addition of this admin page!

While I was in there I added a help page, copying the text from README.md (and there was a typo there that I fixed that made one of the links not work)

For example: addfilename

The new configuration page: 20210417_config

The new help page: 20210417_help

dsenalik commented 3 years ago

There is a display problem with my implementation. Some file URLs will be of a format that doesn't have a useful file name, such as

https://figshare.com/articles/dataset/Carrot_F3_VCF_file/14067131/1

and this will display as

20211102_figshareexample

To resolve this, a mechanism for obtaining a file name from these types of URLs would be needed. It would have to be an additional column in the chado.fileloc table?

dsenalik commented 3 years ago

With the Nov. 4, 2021 commit, I have done the unthinkable and added a column to the fileloc table.

There is now a display_name column which, when a value is present, is used to present a file name to the user. The example above now has a file name displayed in place of the "1"

20211104_formatter

And the associated widget is where you can enter this optional value

20211104_widget

Note that none of this appears unless file name is enabled from the admin page.

spficklin commented 2 years ago

Hey @dsenalik, I've issued a pull request to your fork to resolve the merge conflict. Can you merge that in? It should then be part of this PR. I'm still working on reviewing the PR but thought I'd get this merge problem out of the way first.

dsenalik commented 2 years ago

Thanks for looking at this. I wasn't sure if this was something you would like to incorporate or not, comments welcome!

spficklin commented 2 years ago

I think you're right, we do need a filename field somewhere, and it makes sense that it would go into the fileloc table. Can I suggest a few changes though?

  1. Rather than name the field fileloc.display_name how about just fileloc.filename. Maybe we should rename the file.name field to something else. It's not really a filename maybe short_desc for a short description?
  2. For the controlled vocabulary term for the fileloc.filename field I think we should use this term instead: data:1050.
  3. The update hook needs some changes because the fixes that it provides wouldn't be available to a new installation of the module. We need to
    • Update the tripal_file_add_fileloc_table() function to have the new filename field.
    • Create the CVterm for the new filename field in the tripal_file_add_terms() function, and associate the semweb term there as well. We can call the tripal_file_add_terms() in the update hook.
    • If we add an ['update_existing' => FALSE] as an option to the calls to chado_inset_cvterm() calls then we can call the tripal_file_add_terms() directly in the update hook without duplicating the code to create the cvterms.
dsenalik commented 2 years ago

I have finally worked on making these changes. I did not yet rename the file.name field, personally I think it's okay, but can certainly do that too.

The update function was lost when I merged commit 1f84ea9 but with all these suggested changes it needed to be updated anyway.

One question, is this okay, because I explicitly specify chado, that might not be right?

    if (!db_field_exists('chado.fileloc', 'filename')) {
      db_add_field('chado.fileloc', 'filename', $filename_schema);

I also added a couple more default licenses.

dsenalik commented 2 years ago

After some discussion as to how it is confusing where to click to download the actual file, why can't you click on the filename, I made the change with https://github.com/tripal/tripal_file/pull/20/commits/d4b9eac6bd1bdb8e8e1883942c2dd004cf50271d so that when the "filename" column is enabled, the download link moves there. An example of how it looks:

20220504_appearance

dsenalik commented 2 years ago

Moved the file name generation code from the formatter to the loader, so that web services work as they should.

spficklin commented 2 years ago

Thanks @dsenalik for all the recent updates! I think I may make the change of the renaming that database 'name' field as it bugs me.

dsenalik commented 2 years ago

I think I'm done for now except for my db_field_exists question.

spficklin commented 2 years ago

Oh, right I forgot to answer that question.

Try the chado_column_exists function to see if the column exists in the table. Here is more info about that function: http://api.tripal.info/api/tripal/tripal_chado%21api%21tripal_chado.schema.api.inc/function/chado_column_exists/3.x

But for creating the column if it doesn't exist I don't think we have a corresponding function to replace the db_add_field.... I'll have to look to see if we've added a fields in an update hook before and see what we did...

spficklin commented 2 years ago

Okay, I see now what we did. I had to go look back at the code that upgrades from Chado v1.2 to v1.3. That upgrade adds new fields to the organism table and we just wrote out the SQL to "ALTER TABLE" and then passed in that SQL to the chado_query function. So in the absence of a chado_add_column function that would be the best way to make sure that the change occurs in the proper Chado schema.

dsenalik commented 2 years ago

I stole the code to generate a tripal job to update db2cv_mview for https://github.com/tripal/tripal_analysis_expression/pull/411 but in doing this, noticed that in update 7100 the variable $mview_id was never defined, so the job would not have worked. I fixed that here and have added similar code there.

spficklin commented 2 years ago

Hey @dsenalik thanks for all your work on this PR. I did not change the database column name as I said I wanted. I didn't want to keep this PR hanging out there. If I feel strongly enough to fix it later (and have time) I will.