Extend chemical associations to support sequence based macromolecules#36
Conversation
* Add search logic for sbmms * Fix some search fields * Fixes and search result list for sbmm * Fixes for short label with more infos, store * Fix rubocop
* Add sbmm and sbmm sample to suggestion / all elements search * Add specs for sbmm samples search * Fix rubocop * Add joins to sbmm sample search queries
…(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
adambasha0
changed the base branch from
main
to
sequence-based-macromolecules
Chemotion.dev.36.webm |
| } | ||
| }, [alertRef.current]); | ||
|
|
||
| const sbmmInventoryTab = (ind) => ( |
There was a problem hiding this comment.
Please rename the variable ind to eventKey to improve readability.
| editChemical(value) { | ||
| self.isChemicalEdited = value; | ||
| }, | ||
| saveSampleOrInventory(closeView) { |
There was a problem hiding this comment.
Is this function (saveSampleOrInventory) used anywhere? And even if it is, the first part of the if is without function, as it only sets the saveInventoryAction Field to true if it is already true.
| has_logidze | ||
| acts_as_paranoid | ||
| belongs_to :sample | ||
| belongs_to :sample, optional: true |
There was a problem hiding this comment.
It might be easier to create a polymorphic relationship instead of managing an XOR between Sample and SBMM.
Especially when we add more possible relation candidates in the future.
belongs_to :something, polymorphic: true
When properly set up with a migration that adds the fields something_id and something_type in the DB, this allows a lot more flexibility.
There was a problem hiding this comment.
It is not planned to add more relations in the near future. The polymorphic approach would have other cons that need to be dealt with, like the possibility of orphaned record. It has also more migration complexity because it would require migrating existing data from sample_id to something else (say: chemical_owner_id), which I do not think should be done in this PR. We could think about it though if a lot more relation candidates are needed. But if you want, you can do it ofc :)
| type, | ||
| }; | ||
| if (type === 'SBMM') { | ||
| params.sequence_based_macromolecule_id = sample.id; |
There was a problem hiding this comment.
Please note that the SBMM is like the molecule, while an SBMM Sample would be the counterpart to the regular sample.
So I think you might have to change your code to reflect that. Everything in the API and the fetchers should always be sequence_based_macromolecule_sample_id
app/api/chemotion/chemical_api.rb
Outdated
| requires :type, type: String, desc: 'chemical type is sample or SBMM' | ||
| optional :cas, type: String, desc: 'cas number' | ||
| optional :sample_id, type: Integer | ||
| optional :sequence_based_macromolecule_id, type: Integer |
There was a problem hiding this comment.
You are not actually using the SBMM id here, but the SBMM-Sample id. Please update the code in all related places (fetchers, JS etc) to avoid confusion.
See below for further explanation (TL;DR: SBMM is the Molecule, SBMM-Sample is the Sample)
beque
force-pushed
the
sequence-based-macromolecules
branch
from
056974c to
cd31213
Compare
…les to sequence_based_macromolecule_samples & update chemical api, chemical js fetcher, and migration files
beque
force-pushed
the
sequence-based-macromolecules
branch
2 times, most recently
from
20683b4 to
a8de438
Compare
|
@adambasha0 the SBMM branch has been merged into main last year. Would you mind to set your PR against complat main? |
|
3 participants
Aim
This pull request extends the chemical association model to support chemicals linked to either samples or sequence-based macromolecule Samples (SBMM Sample). The goal is to enable full CRUD and UI support for chemicals associated with SBMM Samples, while maintaining backward compatibility for sample-linked chemicals.
Implementation
Database:
Added sequence_based_macromolecule_id to the chemicals table (with a foreign key).
Enforced mutual exclusivity and presence validation for sample_id and sequence_based_macromolecule_id in the Chemical model.
Backend:
Updated the Chemical model to allow association with either a sample or a SBMM Sample.
Updated API endpoints to accept and process both types of associations.
Added/updated model validations to ensure a chemical belongs to exactly one parent (sample or SBMM Sample).
Frontend:
Updated ChemicalTab, fetchers, and detail components to support both sample- and SBMM-linked chemicals.
Added a type prop to distinguish between sample and SBMM Sample context.
Enabled the inventory tab for sequence-based macromolecule samples.
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