feat: Incorporate "Gas Scheme" feature in reaction variations table #2250

JanCBrammer · 2024-12-03T11:41:19Z

This PR addresses #2213.
See also https://docs.google.com/document/d/1Jd6V5zqRv0cfw_xAJE-oHDYzSuBMQyBBz3wQQBxRGJo/edit and https://www.chemotion.net/docs/eln/ui/elements/reactions?_highlight=gas#gas-phase-reaction-scheme.

app/api/entities/reaction_variation_entity.rb

github-actions · 2024-12-03T11:42:48Z

lib/reporter/docx/detail_reaction.rb

@@ -60,23 +60,25 @@ def variations
            'startingMaterials' => variation_materials(var, :startingMaterials),
            'reactants' => variation_materials(var, :reactants),
            'solvents' => variation_materials(var, :solvents),
-            'products' => variation_products(var),
+            'products' => variation_materials(var, :products),


Metrics/ClassLength: Class has too many lines. [626/200]

github-actions · 2024-12-03T11:42:48Z

lib/reporter/docx/detail_reaction.rb

-            (v[:aux][:isReference] ? '; Ref' : '')
-        end
-      end
+        variation[type].map do |_, vi|


Metrics/CyclomaticComplexity: Cyclomatic complexity for variation_materials is too high. [8/7]

github-actions · 2024-12-03T12:03:01Z

LCOV of commit `807bd3d` during Continuous Integration #4189

