search

pombase / canto

The PomBase community curation tool
https://curation.pombase.org
Other
19 stars 7 forks source link

Metagenotype manage link doesn't enable correctly #1804

Closed jseager7 closed 5 years ago

jseager7 commented 5 years ago

I've noticed that the button that links to the Metagenotype Management page from the Genotype Management page never seems to enable, even when the conditions for creating a metagenotype are satisfied (for example, when there is at least one pathogen genotype and one host genotype). The button also doesn't disable immediately (or isn't disabled by default), meaning that it's possible to skip the check by clicking the button fast enough. The bug is probably to do with a logic error in genotypeManageCtrl.

jseager7 commented 5 years ago

I thought that this problem could be fixed by just checking for a pathogen genotype, since I assumed that the gene confirmation page didn't allow the user to progress unless they'd already added a host organism (I figured that we don't need to check for a host genotype, since metagenotypes can use hosts with no genes).

Unfortunately, PHI-Canto seems to regard a session with only one pathogen (and no hosts) as a valid session, so my solution isn't sufficient: it will still enable the metagenotype page even if there are no host organisms in the session. The only solution is to check the list of organisms as well, but the organism service isn't currently injected into the Genotype Management controller, and it seems a bit excessive to inject a whole service just for the sake of one simple check.

@kimrutherford I was hoping we could store the state of the metagenotype management link (that is, whether it's enabled or disabled) in some sort of session state service instead, but I don't think Canto currently has anything like this.

@CuzickA I can go ahead with my solution for now if we think that it's unlikely curators will be curating data solely for pathogens (and adding no hosts). Alternatively, if it was never intended that sessions should only curate for pathogens – meaning they should always collect data on pathogen–host interactions; at least one pathogen and one host – then the logic of the gene confirmation page needs to be changed to reflect this.

kimrutherford commented 5 years ago

I was hoping we could store the state of the metagenotype management link (that is, whether it's enabled or disabled) in some sort of session state service instead, but I don't think Canto currently has anything like this.

Yep, it doesn't do that at the moment but it could. Let's have a Skype call about it next week.

jseager7 commented 5 years ago

We've decided we can ultimately fix this problem by moving the responsibility for fetching and storing organisms from organismSelectorCtrl to genotypeManageCtrl – following the pattern of 'pushing state up'. This should also provide a more reliable fix to other issues (such as #1790). Unfortunately, this refactoring will affect the Metagenotype Management page too, since that also depends on the organismSelector directive.

Longer term, we could move all of the model data for the Genotype Management page into a model service that depends on the Curs data service – for example, GenotypeManageModelService – which will be a shared data store for the controllers on the Genotype Management page. The controllers can request changes to this service using callbacks or events, or they could be allowed to manipulate the model directly. We'd also have to ensure that changes to the model were synced back to the server automatically.

jseager7 commented 5 years ago

This has been fixed by #1825 (specifically commit c1ac25e4f4485944f753483b3470a5eb59c9914e).