Fix ENSA version calculation #2222
Conversation
There was a problem hiding this comment.
Hey @jamie-suse
I think there is room for improvement in the code, beyond the logic now looks correct.
Anyway, out of that, and not asking to do changes about this, just food for thoughts.
I think that personally, I would design the code differently. I would have a function named getSapSystemsByCluster(cluster_id) or similar, which returns you all the sap systems belonging to this cluster, returning the sap system read model, and from there, you can get the needed field, in this case the ensa version.
The current code looks so tailored to the current need that those functions are not really usable anywhere else.
@arbulu89 this is a good point which might result in the opportunity of being able to expose the information about which ensa versions are there in this ascs ers cluster directly from the backend, so we do not have the same logic applied in the frontend. I would track this as debt though, because this logic might be applied at read time by composing query results or it might be projected either in cluster read model or a dedicated read model if we leverage events. |
|
Okay I will make a ticket for tracking the refactor 👍 |
lib/trento/clusters.ex
Outdated
| @@ -225,4 +230,23 @@ defmodule Trento.Clusters do | |||
| end | |||
|
|
|||
| defp maybe_request_checks_execution(error), do: error | |||
|
|
|||
| @spec get_ensa_version([String.t()]) :: EnsaVersion.t() | |||
| defp get_ensa_version(sap_system_ids) do | |||
There was a problem hiding this comment.
The name is a bit misleading for me, but I don't have any better suggestion, so...
There was a problem hiding this comment.
As far as I know we don't really have terminology for this. The best I can think of is get_aggregated_ensa_version(), what do you think?
test/trento/clusters_test.exs
Outdated
| [%Trento.Clusters.ValueObjects.AscsErsClusterSapSystem{sid: sid}] = sap_systems | ||
| test "should start a checks execution on demand for ascs_ers clusters with ENSA 1 version" do | ||
| %SapSystemReadModel{id: sap_system_id_1, sid: sid_1} = | ||
| insert(:sap_system, ensa_version: Trento.SapSystems.Enums.EnsaVersion.ensa1()) |
There was a problem hiding this comment.
You should use the aliased short version here EnsaVersion.ensa1() (and in many more places)
There was a problem hiding this comment.
This is because these tests use both Trento.SapSystems.Enums.EnsaVersion and Trento.Clusters.Enums.EnsaVersion. I'm fine with renaming the cluster's ENSA version something else like ClusterEnsaVersion or AggregatedEnsaVersion, what do you think?
jamie-suse
force-pushed
the
ensa-version-fix
branch
from
9569712 to
b4bdb1e
Compare
Faker has failed me
Description
This PR is a correction to the aggregated ENSA version calculation made in #2177
The way the relevant SAP systems and resulting ENSA version are determined is by:
additional_sidsfield for the cluster)How was this tested?
Updated unit tests to validate the new logic