Summary coverage rate:
  lines......: 65.5% (14049 of 21453 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

github-actions · 2024-12-18T10:54:05Z

LCOV of commit `2efc953` during Continuous Integration #4258

Summary coverage rate:
  lines......: 66.7% (14339 of 21495 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

adambasha0

Hi Jan, Thanks for the feature. I posted some suggestions and potential bug that you can check later.

adambasha0 · 2024-12-18T15:02:06Z

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.

-ActiveRecord::Schema.define(version: 2024_07_26_064022) do
+ActiveRecord::Schema.define(version: 2024_11_29_093956) do


is the timestamp change needed in this PR? - updating the timestamp might seem unnecessary

Changing the timestamp is meant to ensure that we run the migration that's necessary for this PR to work.
See db/migrate/20241129093956_add_gas_materials_to_reaction_variations.rb.

adambasha0 · 2024-12-20T10:32:34Z

db/migrate/20241129093956_add_gas_materials_to_reaction_variations.rb

+      variations.each do |variation|
+        SAMPLES_TYPES.each do |key|
+          variation[key].each_value do |material|
+            material['aux'].delete('materialType')


many repeated code in this block, especially: material.delete
suggestion: combine all keys to be deleted in one array and loop over the array to delete elements in a separate method
keys_to_delete = %w[duration temperature concentration turnoverNumber turnoverFrequency]

I feel like a one-off migration is not the place to save LOC.
In this case I believe it doesn't hurt to keep the code explicit.

app/api/entities/reaction_variation_entity.rb

adambasha0 · 2024-12-20T12:02:48Z

db/migrate/20241129093956_add_gas_materials_to_reaction_variations.rb

+end
+
+class AddGasMaterialsToReactionVariations < ActiveRecord::Migration[6.1]
+  def up


I am unsure what these migrations are used for, especially the down migration. can you explain the design concept here?

The migration is modifying the variations column of the reaction table.
Note that the migration does not add or remove columns in the table.
Instead it updates the existing data in the variations column to accommodate the gas feature.
Accommodating the gas feature consists of adding new entries to materials (such as aux.gasType).
It also means some restructuring of the materials' data to make the overall data structure more uniform.
The down migration undoes all changes related to the gas feature.

how did it exist at the first place? - the gas phase feature was not there before

I think I don't understand the question. What's "it" referring to?

how come these attributes (such as: turnoverFrequency, turnoverNumber, concentration, temperature, duration, and gasType) are deleted from the reactions variations, if there are not supposed to be there at the first place?

The attributes related to the gas feature are deleted in the down migration because the down migration is reverting the up migration where they're added.

adambasha0 · 2024-12-20T12:07:35Z

db/migrate/20241129093956_add_gas_materials_to_reaction_variations.rb

+            material.delete('duration')
+            material.delete('temperature')
+            material.delete('concentration')
+            material.delete('turnoverNumber')


isn't it better to adhere to ruby's standard naming convention about using snake case and also to match the attribute names in the the gas_phase_data column in reactions_samples table?

The reaction-variations data-structure is exclusively handled on the client (JS) side, that's why I stuck with camelCase.

adambasha0 · 2024-12-20T12:20:14Z

spec/api/entities/reaction_entity_spec.rb

@@ -27,158 +27,182 @@
        variations: [{ id: '1',
                       analyses: [1, 2],
                       notes: '',
-                       products: { '47': { aux: { yield: 0,


maybe it would be nice to have factory reaction_with_variations defined in factories/reactions.rb. This would make it easier to create reactions with variations consistently

Good point, I avoided doing this so far tbh. See bf86a76.

Ok, good. Did I miss the utilization of reaction_with_variations to test the functionality of updating a value or unit (e.g., in the gas phase) and verifying the recalculation of affected material values or units? Additionally, do we have a validation mechanism in the backend to ensure the accuracy of values submitted from the client to the server before they are saved in the database?"

Did I miss the utilization of reaction_with_variations to test the functionality of updating a value or unit (e.g., in the gas phase) and verifying the recalculation of affected material values or units?

Do the tests under spec/javascripts/packs/src/apps/mydb/elements/details/reactions/variationsTab/ cover what you have in mind?

ensure the accuracy of values submitted from the client to the server before they are saved in the database?

What do you mean with "accuracy"?

adambasha0 · 2024-12-20T14:35:19Z

...cript/src/apps/mydb/elements/details/reactions/variationsTab/ReactionVariationsComponents.js

+}
+
+function MaterialFormatter({ value: cellData, colDef }) {
+  const { currentEntry, displayUnit } = colDef.entryDefs;


can you check this error? , it happens when I click on duration button: cellData undefined

Do you mean the duration entry of a gaseous product?

What data resulted in the error? For existing data, you might need to run the migration (the one that's part of this PR).

github-actions · 2025-01-09T11:42:52Z

lib/reporter/docx/detail_reaction.rb

@@ -60,23 +60,25 @@ def variations
            'startingMaterials' => variation_materials(var, :startingMaterials),
            'reactants' => variation_materials(var, :reactants),
            'solvents' => variation_materials(var, :solvents),
-            'products' => variation_products(var),
+            'products' => variation_materials(var, :products),


Metrics/ClassLength: Class has too many lines. [627/200]

github-actions · 2025-01-09T12:03:00Z

LCOV of commit `bf86a76` during Continuous Integration #4290

Summary coverage rate:
  lines......: 66.9% (14431 of 21587 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

The example of the reaction variations data structure is unnecessarily verbose and prone to be outdated.

github-actions · 2025-01-24T07:10:56Z

lib/reporter/docx/detail_reaction.rb

-            (v[:aux][:isReference] ? '; Ref' : '')
-        end
-      end
+        variation[type].map do |_, vi|


Metrics/CyclomaticComplexity: Cyclomatic complexity for variation_materials is too high. [8/7]

github-actions · 2025-01-24T07:30:13Z

LCOV of commit `90944aa` during Continuous Integration #4353

Summary coverage rate:
  lines......: 66.9% (14443 of 21581 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

github-actions · 2025-01-24T12:30:18Z

LCOV of commit `63c42a7` during Continuous Integration #4360

Summary coverage rate:
  lines......: 66.8% (14426 of 21582 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

github-actions bot reviewed Dec 3, 2024

View reviewed changes

adambasha0 self-requested a review December 13, 2024 10:41

adambasha0 assigned JanCBrammer Dec 13, 2024

adambasha0 added feature enhancement labels Dec 13, 2024

JanCBrammer linked an issue Dec 17, 2024 that may be closed by this pull request

Reaction Variations: Incorporate "Gas Scheme" feature #2213

Open

JanCBrammer force-pushed the reaction-variations-gas-phase branch from 807bd3d to 2efc953 Compare December 18, 2024 10:31

JanCBrammer force-pushed the reaction-variations-gas-phase branch from 2efc953 to c02afd0 Compare December 19, 2024 06:27

adambasha0 requested changes Dec 20, 2024

View reviewed changes

JanCBrammer force-pushed the reaction-variations-gas-phase branch from c02afd0 to bf86a76 Compare January 9, 2025 11:41

github-actions bot reviewed Jan 9, 2025

View reviewed changes

PiTrem added this to the v2.0.0 milestone Jan 21, 2025

JanCBrammer added 15 commits January 24, 2025 06:49

Add comment to give some context w.r.t. state management

794d433

Move function

10b781e

Refactor column definitions

1ba6ddd

Adapt column definitions to material's gas types

f5b7444

Move components to dedicated file

8659b68

Rename file

3b526cd

Set columnDefinitions with useReducer hook

8051c82

Remove local state reactionVariations

0ab5123

Fix formatting after rebase

04a6f46

Change editability of duration according to gas mode

5cf1141

Enable updates of column group properties

30fe099

Update columnDefinitions on gasMode change

35b5ad3

Update materials' gas type in columnDefinitions

6391731

Enable representation of gaseous materials

b03027b

Add gas parser

1465db3

JanCBrammer added 13 commits January 24, 2025 06:49

Inline function

9001262

Move functions

436a7ee

Add feedstock parser

7b95534

Test and refactor gas parser

b3077cf

Test helper functions

0f903e8

Adapt entities to gas types

c0e81d1

Adapt report generation to gas types

0953260

Shorten comment

6b9e9be

The example of the reaction variations data structure is unnecessarily verbose and prone to be outdated.

Migrate to material-specific entries

bfe7f4e

Update changelog

75f03b5

Extract predicates

6bd9721

Add factory for reaction with variations

351f8c1

Resolve merge conflicts

90944aa

JanCBrammer force-pushed the reaction-variations-gas-phase branch from bf86a76 to 90944aa Compare January 24, 2025 07:09

github-actions bot reviewed Jan 24, 2025

View reviewed changes

Show all material entries in overlay

63c42a7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Incorporate "Gas Scheme" feature in reaction variations table #2250

feat: Incorporate "Gas Scheme" feature in reaction variations table #2250

JanCBrammer commented Dec 3, 2024

github-actions bot Dec 3, 2024

github-actions bot Dec 3, 2024

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 18, 2024

adambasha0 left a comment

adambasha0 Dec 18, 2024

JanCBrammer Jan 7, 2025

adambasha0 Dec 20, 2024 •

edited

Loading

JanCBrammer Jan 9, 2025

adambasha0 Dec 20, 2024

JanCBrammer Jan 9, 2025

adambasha0 Jan 10, 2025 •

edited

Loading

JanCBrammer Jan 10, 2025

adambasha0 Jan 14, 2025

JanCBrammer Jan 14, 2025

adambasha0 Dec 20, 2024

JanCBrammer Jan 9, 2025

adambasha0 Dec 20, 2024

JanCBrammer Jan 9, 2025

adambasha0 Jan 10, 2025

JanCBrammer Jan 10, 2025 •

edited

Loading

adambasha0 Dec 20, 2024

JanCBrammer Jan 9, 2025

github-actions bot Jan 9, 2025

github-actions bot commented Jan 9, 2025

github-actions bot Jan 24, 2025

github-actions bot commented Jan 24, 2025

github-actions bot commented Jan 24, 2025

feat: Incorporate "Gas Scheme" feature in reaction variations table #2250

Are you sure you want to change the base?

feat: Incorporate "Gas Scheme" feature in reaction variations table #2250

Conversation

JanCBrammer commented Dec 3, 2024

github-actions bot Dec 3, 2024

Choose a reason for hiding this comment

github-actions bot Dec 3, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2024

LCOV of commit 807bd3d during Continuous Integration #4189

github-actions bot commented Dec 18, 2024

LCOV of commit 2efc953 during Continuous Integration #4258

adambasha0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambasha0 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambasha0 Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanCBrammer Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Jan 9, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2025

LCOV of commit bf86a76 during Continuous Integration #4290

github-actions bot Jan 24, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2025

LCOV of commit 90944aa during Continuous Integration #4353

github-actions bot commented Jan 24, 2025

LCOV of commit 63c42a7 during Continuous Integration #4360

LCOV of commit `807bd3d` during Continuous Integration #4189

LCOV of commit `2efc953` during Continuous Integration #4258

adambasha0 Dec 20, 2024 •

edited

Loading

adambasha0 Jan 10, 2025 •

edited

Loading

JanCBrammer Jan 10, 2025 •

edited

Loading

LCOV of commit `bf86a76` during Continuous Integration #4290

LCOV of commit `90944aa` during Continuous Integration #4353

LCOV of commit `63c42a7` during Continuous Integration #4360