-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split cell lines #1971
base: main
Are you sure you want to change the base?
Split cell lines #1971
Conversation
app/api/chemotion/cell_line_api.rb
Outdated
begin | ||
use_case = Usecases::CellLines::Split.new(cell_line_to_copy.first, @current_user, params[:collection_id]) | ||
splitted_cell_line_sample = use_case.execute! | ||
splitted_cell_line_sample.container = update_datamodel(params[:container]) if @params.has_key?("container") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/PreferredHashMethods: Use Hash#key?
instead of Hash#has_key?
.
app/api/chemotion/cell_line_api.rb
Outdated
begin | ||
use_case = Usecases::CellLines::Split.new(cell_line_to_copy.first, @current_user, params[:collection_id]) | ||
splitted_cell_line_sample = use_case.execute! | ||
splitted_cell_line_sample.container = update_datamodel(params[:container]) if @params.has_key?("container") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
app/usecases/cell_lines/copy.rb
Outdated
@collection_id = collection_id | ||
end | ||
|
||
def execute! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout/TrailingWhitespace: Trailing whitespace detected.
LCOV of commit
|
Closes #1878 |
LCOV of commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished first round of review. Will test UI next.
end | ||
end | ||
|
||
desc 'Splits a cell line' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename from "Splits a cell line" to "Split a cell line"? Then it'd be consistent with the other descriptions that use the grammatical base form of the verb, not the third-person (e.g., "Copy a cell line").
begin | ||
use_case = Usecases::CellLines::Copy.new(cell_line_to_copy.first, @current_user, params[:collection_id]) | ||
copied_cell_line_sample = use_case.execute! | ||
copied_cell_line_sample.container = update_datamodel(params[:container]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also check for presence of :container
like in the :split
endpoint (line 142)?
end | ||
end | ||
|
||
desc 'Splits a cell line' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from use_case
this endpoint duplicates the :copy
endpoint. Is drying this up worth it, or do we apply the rule of three?
@@ -151,4 +151,25 @@ export default class CellLinesFetcher { | |||
}); | |||
return promise; | |||
} | |||
|
|||
// Here better as parameter list of ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this comment could use a bit more context?
def execute! | ||
@copied_cell_line_sample = Usecases::CellLines::Copy.new(@cell_line_sample_to_copy, @current_user, | ||
@collection_id).execute! | ||
decrease_cell_line_counter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add short comment why decreasing the counter is necessary since it's not immediately obvious.
let(:container_param) do | ||
{ 'name' => 'new', | ||
'children' => | ||
[{ 'name' => 'new', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation seems off?
it 'user cell line amount was changed' do | ||
loaded_cell_line_sample_copied | ||
old_value = user.counters['celllines'] | ||
expect(user.reload.counters['celllines']).not_to be old_value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be more specific by using .to be >
instead of .not_to be
?
|
||
require 'spec_helper' | ||
|
||
RSpec.describe Usecases::CellLines::Copy do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add tests for Usecases::CellLines::Split
as well?
Are there plans to document the copy and split features over at https://www.chemotion.net/docs/eln/ui/elements/cell_lines? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm that the "Copy Cell line" and "Split Cell line" buttons work analogous to the corresponding buttons for the other elements.
However, the naming of copied cell line samples is different from other elements.
For samples and reactions, the copies get new, (seemingly) unique names.
That is not the case for cell line samples.
For the latter, irrespective of the name of the original (i.e., the copied cell line sample), the copy is called "C".
Another (potential) naming issue is that the cell line sample names aren't unique.
When I create two cell lines samples (within the same cell line), both are called "C0".
This PR implements the feature to create sub cell lines of a cell line. The inheritance is implemented by the ancestry gem as already done in samples.
The PR also includes the copy functionality which had an own PR but will be closed in favour of this one.