update manifest conditional format by asset type #4752
Conversation
…uired-fields-format
| SequencescapeExcel::ConditionalFormatting.new( | ||
| name: 'empty_mandatory_cell', | ||
| options: { | ||
| 'type' => :cellIs, |
There was a problem hiding this comment.
I am not really sure actually, I basically copy past the object as they are defined in the conditional_format yml file
| # SampleManifest::Generator | ||
| class SampleManifest::ColumnConditionalFormatUpdater | ||
| # Predefined conditional formats for required fields. | ||
| REQUIRED_COLUMNS_CONDITIONAL_FORMATS = [ |
There was a problem hiding this comment.
Messaged re an alternative/ additional way of doing this :)
There was a problem hiding this comment.
Messaged re an alternative/ additional way of doing this :)
The proposed solution is to modify the columns.yml config file. This will result in modifying the column format for all templates where the columns in question are present. Here, the column background is set to red, which may cause confusion, as the columns may be present without necessarily having the "not empty" constraint. For example, in the multiplex library manifest types, the library_type column is present but not required to be filled in.
There was a problem hiding this comment.
Where is the code that makes library_type required for one manifest type but not the other?
I would prefer a different approach, as this adds a new place where columns are configured - it's all quite neat at the moment, in columns.yml.
Can't find the words to explain this properly, but I also don't think that 'by asset type' will turn out to be a common thing to control the requiredness.
Another thing we can do is, if we want a column to behave differently in one manifest type vs another, we make a new column definition for it - e.g. see collected_by_for_cardinal, collected_by_for_controls and collected_by_for_scrna_core, which all save to the 'collected_by' column, but have different configuration, to be used in different manifest types.
There was a problem hiding this comment.
Where is the code that makes
library_typerequired for one manifest type but not the other?
It is happening here:
def update_column_formatting_by_asset_type
if library_asset?
update_columns_formatting(%w[library_type insert_size_from insert_size_to], REQUIRED_COLUMNS_CONDITIONAL_FORMATS)
else
@columns
end
end
I would prefer a different approach, as this adds a new place where columns are configured - it's all quite neat at the moment, in
columns.yml.Can't find the words to explain this properly, but I also don't think that 'by asset type' will turn out to be a common thing to control the requiredness.
Another thing we can do is, if we want a column to behave differently in one manifest type vs another, we make a new column definition for it - e.g. see
collected_by_for_cardinal,collected_by_for_controlsandcollected_by_for_scrna_core, which all save to the 'collected_by' column, but have different configuration, to be used in different manifest types.
Thank you for pointing to right direct, this is seems to put a simpler approach!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4752 +/- ##
===========================================
+ Coverage 89.35% 89.38% +0.03%
===========================================
Files 1410 1411 +1
Lines 30355 30425 +70
===========================================
+ Hits 27123 27195 +72
+ Misses 3232 3230 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Sorry it's taken me a while to get back to this.
I’m just wondering whether there is a way to split up the app/models/sample_manifest/column_conditional_format_updater.rb file to make it as general as the name suggests, as it is currently quite specific to the library/asset code.
Maybe the library/ asset specific code could be passed in as a sort of validation, and then this file could be reused and generic, as I think its a good layer of abstract to have between the column config not being coupled with every manifest. I’m sure there is a design pattern for this which I’ll try dig out!
Equally, this works so ok if you would like to keep it as is thats fine. Maybe worth seeing what other members of the team say in code reviews. I’m happy to try help design it out if we decide to go down that route
No worries at all! I see what you mean about making column_conditional_format_updater.rb more generic. asset_type is indeed more generic than the manifest, as different manifests can share the same asset type. I agree that decoupling the column formatting logic from the manifest could be beneficial, though currently, we rely on the manifest type to generate the spreadsheet and apply formatting. I’ve kept the class general and extensible to accommodate similar future requirements, but for now, it only checks for asset_type = library. |
| # SampleManifest::Generator | ||
| class SampleManifest::ColumnConditionalFormatUpdater | ||
| # Predefined conditional formats for required fields. | ||
| REQUIRED_COLUMNS_CONDITIONAL_FORMATS = [ |
There was a problem hiding this comment.
Where is the code that makes library_type required for one manifest type but not the other?
I would prefer a different approach, as this adds a new place where columns are configured - it's all quite neat at the moment, in columns.yml.
Can't find the words to explain this properly, but I also don't think that 'by asset type' will turn out to be a common thing to control the requiredness.
Another thing we can do is, if we want a column to behave differently in one manifest type vs another, we make a new column definition for it - e.g. see collected_by_for_cardinal, collected_by_for_controls and collected_by_for_scrna_core, which all save to the 'collected_by' column, but have different configuration, to be used in different manifest types.
Thank you for the pointing out to the right direction - it makes more sense and it is more straightforward ! |
No description provided.