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

Support alternative names #304

Open
mluypaert opened this issue Jun 30, 2020 · 23 comments
Open

Support alternative names #304

mluypaert opened this issue Jun 30, 2020 · 23 comments
Assignees
Labels
enhancement New feature or request

Comments

@mluypaert
Copy link
Member

Most NS entities currently have a primary wormbase identifier (accession, e.g. gene WBGene00160843) and a display identifier (public name, e.g. Cbn-tufm-1.2). However, in scientific literature, genes (and other entities) are often referred to using alternative identifiers and names.
These names are collected together into an “Other names” list attribute in AceDB, but are not currently captured in Datomic. The Datomic names service would be a convenient mechanism for recording and tracking these additional "Other names", so support for other-names should be added (to the Datomic Names Service).

@mluypaert mluypaert added the enhancement New feature or request label Jun 30, 2020
@mluypaert mluypaert self-assigned this Jun 30, 2020
@azurebrd
Copy link

azurebrd commented Jul 3, 2020

It would also be useful (for Caltech) to have the Species, Split_into, and Merged_into, but it's fine if it's out of scope.

@Paul-Davis
Copy link

@mluypaert I have branched the above requests from @azurebrd into a new issue and we can discuss it on the new_names_service slack

mluypaert added a commit that referenced this issue Jul 9, 2020
…to add/delete a set of other-names from a gene its other-names #304
mluypaert added a commit that referenced this issue Jul 10, 2020
mluypaert added a commit that referenced this issue Sep 10, 2020
… to add/delete sets of other-names from genes other-names attribute in batch #304
mluypaert added a commit that referenced this issue Sep 11, 2020
…sponse structure to state before cb1f4d7 (and analoguous for other-names) #304
@mluypaert
Copy link
Member Author

Related commit: eda2c93 (forgot to add issue tag to commit msg)

@mluypaert
Copy link
Member Author

Additional commits I forgot to link to this issue: 649adea, 7777626.
Last commit on feature/iss-304-other-names branch at this point: 7777626

@mluypaert
Copy link
Member Author

@sibyl229
I completed the API development needed for this new feature, on branch feature/iss-304-other-names (not merged into master yet, as I only want to do so once all development for this issue has been completed).
Updated swagger docs are available at https://test-names.wormbase.org/api-docs/index.html#/. Here's a short summary of the API endpoints with new input/output data, and some caveats:

  • /gene GET requests now also searches the genes for pattern matches in :gene/other-names and returns a gene's other-names if a gene has any
  • /gene POST requests can now also take other-names as input data to create genes with alias names
  • /gene/{identifier} GET requests now also returns the gene's other-names if the gene has any
  • /gene/{identifier} PUT requests DO NOT support the update of other-names for a gene, there's two new endpoints for that (see below)
  • gene/{identifier}/split POST requests now support specifying (optional) other-names for the split-product

And these are the new API endpoints:

  • /gene/{identifier}/update-other-names PUT requests should be used to add new (additional) other names to a gene
  • /gene/{identifier}/update-other-names DELETE requests should be used to retract other names from a gene

I've decided to implement other-names updates through these new API endpoints rather than through the existing /gene/{identifier} PUT endpoint because this would be incredibly tricky to prevent race conditions when two curators are updating the other-names of a gene at the same moment (because of datomic properties), and because I think (after consolidation with Paul Davis) that this would be the way curators would typically want to edit a gene's other-names: by adding or removing specific names, rather than redefining the complete set.

In order to make this new feature available to the curators, the web client would need some updates. Here's the list of things I thought of so far that would need changing:

  • It would be useful for the Gene search result dropdown to also show the other-names of the matching genes (would make the search results more understandable when the pattern does only match in the other-names)
  • The recent activities (gene overview page) and change history (gene detail page) views would need to be updated to allow the display of multi-cardinality value updates (more than one value can get added or removed to the same attribute in one API operation)
  • The gene details page would need a new field to represent the gene's other-names, and would need to allow updates to those names. I am thinking the most practical solution to do this could be a select box (or something similar) which allows adding/removing elements from the list (something similar to this). Registering those add/remove actions could then be used to build the appropriate /gene/{identifier}/update-other-names API endpoint calls to submit when the user clicks the "update" button. I'm not bit on GUIs so feel free to implement it differently if you think of a more intuitive/useful way to do so.
  • The split gene view would need an extra field to allow the definition of the new split-product's other-names

