Override where clause functions and added helpers dir #178
Conversation
…, added helpers directory
Oferlis
requested review from
agrare,
Fryguy,
ereztheodorou,
erantzabari,
llivne and
gregoryb-at-ibm
as code owners
|
Not sure why the bot is not triggering here, but the use of |
| @@ -0,0 +1,14 @@ | |||
| module ManipulationHelper | |||
There was a problem hiding this comment.
I'm not sure this should be a top-level helper, because then it's global to the whole app...it should probably namespaced to the plugin.
app/helpers/manipulation_helper.rb
Outdated
| if storage_systems.instance_of? Array and storage_systems.length >=1 | ||
| rec_cond = "" | ||
| storage_systems.each do |i| | ||
| rec_cond = rec_cond + "#{events_table_name(assoc)}.physical_storage_id = #{i}" |
|
Please address the rubocop issues found by the bot. |
| @@ -0,0 +1,14 @@ | |||
| module ManipulationHelper | |||
| def manipulate_storage_systems(assoc, storage_systems = nil) | |||
| if storage_systems.instance_of?(Array) && storage_systems.length >= 1 | |||
There was a problem hiding this comment.
What is the storage_systems.instance_of?(Array) check actually checking for? Are you really just checking for nil?
There was a problem hiding this comment.
The purpose is to make sure storage_systems is an instance of Array so length property won't cause an error
There was a problem hiding this comment.
Right, but there are other ways of doing that like checking respond_to?(:length). But what are you seeing coming in aside from the nil? or is it only the nil?
| @@ -114,6 +114,14 @@ def count_resources(component = nil) | |||
| end | |||
| end | |||
|
|
|||
| def event_where_clause(assoc = :ems_events, storage_systems = nil) | |||
| if storage_systems && storage_systems != [''] | |||
There was a problem hiding this comment.
What would cause [''] to be an incoming value? If you can prevent this from the calling side and accept an empty array instead, then this line can be simplified to the following since .present? handle both nil and []
if storage_systems.present?
app/models/manageiq/providers/autosde/storage_manager/physical_storage.rb
Outdated
Show resolved
Hide resolved
|
Checked commits Autosde/manageiq-providers-autosde@3732a27~...e5f95ee with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint app/helpers/manipulation_helper.rb
|
|
This pull request is not mergeable. Please rebase and repush. |
|
This pull request has been automatically closed because it has not been updated for at least 3 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
In order to filter events by the selected storage system, overriding the where_clause functions was needed.
Since both models share most of the query structuring code, I added a new 'helpers' directory and a helper function that both models use.
This PR is related to another in UI repo:
ManageIQ/manageiq-ui-classic#8424
Other helpers should be added to this directory