Y24-402 - Specify 'no. pools' rather than 'no. samples per pool' in submission #4467

dasunpubudumal · 2024-10-28T14:09:03Z

Changes proposed in this pull request

Instructions for Reviewers

[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to main]
- Check story numbers included
- Check for debug code
- Check version

…fy-no-pools-rather-than-no-samples-per-pool-in-submission

codecov · 2024-10-30T14:48:34Z

Codecov Report

Attention: Patch coverage is 96.92308% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.21%. Comparing base (2f7cdbe) to head (9ef7c10).

Files with missing lines	Patch %	Lines
.../models/submission/validations_by_template_name.rb	96.87%	2 Missing ⚠️

Additional details and impacted files

@@                          Coverage Diff                          @@
##           migrating-to-number-of-pools-epic    #4467      +/-   ##
=====================================================================
- Coverage                              89.22%   89.21%   -0.02%     
=====================================================================
  Files                                   1392     1392              
  Lines                                  29830    29887      +57     
=====================================================================
+ Hits                                   26616    26663      +47     
- Misses                                  3214     3224      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrewsparkes

Minor comments. Just check that conditional formatting one where its still 5 and 25 and I can approve

app/models/submission/validations_by_template_name.rb

config/bulk_submission_excel/conditional_formattings.yml

KatyTaylor · 2024-11-08T09:11:39Z

config/bulk_submission_excel/columns.yml

-      promptTitle: "Samples per Pool"
-      prompt: "The requested number of samples per pool (between 5 and 25 inclusive)"
+      promptTitle: "Number of pools"
+      prompt: "The requested number pools (between 2 and 8 inclusive)"


Should be 1 and 8.
Was wrong in the story originally - my bad!

Thanks for updating! conditional_formattings.yml still has the old range.

andrewsparkes

I left some comments and questions.
Think it needs better test coverage too.
Can discuss!

andrewsparkes · 2024-11-26T14:15:41Z

app/models/submission/validations_by_template_name.rb

    end
  end

+  def apply_number_of_samples_per_pool_validation
+    # Creates groups of rows based on the study and project name (pool_number.e., study-project combinations)


what's pool_number.e. ?

andrewsparkes · 2024-11-26T14:17:42Z

app/models/submission/validations_by_template_name.rb

@@ -45,13 +62,11 @@ def apply_additional_validations_by_template_name
  #
  # @param column_header [String] The header of the column to validate.
  # @return [void]
-  # rubocop:disable Metrics/MethodLength,Metrics/AbcSize
+  # rubocop:disable Metrics/MethodLength


I keep running fowl of this rubocop, I wonder if we should increase the allowed length? it's too short

andrewsparkes · 2024-11-26T14:19:30Z

app/models/submission/validations_by_template_name.rb

-  # rubocop:enable Metrics/MethodLength,Metrics/AbcSize
+  # rubocop:enable Metrics/MethodLength
+
+  def validate_samples_per_pool_for_labware


add method comments for new methods.
codepilot is pretty good at doing most of it if you highlight the method and ask it to 'add a method comment'

andrewsparkes · 2024-11-26T14:21:05Z

app/models/submission/validations_by_template_name.rb

+    barcodes = rows.pluck(headers.index(HEADER_BARCODE))
+    well_locations = rows.pluck(headers.index(HEADER_PLATE_WELLS))
+
+    if plate?(barcodes, well_locations)


if it's not a plate or tube? e.g. we have tube racks now.
raise exception maybe

plus add test for tube

andrewsparkes · 2024-11-26T14:23:15Z

app/models/submission/validations_by_template_name.rb

+
+  # rubocop:disable Metrics/AbcSize
+  # rubocop:disable Metrics/MethodLength
+  def validate_for_tubes(barcodes, rows)


refactor this? rubocop clearly not happy
ask codepilot for ideas?
plus add more test coverage

andrewsparkes · 2024-11-26T14:29:16Z

app/models/submission/validations_by_template_name.rb

+  end
+
+  # Checks if the asset is either a tube or a plate.
+  def valid_labware?(barcodes, well_locations)


