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

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

Merged
merged 14 commits into from
May 30, 2022

Conversation

dsenalik
Copy link
Collaborator

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 dsenalik mentioned this pull request Apr 17, 2021
@dsenalik
Copy link
Collaborator Author

dsenalik commented Nov 2, 2021

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
Copy link
Collaborator Author

dsenalik commented Nov 4, 2021

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
Copy link
Member

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
Copy link
Collaborator Author

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

@spficklin
Copy link
Member

spficklin commented Mar 14, 2022

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
Copy link
Collaborator Author

dsenalik commented May 4, 2022

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
Copy link
Collaborator Author

dsenalik commented May 4, 2022

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 d4b9eac so that when the "filename" column is enabled, the download link moves there. An example of how it looks:

20220504_appearance

@dsenalik
Copy link
Collaborator Author

dsenalik commented May 4, 2022

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

@spficklin
Copy link
Member

spficklin commented May 4, 2022

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
Copy link
Collaborator Author

dsenalik commented May 4, 2022

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

@spficklin
Copy link
Member

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
Copy link
Member

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
Copy link
Collaborator Author

dsenalik commented May 7, 2022

I stole the code to generate a tripal job to update db2cv_mview for tripal/tripal_analysis_expression#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 spficklin merged commit 7ff32df into tripal:7.x-1.x May 30, 2022
@spficklin
Copy link
Member

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.

@dsenalik dsenalik deleted the issue1 branch May 31, 2022 17:15
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

Successfully merging this pull request may close these issues.

2 participants