search

bihealth / sodar-server

SODAR: System for Omics Data Access and Retrieval
https://github.com/bihealth/sodar-server
MIT License
14 stars 3 forks source link

Process name column allows selecting process with editing disabled #1875

Closed sellth closed 5 months ago

sellth commented 5 months ago

The insert row functionality of the samplesheet editor gives users the ability to change "Protocol Name" of a process node. These values should be fixed and changing one row would lead to ISA parsing errors.

grafik

mikkonie commented 5 months ago

This is not a bug. These do not cause parsing errors, unless there is some special case where they do. In that case, please let me know where that happens.

Furthermore, by default, SODAR sets up process columns as uneditable, with the default value being applied being the one already in the existing sheet/template. So by default they already are "fixed". Closing.

sellth commented 5 months ago

These changes raise ModerateIsaValidationWarning: Parameter Value "xxx" not declared for Protocol "yyy" in investigation file.

Users should not be allowed to edit these cells at all imho as this almost always results in an invalid state. That SODAR does not show the parser warnings after saving the samplesheet is also questionable.

mikkonie commented 5 months ago

Stop reopening closed tickets.

If you want a ticket to restrict processes to a single value per column, open a feature request for that. I'm all for it, if there are no objections from other expert users. Also name that issue descriptively, "User can make invalid choices during Insert Row" means nothing.

To reiterate once more: with default settings this "issue" does not exist in the first place, as the default (and usually only) value gets autofilled and the user does not get to change anything.

mikkonie commented 5 months ago

Reopening myself :P It appears there actually is a bug in here. Even though the editing is disabled and a default value set, the choice dropdown for a process cell can be entered.

This definitely should not be and it has definitely worked before. Looking into it.

mikkonie commented 5 months ago

This is interesting. This only happens with ObjectSelectEditor when used on protocol ref columns, while other editors exhibit expected behaviour. The fieldEditable value is correctly set to false upon initial building of the grid, but the editor still opens.

I guess this must go wrong somewhere in enableNextNodes(). Investigating..

Update: this doesn't look correct at least.

// If the next node is a material, enable editing its name
// Else if it's a process, enable editing for all cells (if available)
mikkonie commented 5 months ago

Fixed. That was a braindead developer error, no excuse.