-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add absent_at
field to projectors and broadcast events
#1775
Conversation
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.
Some tiny requests.
We are fine after those are fixed
@@ -97,6 +97,21 @@ defmodule TrentoWeb.V1.SapSystemView do | |||
|
|||
def render("application_instance_health_changed.json", %{health: health}), do: health | |||
|
|||
def render("instance_absent_at_changed.json", %{ |
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 would follow the same pattern we use in others and wrap the data in a instance
field, like what we do in database_instance_health_changed.json
and database_instance_system_replication_changed.json"
@@ -97,6 +97,21 @@ defmodule TrentoWeb.V1.SapSystemView do | |||
|
|||
def render("application_instance_health_changed.json", %{health: health}), do: health | |||
|
|||
def render("instance_absent_at_changed.json", %{ |
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.
Let's put this function at the end, just after def render("instance_deregistered.json", %{
@@ -295,6 +297,67 @@ defmodule Trento.DatabaseProjectorTest do | |||
1000 | |||
end | |||
|
|||
test "should broadcast database_instance_absent_at_changed when DatabaseInstanceMarkedAbsent event is received" 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.
The main things under test is the projection, not the broadcasting.
Let's change the test text to:
should update the absent_at field on DatabaseInstanceMarkedAbsent event is received
.
The same for others
instance_number: instance_number, | ||
host_id: host_id, | ||
sid: sid | ||
} = insert(:database_instance_without_host) |
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.
Let's put some value to absent_at
first:
insert(:database_instance_without_host, absent_at: DateTime.utc_now())
@@ -213,6 +215,85 @@ defmodule Trento.SapSystemProjectorTest do | |||
) | |||
end | |||
|
|||
test "should broadcast application_instance_absent_at_changed when ApplicationInstanceMarkedAbsent event is received" do | |||
insert(:sap_system, id: sap_system_id = Faker.UUID.v4()) |
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.
%{id: sap_system_id} = insert(:sap_system)
|
||
ProjectorTestHelper.project(SapSystemProjector, event, "sap_system_projector") | ||
|
||
%{ |
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.
This doesn't make sense. You already get this information from the factory, you don't need to re-query.
You can "copy/paste" what you did in the database projection tests
@@ -213,6 +215,85 @@ defmodule Trento.SapSystemProjectorTest do | |||
) | |||
end | |||
|
|||
test "should broadcast application_instance_absent_at_changed when ApplicationInstanceMarkedAbsent event is received" do | |||
insert(:sap_system, id: sap_system_id = Faker.UUID.v4()) | |||
event = build(:application_instance_registered_event, sap_system_id: sap_system_id) |
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.
You don't need this event, right?
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.
Great! Just a tiny typo on the test descriptions.
You can merge without a new review
@@ -295,6 +297,67 @@ defmodule Trento.DatabaseProjectorTest do | |||
1000 | |||
end | |||
|
|||
test "should update the absent_at field on DatabaseInstanceMarkedAbsent event is received" 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.
it looks like we should replace on
by when
, right?
Description
This change makes the SAP system and database projectors listen to the
*MarkedAsAbsent
/*MarkedAsPresent
events, whereupon they update the read models and broadcast this change through the websocket channel as events namedapplication_instance_absent_at_changed
/database_instance_absent_at_changed
How was this tested?
Added unit tests for the new behaviour in the projectors