Skip to content
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

Fix Incorrect RDBMS Value Display on SAP Systems Overview #2492

Merged
merged 7 commits into from
Apr 9, 2024

Conversation

EMaksy
Copy link
Member

@EMaksy EMaksy commented Apr 4, 2024

Description

This PR addresses an issue on the SAP systems overview page, where the displayed RDBMS and Tenant values are identical. The Tenant value displayed is correct, but the attached Rdbms value uses the Tenant value.

Now the displayed RDBMS reflect the database sid as in future the tenant and database sid could be different as multi tenancy support is currently work in progress.

How was this tested?

Add unit test for sap system view

@EMaksy EMaksy added javascript Pull requests that update Javascript code elixir Pull requests that update Elixir code labels Apr 4, 2024
@EMaksy EMaksy self-assigned this Apr 4, 2024
@EMaksy EMaksy force-pushed the show_correct_attached_rdbms branch 4 times, most recently from 9976ca8 to 10bb953 Compare April 4, 2024 12:51
@EMaksy EMaksy requested a review from arbulu89 April 4, 2024 13:06
@EMaksy EMaksy added the env Create an ephimeral environment for the pr branch label Apr 4, 2024
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great!
My only concern is the sap_system_registered event, which most probably is not sending the database_id in the view function, as it doesn't use the generic sap_system rendering function.
I think that if we fix that, everything else looks great

