Y24 -473 generalize multiple source identifier filtering logic

[seenanair]

Closes #1490

Changes proposed in this pull request

Added the SourceIdentifierFilterable concern, thereby generalizing the code to apply multiple source_identifiers, which can be called from both Pacbio::LibraryResource and Pacbio::RequestResource.
#1488 (review)

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

[codecov]

Codecov Report

Attention: Patch coverage is 99.25373% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.95%. Comparing base (2516504) to head (d2130ae).
Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
...esources/v1/shared/source_identifier_filterable.rb	96.55%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1508      +/-   ##
===========================================
+ Coverage    97.93%   97.95%   +0.01%     
===========================================
  Files          428      430       +2     
  Lines        13479    13584     +105     
===========================================
+ Hits         13201    13306     +105     
  Misses         278      278              

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

[stevieing]

Looks really good. With plenty of documentation.

I will have a proper look later. Are there any specific filters I can use if I run locally to test it?

[seenanair]

Looks really good. With plenty of documentation.

I will have a proper look later. Are there any specific filters I can use if I run locally to test it?

Thanks.
Yes, it is for the 'source' filter. To test it, we need to select 'Source' in the Libraries and Samples page within the 'Filter Results' panel, using multiple values of source separated by commas.

[stevieing]

personally I would use full names as there is no discernable difference from a syntactic view e.g record_ids, filtered_records. It removes ambiguity

[stevieing]

I am not sure it is the right idea to ignore this. This is a great piece of work but if you split it into functions it will be a lot more readable. Happy to go through it with you.

[seenanair]

Ok. I will see I can break it down further. 👍

[stevieing]

I suspect Rubocop would have some ideas too but because of the Metrics/BlockLength it won't make any more suggestions.

[seenanair]

I’ve broken the code into functions and modularized it using module functions. If these are changed to private class methods, RuboCop still raises concerns about the module size. Alternatively, the other option to wrap these methods into another module and include it within this concern will allow to remove 'block length' warning .

I’m not entirely convinced about the module size limits imposed by RuboCop or the need to artificially modify the code just to satisfy it. While there is definitely value in splitting functions to improve readability and modularity, the imposed limit and the way RuboCop evaluates the structure feels bit unjustified.

[stevieing]

I understand your frustration and Rubocop can be dogmatic sometimes but looking at the result I find it far easier to understand what is going on. It is a question of maintainability.

I think if we ignore Rubocop when we disagree with it we just end up with lots of stuff disabled which can defeat the purpose of it. I really struggle with long methods because I get overwhelmed and need to break it down. I would now feel far more confident modifying it because I know what is going on. The method names help me to understand. I hope that adds some context?

[stevieing]

Another thing this does for me is it helps me to identify places where we can make stuff more flexible. For example in this filter you have used plate, tube and well distinctions. I see this a lot in our code which leads me to believe that we need to do some work around labware. Having a big method can sometimes obfuscate this.

[seenanair]

Yes, absolutely. I fully agree with your point, and breaking down the structures absolutely made more sense. As you said, it is more readable, modular, and clear now.
What I was referring to was the structure it seems to be approving of. It would have been equally good, or even better, to have the new methods added as private in-class methods, but RuboCop is not happy with that because of the size. This is the only point I am not convinced about.

Though it was mostly trial and error, I learned more through this process. Thanks for your suggestions 👍

I understand your frustration and Rubocop can be dogmatic sometimes but looking at the result I find it far easier to understand what is going on. It is a question of maintainability.

I think if we ignore Rubocop when we disagree with it we just end up with lots of stuff disabled which can defeat the purpose of it. I really struggle with long methods because I get overwhelmed and need to break it down. I would now feel far more confident modifying it because I know what is going on. The method names help me to understand. I hope that adds some context?

[stevieing]

You are correct. It would have been better to have private in-class methods. Looking at what you have done I think there is a way we could reduce the size significantly by changing the structure. For example if we turned joins into a hash we could probably use map or reduce and build the filter in one go using Arel.

[seenanair]

I'm not entirely sure how to do it, may be we can do by pairing on it at sometime later after finishing y24-149, if that's ok.

[stevieing]

Sure. I was going to have a crack but not quite sure either so pairing is a good idea.

[BenTopping]

Nice work.

I think we can also use this in resources/v1/ont/request_resource.rb for source_identifier.

[BenTopping]

Should this be in shared, resources/v1/shared instead of as a model concern, similar to run_suitability?