I don't like the look of this as a way to validate a labware. Where's this used in the code (search doesn't find it in your PR changes)? and why is there no test?
e.g. a TubeRack is a labware but won't have wells but is not a tube.
Maybe look for a valid sti_type? plate or tube if that's the only 2 types you want to allow?

This was probably something I had in a previous iteration and forgot to remove.

andrewsparkes · 2024-11-26T14:29:37Z

app/models/submission/validations_by_template_name.rb

+      samples_per_pool += 1 if pool_number < remainder
+      next unless samples_per_pool > SAMPLES_PER_POOL[:max] || samples_per_pool < SAMPLES_PER_POOL[:min]
+
+      errors.add(


andrewsparkes · 2024-11-26T14:30:54Z

config/bulk_submission_excel/columns.yml

  unlocked: true
  type: :integer
  validation:
    options:
      type: :whole
      operator: :between
-      formula1: "5"
-      formula2: "25"
+      formula1: "1"


just checking 1 is the minimum, I thought it was 2

you have 2-8 in the ranges below

…fy-no-pools-rather-than-no-samples-per-pool-in-submission

Adding columns and conditional formatting

9a47613

dasunpubudumal linked an issue Oct 28, 2024 that may be closed by this pull request

Y24-402 - Specify 'no. pools' rather than 'no. samples per pool' in submission #4423

Open

5 tasks

dasunpubudumal marked this pull request as draft October 28, 2024 14:09

dasunpubudumal added 5 commits October 29, 2024 11:31

Removing samples_per_pool column

ae562ca

Adding study-project uniqueness validation

f4ea40a

Merge remote-tracking branch 'origin/develop' into 4423-y24-402-speci…

6054967

…fy-no-pools-rather-than-no-samples-per-pool-in-submission

Refactoring with the column name change

73b3530

Refactoring with the column name change

273d79a

dasunpubudumal added 16 commits October 31, 2024 11:18

Refactoring with the column name change

7b9d50e

New tests

7ced469

More tests

af5ef2a

Making prettier happy

0883d04

Making prettier happy

5f0691f

Fixing linters

cb26850

Fixing linters

04c49ae

Refactoring for tubes and plates

8887732

Refactoring for tubes and plates

5aa28cb

Reverting some changes that aren't required

7aba186

Replacing no of sample per pool with no of pools

8223742

Adding validation logic for tubes

ce715de

Adding validation logic for tubes

217a62f

Changing column name

89a86bd

Refactoring the existing bulk submission tests to include the new ones

f4649f8

Adding number of pools to the UI

7058a7c

dasunpubudumal marked this pull request as ready for review November 6, 2024 17:13

Removing redundant column validations

6b25c81

andrewsparkes reviewed Nov 7, 2024

View reviewed changes

dasunpubudumal added 2 commits November 7, 2024 14:19

Refactoring for reviews

0f93a2d

Refactoring for reviews

3ddc5b9

Linting

d47250d

dasunpubudumal requested review from KatyTaylor and BenTopping November 7, 2024 16:28

KatyTaylor reviewed Nov 8, 2024

View reviewed changes

Update number of pools validation range in columns.yml

c2c5e20

dasunpubudumal closed this Nov 15, 2024

dasunpubudumal reopened this Nov 15, 2024

dasunpubudumal changed the base branch from develop to migrating-to-number-of-pools-epic November 15, 2024 12:12

andrewsparkes requested changes Nov 26, 2024

View reviewed changes

dasunpubudumal added 13 commits November 26, 2024 14:41

Merge remote-tracking branch 'origin/develop' into 4423-y24-402-speci…

fa10bc9

…fy-no-pools-rather-than-no-samples-per-pool-in-submission

Raising exception for invalid labware types

bdd639d

[skip ci] Update comment

eaacc48

Fixing rubocop issue

933afad

Adding more tests

b881745

Adding method docs

682785c

Refactor validation logic

dfce479

Adding tests for tubes

8c75444

Adding a test for invalid scenario

ff64214

[skip ci] refactoring some comments in tests

fea780f

Updating tests to add shared examples for invalid scenarios

3375001

[skip ci] Updating tests to add shared examples for invalid scenarios

d58bd71

Changing shared example names

e409e5e

dasunpubudumal requested a review from andrewsparkes November 28, 2024 08:58

dasunpubudumal added 2 commits November 28, 2024 09:04

Refactoring long methods

e44c0ce

Refactoring long methods

9ef7c10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Y24-402 - Specify 'no. pools' rather than 'no. samples per pool' in submission #4467

Y24-402 - Specify 'no. pools' rather than 'no. samples per pool' in submission #4467

dasunpubudumal commented Oct 28, 2024

codecov bot commented Oct 30, 2024 •

edited

Loading

andrewsparkes left a comment

KatyTaylor Nov 8, 2024

KatyTaylor Nov 27, 2024

andrewsparkes left a comment

andrewsparkes Nov 26, 2024

andrewsparkes Nov 26, 2024

andrewsparkes Nov 26, 2024

andrewsparkes Nov 26, 2024

andrewsparkes Nov 26, 2024

andrewsparkes Nov 26, 2024

andrewsparkes Nov 26, 2024

dasunpubudumal Nov 27, 2024

andrewsparkes Nov 26, 2024

andrewsparkes Nov 26, 2024

andrewsparkes Nov 26, 2024

Y24-402 - Specify 'no. pools' rather than 'no. samples per pool' in submission #4467

Are you sure you want to change the base?

Y24-402 - Specify 'no. pools' rather than 'no. samples per pool' in submission #4467

Conversation

dasunpubudumal commented Oct 28, 2024

Changes proposed in this pull request

Instructions for Reviewers

codecov bot commented Oct 30, 2024 • edited Loading

Codecov Report

andrewsparkes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsparkes left a 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

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

codecov bot commented Oct 30, 2024 •

edited

Loading