Extend chemical associations to support sequence based macromolecules samples#2961
Extend chemical associations to support sequence based macromolecules samples#2961adambasha0 wants to merge 11 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request extends the Chemical model to support associations with sequence-based macromolecules (SBMM) in addition to the existing Sample associations. This allows chemicals to be linked to either a Sample or a SequenceBasedMacromolecule, enabling better inventory management for both entity types.
Changes:
- Added database migration and foreign key to link chemicals to sequence-based macromolecules
- Implemented mutual exclusivity validations ensuring a chemical belongs to either a sample or SBMM, not both
- Extended API endpoints to handle type parameter ('sample' vs 'SBMM') for chemical operations
- Updated frontend components to support chemical tab integration in SBMM sample details with conditional rendering
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| db/migrate/20250902_add_sequence_based_macromolecule_id_to_chemicals.rb | Migration adding sequence_based_macromolecule_id column and foreign key constraint |
| db/schema.rb | Schema updates reflecting new column and foreign key |
| app/models/chemical.rb | Added belongs_to association and validations for mutual exclusivity of parent entities |
| app/models/sequence_based_macromolecule.rb | Added has_one :chemical association |
| spec/models/chemical_spec.rb | Updated schema annotations (no new tests added) |
| app/api/chemotion/chemical_api.rb | Extended GET/PUT/POST endpoints to support type parameter and SBMM IDs |
| app/javascript/src/fetchers/ChemicalFetcher.js | Updated fetch and update methods to handle type-specific parameters |
| app/javascript/src/components/chemicals/ChemicalTab.js | Added type prop and conditional rendering for safety tab |
| app/javascript/src/apps/mydb/elements/details/sequenceBasedMacromoleculeSamples/SequenceBasedMacromoleculeSampleDetails.js | Integrated ChemicalTab in SBMM details with inventory tab and save handling |
| app/javascript/src/apps/mydb/elements/details/samples/SampleDetails.js | Added type="sample" prop to existing ChemicalTab usage |
| app/javascript/src/stores/mobx/SequenceBasedMacromoleculeSamplesStore.jsx | Added state and actions for chemical save handling in SBMM context |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/javascript/src/stores/mobx/SequenceBasedMacromoleculeSamplesStore.jsx
Outdated
Show resolved
Hide resolved
| # frozen_string_literal: true | ||
|
|
||
| # == Schema Information | ||
| # | ||
| # Table name: chemicals | ||
| # | ||
| # id :bigint not null, primary key | ||
| # cas :text | ||
| # chemical_data :jsonb | ||
| # deleted_at :datetime | ||
| # updated_at :datetime | ||
| # sample_id :integer | ||
| # id :bigint not null, primary key | ||
| # cas :text | ||
| # chemical_data :jsonb | ||
| # deleted_at :datetime | ||
| # updated_at :datetime | ||
| # sample_id :integer | ||
| # sequence_based_macromolecule_id :bigint | ||
| # | ||
| # Foreign Keys | ||
| # | ||
| # fk_rails_... (sequence_based_macromolecule_id => sequence_based_macromolecules.id) | ||
| # |
There was a problem hiding this comment.
The spec file should include tests for the new validations added to the Chemical model. Specifically, tests should be added to verify that:
- A chemical cannot have both sample_id and sequence_based_macromolecule_id set (only_one_parent validation)
- A chemical must have at least one of sample_id or sequence_based_macromolecule_id set (at_least_one_parent validation)
- A chemical can be successfully created with sequence_based_macromolecule_id instead of sample_id
These are critical business logic validations that should be tested to prevent regressions.
| params do | ||
| requires :sample_id, type: Integer, desc: 'sample id' | ||
| requires :type, type: String, desc: 'chemical type is sample or SBMM' | ||
| optional :sample_id, type: Integer, desc: 'sample id' | ||
| optional :sequence_based_macromolecule_id, type: Integer, desc: 'sequence based macromolecule id' | ||
| end |
There was a problem hiding this comment.
The API endpoints don't validate that the required parent ID is provided based on the type. When type is 'SBMM', sequence_based_macromolecule_id should be required, and when type is 'sample', sample_id should be required. Currently both are optional, which could lead to runtime errors when trying to find_by with a nil ID. Consider adding validation logic to ensure the appropriate ID is present for the given type.
There was a problem hiding this comment.
You could omit the type parameter completely and define sample_id and sequence_based_macromolecule_sample_id as mutually exclusive. See https://github.com/ruby-grape/grape?tab=readme-ov-file#mutually_exclusive (exactly_one_of is also a good alternative)
db/migrate/20250902_add_sequence_based_macromolecule_id_to_chemicals.rb
Outdated
Show resolved
Hide resolved
068c739 to
8b23b66
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
db/migrate/20260220000002_add_sequence_based_macromolecule_sample_id_to_chemicals.rb
Show resolved
Hide resolved
...lements/details/sequenceBasedMacromoleculeSamples/SequenceBasedMacromoleculeSampleDetails.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...lements/details/sequenceBasedMacromoleculeSamples/SequenceBasedMacromoleculeSampleDetails.js
Show resolved
Hide resolved
db/migrate/20251017_add_sequence_based_macromolecule_sample_id_to_chemicals.rb
Outdated
Show resolved
Hide resolved
0447cf5 to
3d54936
Compare
LCOV of commit
|
| params do | ||
| requires :sample_id, type: Integer, desc: 'sample id' | ||
| requires :type, type: String, desc: 'chemical type is sample or SBMM' | ||
| optional :sample_id, type: Integer, desc: 'sample id' | ||
| optional :sequence_based_macromolecule_id, type: Integer, desc: 'sequence based macromolecule id' | ||
| end |
There was a problem hiding this comment.
You could omit the type parameter completely and define sample_id and sequence_based_macromolecule_sample_id as mutually exclusive. See https://github.com/ruby-grape/grape?tab=readme-ov-file#mutually_exclusive (exactly_one_of is also a good alternative)
| } | ||
|
|
||
| const updateInventoryTabInCollection = (inventoryOrder) => { | ||
| if (!currentCollection || currentCollection.is_sync_to_me) return; |
There was a problem hiding this comment.
Beware, the sync_to_me mechanism is currently being removed/changed. Depending on which PR gets merged first, either you have to update the code referencing collection sharing/synching here or we have to do it in #2783
LCOV of commit
|
19b492f to
07d2055
Compare
…(SBMM) - Add SBMM foreign key to chemicals table and update models for polymorphic-like association - Enforce mutual exclusivity and presence validation for sample_id and sequence_based_macromolecule_id - Update API, fetchers, and UI components to handle both sample- and SBMM-linked chemicals - Extend state management and tests for new association logic - Enable inventory tab functionality for sequence-based macromolecule samples
…les to sequence_based_macromolecule_samples & update chemical api, chemical js fetcher, and migration files
… linkage - return 404 when updating a missing chemical - refresh detail tab ordering when tab props change - persist/hide SBMM inventory tab layout when inventory_sample is toggled - make ChemicalTab sample update callback optional and remove debug logging - consolidate migrations and add indexes for SBMM inventory/chemical relations
07d2055 to
26fa6f8
Compare
LCOV of commit
|
LCOV of commit
|
This pull request extends the Chemical model to support associations with sequence-based macromolecules (SBMM) samples in addition to the existing Sample associations. This allows chemicals to be linked to either a Sample or a SequenceBasedMacromolecule sample, enabling better inventory management for both entity types.
Changes:
rather 1-story 1-commit than sub-atomic commits
commit title is meaningful => git history search
commit description is helpful => helps the reviewer to understand the changes
code is up-to-date with the latest developments of the target branch (rebased to it or whatever) => ⏩-merge for linear history is favoured
added code is linted
tests are passing (at least locally): we still have some random test failure on CI. thinking of asking spec/examples.txt to be commited
in case the changes are visible to the end-user, video or screenshots should be added to the PR => helps with user testing
testing coverage improvement is improved.
CHANGELOG : add a bullet point on top (optional: reference to github issue/PR )
parallele PR for documentation on docusaurus if the feature/fix is tagged for a release