lib/trento_web/views/v1/sap_system_view.ex Outdated Show resolved Hide resolved
test/trento_web/views/v1/sap_system_view_test.exs Outdated Show resolved Hide resolved
@@ -36,6 +36,8 @@ export const sapSystemFactory = Factory.define(({ params }) => {
const id = params.id || faker.string.uuid();
const sid = params.sid || faker.string.alphanumeric(3, { casing: 'upper' });
const databaseId = params.database_id || faker.string.uuid();
const databaseSid =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it is time at some point to have a sid generator function eheh
nah, forget about it in this PR at least.
if you want, you can work afterwards on that as small tech-debt.
But it is not a big deal anyway

@arbulu89 arbulu89 added the enhancement New feature or request label Apr 4, 2024
@EMaksy EMaksy force-pushed the show_correct_attached_rdbms branch 2 times, most recently from a43ca45 to 4cfbb4a Compare April 4, 2024 15:45
@EMaksy EMaksy changed the title Fix Incorrect RDBMS Value Display on SAP Systems Overview Page Fix Incorrect RDBMS Value Display on SAP Systems Overview Apr 5, 2024
@EMaksy EMaksy force-pushed the show_correct_attached_rdbms branch from 46bff9b to 2070aac Compare April 5, 2024 16:36
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional comments.
But we are going in the correct direction

lib/trento_web/views/v1/sap_system_view.ex Outdated Show resolved Hide resolved
lib/trento_web/views/v1/sap_system_view.ex Outdated Show resolved Hide resolved
test/trento_web/views/v1/sap_system_view_test.exs Outdated Show resolved Hide resolved
test/trento_web/views/v1/sap_system_view_test.exs Outdated Show resolved Hide resolved
test/trento_web/views/v1/sap_system_view_test.exs Outdated Show resolved Hide resolved
@EMaksy EMaksy force-pushed the show_correct_attached_rdbms branch from 0e240ab to 19ea35b Compare April 8, 2024 13:32
@EMaksy EMaksy requested review from CDimonaco and rtorrero April 8, 2024 14:49
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!!

@EMaksy EMaksy marked this pull request as ready for review April 8, 2024 14:50
Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect @EMaksy, thanks!

@EMaksy EMaksy force-pushed the show_correct_attached_rdbms branch 2 times, most recently from 94dbc23 to a961cd4 Compare April 9, 2024 12:23
@EMaksy EMaksy merged commit 81e2d55 into multi-tenant Apr 9, 2024
23 of 24 checks passed
@EMaksy EMaksy deleted the show_correct_attached_rdbms branch April 9, 2024 12:39
arbulu89 pushed a commit that referenced this pull request Apr 9, 2024
* The RDBMS value in sap systems overview shows the database sid

* Remove unused attached Rdbms data from DatabasesOverview

* Add database_sid on sap system registration event

* Refactor view and tests

* Add test for sap system view render

* Address comments about test and view

* Refactor sap_system_view test
arbulu89 pushed a commit that referenced this pull request Apr 16, 2024
* The RDBMS value in sap systems overview shows the database sid

* Remove unused attached Rdbms data from DatabasesOverview

* Add database_sid on sap system registration event

* Refactor view and tests

* Add test for sap system view render

* Address comments about test and view

* Refactor sap_system_view test
arbulu89 added a commit that referenced this pull request Apr 16, 2024
* Split sap system database aggregates (#2445)

* Add new database aggregate code

* Remove database logic from sap system aggregate

* Redirect router commands to database aggregate

* Adapt deregistration process manager

* Adapt enrichment protocol

* Adapt sap system discovery policy

* Adapt remaining code alias and tests

* Adapt related e2e tests

* Update the aggregates docstring

* Leftover dosctring and pattern matching entries

* Move database events (#2452)

* Create new databases context file

* Move the database events to its folder

* Sap system deregistration/restore based on the database (#2456)

* Add relationship between database and sap systems readmodel

* Add enriched database id to RegisterApplicationInstance command

* Add database id to SapSystemRegistered event

* Sap system projector projects database association

* Changed sap system view to reflect readmodels changes

* Update test factory

* Add explicit Restore/Deregistration to SapSystem aggregate

* Add event handlers for deregistration/restore of sap systems

* mix credo

* Refactor database instances relationship in sap systems rm

* Route new sap system commands in commanded

* Add db host and tenant in restore sap system command

* Fix database restore event handler test

* Addressing feedbacks, fix testing

* Database rollup (#2469)

* Create required rollup command and events

* Update the database aggregate to use the rollup logic

* Add rollup events to database lifespan

* Handle the rollup events

* Use database rollup in deregistration process manager

* Move database projections (#2454)

* Update database read models and projections

* Adapt all the aliases and new fields

* Adapt tests to use the new database projections

* Fix dialyzer warning

* Fix conflicted aliases and tests

* Move database controller views (#2472)

* Move the database controller

* Move the database view

* Remove fields that cause errors in view rendering

* Adapt frontend to use the new database_id field (#2473)

* Adapt frontend to use the new database_id field

* Fix host details view story to show the sap instance type

* Update frontend to correlate SAP systems and databases properly (#2480)

* Update backend and fix tests

* Fix frontend links, test and update factory

* Update open api schema and fix test

* Refactor test and update database_factory

* Add database to keep reference

* Apply database health to sap system (#2474)

* Create and modify new commands and events to add database health

* Implement the domain logic

* Add new event listener to listen to db health change events

* Update the other event handler and middleware functions

* Add e2e test case

* Handle old database events (#2485)

* Add migration to remove old sap system entries

* Upcast old events to use the proper database id

* Handle old events that do not apply now

* Handle legacy rolled up event

* Add common upcaster function

* Test the events upcasting logic

* Use a macro to upcast all the related events

* Test legacy events are being ignored

* Handle database legacy events in the sap system aggregate

* Use a enrichment protocol instead of aggregate code

* Make the rollback possible in the migration

* Fix Incorrect RDBMS Value Display on SAP Systems Overview (#2492)

* The RDBMS value in sap systems overview shows the database sid

* Remove unused attached Rdbms data from DatabasesOverview

* Add database_sid on sap system registration event

* Refactor view and tests

* Add test for sap system view render

* Address comments about test and view

* Refactor sap_system_view test

* Fix get systems by cluster selector (#2499)

* Fix getSystemsByClusterHosts selector function

* Add check in e2e test

* Multi tenant discovery (#2500)

* wip

* Database aggregate support for multiple tenants

* Sap system policy support for multi tenant discovery

* removed tenant information from DatabaseInstanceRegistered event

* Database tenants on DatabaseReadModel

* Mix format

* wip

* Enrich application instance middleware with updated tenant query

* health summary service extract tenant from sap system

* HealthOverview returns database_sid as deprecated tenant field

the new field database_sid is added for clarity and the frontend is
changed to reflect the new api field

* Fix database view and controller

* mix credo

* fix mispell

* Npm run format

* Addressing review feedbacks

* Add upcasting regression e2e test to the CI (#2504)

* Add upcasting regression e2e test to the CI

* Use gh-action for restore and switch to plain sql

* Pin ubuntu container to 22.04

* Remove fabriziosestito and add janvhs in the maintainers

* Revert wrongly changed path (#2509)

* Revert wrongly changed path

* Remove newline

* Remove sid duplication from view (#2516)

* Remove sid duplication from view

* Filter by id instead of sid

* Fix state mockup and move import

* Apply cosmetics

* Add photofinish scenario for multi-tenant case (#2521)

* Add photofinish scenario for multi-tenant case

* Update correct agent_id in filenames

* Add photofinish scenario usage in CI

* Adjust agent version

---------

Co-authored-by: Carmine Di Monaco <carmine.dimonaco@suse.com>
Co-authored-by: Eugen Maksymenko <emaksymenko@suse.com>
Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elixir Pull requests that update Elixir code enhancement New feature or request env Create an ephimeral environment for the pr branch javascript Pull requests that update Javascript code
Development

Successfully merging this pull request may close these issues.

3 participants