Would these web client changes be something you can take on some time in the coming months? Definitely not a high priority, this can be done when you would have the time for it. Feel free to ping to me on slack for question/clarifications.

@sibyl229
Copy link
Contributor

Hi @mluypaert @Paul-Davis this is going to be a bit tricky. It changes the assumptions of how the UI is designed for, by having separate API endpoints for updating other-names from updating everything else. And it is going to be usability issue that needs to be addressed, when having to use two different forms, two submit buttons, or two responses when performing what is perceived as a single action.

It would be better to know about this before the API is implemented, so the options isn't so limited.

Before we delve into the implementation, can we clarify that this is a feature people actually use through the name service UI? I know many curators use the Caltech OA to submit changes. It's a question of which or both systems need to be change.

Also, let me loop in @azurebrd in case OA needs to be changed, to hear his thoughts on this.

@azurebrd
Copy link

Thanks @sibyl229 !

The OA doesn't update the names service. There's an export function using the wb-names-export.jar to get strains and variations (e.g.)
java -cp wb-names-export.jar clojure.main -m wormbase.names.export strain /tmp/strains.csv
We use that output on a cronjob to populate the data that the OA looks at for its autocompletes. If that will keep working, then that part's fine.

Separately we have another form, which allows curators to create objects through the names service, and if that works, to also populate the same postgres tables that the OA uses for strain/variation autocomplete. This uses the API
https://(test-)*names.wormbase.org/api/entity/<strain|variation>/
sending json with data->name, prov->why, prov->who->email
If that's also staying the same, then that part's also fine.

If either of those won't work, please let me know, and we can either talk about it, or let me know how things should be updated in the future.

@mluypaert
Copy link
Member Author

@sibyl229 Sorry, didn't think about that aspect when I was implementing this (first assignment at Wormbase). Would it be possible to call multiple endpoints with each a part of the data upon clicking a single update button, or is that technically impossible the way the UI is implemented? Because even if you would use a separate form for updating the other-names, this might be required as you'd need to POST to add names and DELETE to remove others.

@mluypaert
Copy link
Member Author

@azurebrd
Nothing changed to strains and variations, only some gene endpoints, so that should pose any problems for the forms you use.

As for the export, do you also use this for gene feature exports, or only for strains and variations? I still need to review the export tool, but for genes I may need to make some changes to be able to export the other-names (as it's a multi-cardinal field).

@azurebrd
Copy link

@mluypaert cool cool.

Oh, I'd missed the gene stuff. Originally we were getting that from acedb dumps, so it became a separate set of scripts a long while ago.

We use this to get gene, locus, sequence, dead_live_suppressed, biotype, species
java -cp wb-names-export.jar clojure.main -m wormbase.names.export gene genes.csv

Separately we use this to get Other_name, Split_into, and Merged_into
ftp://ftp.sanger.ac.uk/pub/consortia/wormbase/STAFF/nightly_geneace/genes.ace.gz

As long as that last file keeps getting generated into the ftp, we'll be fine with things as is. At some point it wasn't feasible to get those 3 fields into the names service, and that was fine with Caltech.

Caltech doesn't have a separate tool to create gene objects in the names service, and as far as I know, we don't have plans to.

@sibyl229
Copy link
Contributor

sibyl229 commented Oct 23, 2020

@mluypaert @Paul-Davis no problem! Just for future references.

But currently the UI is designed to track only the current state of the form, and not what gets deleted from a list and what gets added, which is what the API expects as input.

To get around this uncertainty and avoid confusion with what gets submitted with the "Update" button, here is what I think might work. And I put together some mockups to check if it works with the use cases you have in mind.

Here is gene update form:
iPad mini - 1

Clicking on the "Add other names" button brings up a dialog to submit new additional name. The dialog prevents the use of "Update" button (on gene form) until the other name is added:
iPad mini - 2

Clicking on the "Delete" button next to one of the alternative name brings up a dialog to delete that particular name. Similarly, the dialog prevents the use of "Update" button (on gene form) until the other name deletion is complete.
iPad mini - 3

The key limitation to this approach is that deleting each alternative name requires a separate transaction, and filling in the reason field another time. And likely the sane with each addition of an alternative name. I might be able to avoid the limitation for adding an alternative name, but I need to play with the code to be sure.

@mluypaert
Copy link
Member Author

@sibyl229 Thanks for the proposal. This seems like a decent solution to me, but the key limitation you indicated having to submit separate transactions and reasons for every addition/deletion might indeed be a problem. I think I have an idea on how to fix this for the deletions though. If the current other-names list would have a way to select multiple of those names (through checkboxes, a select-multiple form element or something similar), then a single "delete selected" button at the bottom could direct the user to the dialog requesting a reason, processing all as one transaction? And for adding new other-names, this might work through processing a multi-line text area with every new line being a new name to add?

@sibyl229
Copy link
Contributor

sibyl229 commented Oct 26, 2020

@mluypaert thanks for the ideas. Brainstorming is great!

Adding other names can probably work this way. Deleting other names might be trickier.

  1. We need to prevent people from using the existing gene "Update" button. The existing update button appears to be the button to do the job, but it doesn't do what we intend. The dialog serves the purpose of locking people out of some part of the UI that they shouldn't be using while editing gene names.

  2. We need to track which names will be deleted, which the forms doesn't yet track. The forms only track what names will remain.

Can we get confirmation from @Paul-Davis and others that being able to delete multiple names thin one transaction through the UI is important?

@mluypaert
Copy link
Member Author

Adding other names can probably work this way. Deleting other names might be trickier.

  1. We need to prevent people from using the existing gene "Update" button. The existing update button appears to be the button to do the job, but it doesn't do what we intend. The dialog serves the purpose of locking people out of some part of the UI that they shouldn't be using while editing gene names.

Perhaps we can use a "delete other-names" button then to open a pop-up which contains a multi-select form populated with all current other-names and a reason textfield instead then?

  1. We need to track which names will be deleted, which the forms doesn't yet track. The forms only track what names will remain.

Multi-select form fields are standard HTML, which return the selected fields upon submission. I assume that should be usable to capture the selected genes to delete upon submission (of the delete-other-names popup) and create the delete API call from it? When successful, the API will then return the new current other-names after deletion (remaining ones, and potential extras if multiple people would edit the same set simultaneously), which can then be used to update the main gene form.

Can we get confirmation from @Paul-Davis and others that being able to delete multiple names thin one transaction through the UI is important?

I'll check with him tomorrow.

@Paul-Davis
Copy link

@sibyl229 I think in terms of the UI deleting Other_names individually is fine. If people identify the need to remove multiple in a single transaction then maybe we will need to revisit, but I can't see it being an issue at the moment.

@sibyl229
Copy link
Contributor

@Paul-Davis @mluypaert that's great! Thanks for checking!

@sibyl229
Copy link
Contributor

Having discussed with @mluypaert , it would be nice to have the add and delete other name endpoints to accept JSON body of the following format:

{
  data: {
    other-names: [ name1 , name2 ... ]
  },
  prov: {
    ...
  }
}

This format would be more conventional with respect to the existing API body, and would allow the UI form to bind the input elements with the attributes of the JSON body.

@sibyl229
Copy link
Contributor

sibyl229 commented Nov 2, 2020

@mluypaert I forgot to make a note here that the UI work for this is mostly done, with some functionality still requires the API change to be tested.

When the API change is available, could you let me know? There is no rush on this, just so I can test it myself before handing it over.

mluypaert added a commit that referenced this issue Nov 6, 2020
@mluypaert
Copy link
Member Author

@sibyl229 The requested API change is now available on the test env (and in the feature/iss-304-other-names branch) so you can go ahead and test your UI code when you have the time for it.

@sibyl229
Copy link
Contributor

@mluypaert @Paul-Davis i have tested the UI for alternative names against test env. It seems fine to me.

I have this Pull Request against the feature/iss-304-other-names with the UI changes: #323 . This would allow us to deploy the UI changes on the test-env, and tested together with the API changes there. Do you think this approach works?

If there is anything else I need to do, please let me know.

@mluypaert
Copy link
Member Author

mluypaert commented Dec 4, 2020

@sibyl229 PR #323 is now merged and all latest changes for this issue, including the UI, are now live on the test-environment.

@mluypaert
Copy link
Member Author

@sibyl229 I know its been a while, but is there anything still to be tested/verified to work here, or are we good to merge this into master and publish it as a new version on the production name service?

@sibyl229
Copy link
Contributor

sibyl229 commented Mar 8, 2021

@mluypaert I tested it again just now to check if anything regressed. It seems to work fine. Would it be good to have those who use it check it over, if they haven't done so already? If they find a bug, we could fix it.

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

No branches or pull requests

4 participants