Add Deregistration modal #1537

jamie-suse · 2023-06-16T07:35:41Z

Description

Adds the Deregistration Modal that will appear to confirm the user's intention to deregister a host.

How was this tested?

Added Jest test for component, also added Storybook for component

jamie-suse · 2023-06-16T08:47:11Z

assets/js/components/Button/Button.jsx

@@ -12,12 +12,16 @@ const getSizeClasses = (size) => {

 const getButtonClasses = (type) => {
  switch (type) {
+    case 'primary-white-fit':


I created this style and default-fit as the current Button styles use Tailwind class w-full, which stretches the buttons to take the entire width of their container. While from following the design, I need w-fit, so that the width of the button is only as big as it's contents.

There may be a nicer way to achieve this. Thoughts?

Similarly in the design docs, the Modal component's width is also w-fit instead of w-full, so that it is not stretched to the entire screen width. But currently the Modal's w-full class is overriding any classes set from above. Thoughts about this @jagabomb?

Similarly in the design docs, the Modal component's width is also w-fit instead of w-full, so that it is not stretched to the entire screen width. But currently the Modal's w-full class is overriding any classes set from above. Thoughts about this @jagabomb?

Hi @jamie-suse, I do prefer the modal that is smaller in width as it wraps the content more tightly.

arbulu89

Good job!
Let's change some few little things first

arbulu89 · 2023-06-19T06:45:35Z

assets/js/components/Button/Button.jsx

@@ -12,12 +12,16 @@ const getSizeClasses = (size) => {

 const getButtonClasses = (type) => {
  switch (type) {
+    case 'primary-white-fit':


I would put this after primary-white, to keep the "most-used" one first

arbulu89 · 2023-06-19T06:46:40Z

assets/js/components/DeregistrationModal/DeregistrationModal.jsx

+      onClose={onClose}
+    >
+      <div className="text-gray-500">
+        This will ignore all the past events collected by the agent instance


The text looks wrong.
Can you update it with the latest text proposed by @stefanotorresi ?

arbulu89 · 2023-06-19T06:51:40Z

assets/js/components/DeregistrationModal/DeregistrationModal.stories.jsx

+  },
+};
+
+export function Default({ hostName, onCleanUp, onClose }) {


Let's use the new format for declaring stories:
https://github.com/trento-project/web/blob/main/assets/js/components/DottedPagination/DottedPagination.stories.jsx#L28

https://storybook.js.org/docs/react/writing-stories/stories-for-multiple-components

Otherwise we cannot manage the args

arbulu89 · 2023-06-19T06:53:34Z

assets/js/components/DeregistrationModal/DeregistrationModal.stories.jsx

+        hostName={hostName}
+        isOpen={open}
+        onCleanUp={() => {
+          onCleanUp();


What are this specific onCleanUp and onClose used for?
Shouldn't we simply use the setDeregistered and setOpen?
Anyway, this will change once you adapt to the new format

arbulu89 · 2023-06-19T06:54:41Z

assets/js/components/DeregistrationModal/DeregistrationModal.test.jsx

+      />
+    );
+
+    expect(await screen.findByText(new RegExp(hostName))).toBeTruthy();


This RegExp looks out of place. Cannot you simply find the hostname?
There must be tons of examples in our codebase for that

This is just some information for anyone wondering:

I was using the RegExp to be able to do a partial match on the Host name, as it is part of a bigger string. If you just provide the string like that, then testing-library will only look for strings that do an exact match, which doesn't work in this case.

However I found that with the TextMatch functions, you can provide an extra argument: { exact: false } to be able to do the partial text match without a RegExp.

https://testing-library.com/docs/queries/about/#textmatch

We only do a similar thing in StatusPill.test.jsx

CDimonaco · 2023-06-19T07:47:48Z

assets/js/components/DeregistrationModal/DeregistrationModal.jsx

+  hostName,
+  isOpen = false,
+  onCleanUp,
+  onClose = () => {},


I will suggest to not use any default for the onClose, we don't have any default for onCleanUp, I don't see the reason for having the onclose defined with default, this could led to an unexpected flow

I will also suggest to use onCancel intestead of onClose, we have onCleanUp because the cta is cleanup, we have cancel as cta button, and onClose, move to onCancel for consistency

CDimonaco

Just some comments, I agree with @arbulu89 review

CDimonaco · 2023-06-19T07:49:01Z

assets/js/components/DeregistrationModal/DeregistrationModal.jsx

+  hostName,
+  isOpen = false,
+  onCleanUp,
+  onClose = () => {},


I will also suggest to use onCancel intestead of onClose, we have onCleanUp because the cta is cleanup, we have cancel as cta button, and onClose, move to onCancel for consistency

CDimonaco · 2023-06-19T07:49:55Z

assets/js/components/DeregistrationModal/DeregistrationModal.jsx

+import Button from '@components/Button';
+
+function DeregistrationModal({
+  hostName,


Let's use hostname as props name 👀

rtorrero

Nothing to add to what's already been said. Thanks @jamie-suse !

dottorblaster

Green light for now but maybe the button needs more flexibility

dottorblaster · 2023-06-19T12:27:28Z

assets/js/components/Button/Button.jsx

+    case 'primary-white-fit':
+      return 'bg-white hover:opacity-75 focus:outline-none text-jungle-green-500 w-fit transition ease-in duration-200 text-center font-semibold rounded shadow';


Maybe we should consider to remove the ws from these classNames and make the width configurable using a width prop

I think we could create a tech-debt ticket for the Button component.

But for the Modal component, I need it to be fixed now in order to adhere to the design. Should I do this refactor in a separate PR first?

Absolutely, track that and we can tackle that when we merge the deregistration branch 👍

arbulu89

Thank you!

assets/js/components/DeregistrationModal/DeregistrationModal.test.jsx

* Host deregistration events (#1245) * Host deregistration commands * Host deregistration events * Fixing module attributes in host deregistration commands * Fix deregistration commands namespace * Deregistration side effects (#1252) * Add deregistered_at to read model Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> * Handle side effects from HostDeregistered event Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> * Filter deregistered hosts from get_all_hosts Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> * Fix tests Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Add test for filtering unregistered hosts Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Host Deregistration process manager and aggregate handling (#1249) * Host aggregate handling of deregistration event * Wip Deregistration process manager * Process Managers supervisor * Add Deregistration process manager test * Fix credo and mispells * Squashed Process manager state into process manager * Routing deregistration commands to Host aggregate * Fix deregistration process manager state with deftype * Format router file * Deregistration timestamp carried from Deregistration requested command * Fix spell in process manager docstring * Fix typo in test description * Cluster and cluster host deregistration events/commands (#1274) * Cluster and cluster host deregistration events/commands * Address comments * Remove leading space * Fix Hosts query without the soft deleted hosts exclusion (#1282) * Cluster registration also for nodes not DC (#1275) * Cluster registration process happens also when the discovered node is not a DC * Address docs and testing review feedbacks * Removed useless tests from cluster_tests * Add host to cluster when a message from a dc host arrives for a cluster Co-Authored-By: Fabrizio Sestito <fabrizio.sestito@suse.com> * Add register cluster host test when the host is and is not a DC --------- Co-authored-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * Cluster deregistration process manager & aggregate changes (#1278) * Make Process Manager aware of cluster deregistration Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Handle cluster host deregistration in the cluster aggregate Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Route the DeregisterClusterHost to the cluster aggregate --------- Co-authored-by: Jamie Rodriguez <jamie.rodriguez@suse.com> * Cluster deregistered side effects (#1280) * Add deregistered_at field to Cluster read model * ClusterDeregistered event projection * Wip on clusters module, get clusters that are not soft deleted * Clusters entrypoint tests * Test broadcast of cluster deregistration in cluster projector * Removed first from request_check_execution query * Fix query parenthesis in request_clusters_checks_execution * get_all_clusters, tested for deregistered and soft deleted clusters * Handle HostRemovedFromCluster in host projection * Cluster projector reorder of project clauses * Host projector set the cluster to null when HostRemovedFromCluster * Correct typos (#1315) * Database registration changes (#1326) * Database sap system registration change The database istance is register only if the sr is disabled or with sr enabled but only primary instances * Sap systems moduledoc updated * Changed error when registering a non primary database for a sap system * Updated moduledoc of sap system domain * Refuse database registration changes The registration is refused if the sap system does not exists and the database has a secondary role * Fix sap system domain tests * Fix typos * Format * Add SAP System deregistration commands & events (#1314) Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Fix Deregistration process manager state validation * Add application instance deregistration logic (#1357) Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> * Application instances registration changes (#1358) * Add SAP System de/registration logic to Process Manager (#1356) Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Add database instance deregistration logic (#1372) Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Add SAP System deregistration side effects (#1387) Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Detect ascs/ers cluster type (#1392) * Detect ascs/ers cluster type * Update functions order to improve pattern matching * Ascs ers type frontend (#1398) * Add ascs/ers cluster type to the frontend * Improve e2e testing of the clusters view * Discover ASCS/ERS cluster SIDs (#1400) * Discover ascs/ers cluster sids * Add additional_sids to domain and side effects * Add additional_sids to cluster api schema * Handle `:database_not_registered` errors (#1405) Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Add polymorphic embed usage to deftype macro (#1411) * Properly handle multiple errors returned (#1413) * Display ASCS/ERS clusters SID in clusters view (#1403) * Show additional sids in the clusters view * Adapt e2e tests to check for sids * Order get_all_clusters output by id * Revert HeartbeatSucceeded to old name (#1429) * Add deregistration websocket events (#1424) Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Discover ASCS/ERS clusters health and initial details (#1422) * Add nodes entry to the crm payload * Add ascs/ers cluster details value objects * Implement ascs/ers cluster health discovery * Update cluster details to the polymorphic embed * Add tests to the ascs/ers cluster health discovery * Fix validate_required usage * Set default value to additional_sids in cluster events (#1441) * Cluster delta deregistration (#1386) * Add get_cluster_id_by_host_id/1 * Add delta deregistration logic * Add additional ascs/ers cluster details discovery (#1442) * Add additional ascs/ers cluster details discovery * Add comment to parse_ascs_ers_cluster_nodes * Wrap long function in do/end * Set false instead of nil in Enum.filter function * Deregistration process manager retry the rollup errors from aggregates (#1473) * Fix HostRemovedFromCluster concurrency issues (#1474) * Basic deregistration leftovers (#1461) * test * Add deregistered_at field to Database * Fix update of Application and Database when deregistering instances * Put deregistered_at timestamp instead of nil sid on database deregistration * Remove sid to nil when Sap system is deregistered * Emit sap system deregistered when the database is deregistered * Emit database deregistered event when no instances left and db is not already deregistered * Sap systems domain deregistration tests refactored * format sap system domain * Emit database deregistered event when they are instances only when the db is not registered * Removed redundant nil default to deregistered_at in database * Fix mispell in sap system tests * Add fake_sid util to sap system testing suite * Sap system does not get deregistered a second time * Changed test titles in sap system test, to remove "a second time" misleading phrase * Deregistration tombstoning rollup (#1425) * Host tombstoning on host deregistration * Add host tombstone rollup handling in stream event handler * Cluster tombstoned event and lifespan when cluster is deregistered * Cluster rollup triggered on Cluster tombstone event * Move cluster tombstoned event to cluster domain folder * Sap system aggregate emits tombstone event * Sap system aggregate tombstoning test * SapSystem tombstoned event triggers sap system rollup * Removed tombstoned events from lifespan stop mechanism in all aggregates * Formatted Cluster/Host tombstoned events * Fix logging string in stream rollup event handler * Addressing review feedbacks * Credo format * Change redirect plug to use a list of available versions (#1487) * Change redirect plug to use a list of available versions * Add sanity check for empty available api versions list * Use Keyword.get to get the opts * Enable openapi schema versioning (#1488) * Convert the open api plug into a versinable macro * Move V1 schemas to its own folder * Update controllers to used versioned openapi * Update router to pipethrough specific open api version * Disable open api cache on dev * Update controller tests to use new openapi versions * Temporarily set openapi V1 version in CI docs generation * Test paths generation in open api spec * Add endpoint to deregister a host (#1450) Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Add V2 scope to /api/clusters endpoint (#1492) * Add openapi v2 schemas * Add clusters v2 endpoint controller and view * Update pheonix router to add v2 endpoints * Adapt clusters v1 schema and view to be backward compatible * Use clusters v2 version on projector * Remove deprecated code * Use /api/v2/clusters endpoint in frontend * Improve and fix some new schema implementations * Aggregate clauses reorder (#1495) * Reorder host aggregate clauses, host_id: nil clauses compacted * Sap system clauses sap_system_id: nil, compacted * Sap system delta deregistration (#1491) * Fix cluster delta computation * Add get_application_instances_by_host_id/1 and get_database_instances_by_host_id/1 * Add delta deregistration logic * Identify sap systems ensa version (#1496) * Identify sap systems ensa version * Replace nil by no_ensa atom in EnsaVersion enum * DatabaseDeregistered side effects (#1504) * Add deregistered_at field to databases read model * Handle database deregistered side effects * Filtered deregistered databases in the usecases * Make `HostReadModel.last_heartbeat_timestamp` a virtual field (#1505) * Project removal of db/app instances to the read models (#1507) * Project removal of db/app instances to the read models * Reject commands when aggregate deregistered (#1513) * Host aggregate reject commands when the host is deregistered * Cluster aggregate reject all commands except registration when deregistered * Sap system aggregate rejects commands when deregistered * Host aggregate rejects updates when deregistered * Cluster aggregate rejects all commands when deregistered * Fix host tests * Ensa version domain (#1512) * Add ensa_version to SapSystemReadModel * Add SapSystemUpdated event and domain logic * Add and update sap system domain tests * Add SapSystemUpdated projection * Add new ensa_version field in openapi schema * Add test to check updated event is not emitted * Update migration to set default value afterwards * Create AscsErsClusterRole enum (#1520) * Host restoration (#1517) * Host restoration in Host aggregate * Host restoration projector * Addressing typo feedbacks * Move host details update after dergistered_at guard clause * Broadcast sap system updated (#1516) * Broadcast SapSystemUpdated event * Update sap systems in redux state upon update event * Cluster restoration (#1523) * Extract cluster update procedure to separate function * Cluster restoration * Cluster restored projection * Addressing review feedbacks * Addressing review feedbacks * Move sagas functions to separate files according to adr 0009 (#1531) * Update health summary endpoint (#1530) * Update health summary service and view * Update openapi schema and deprecate old fields * Polish the code and handle unclustered systems * Fix function specs * Remove fixed sid usage from db instance creation in test * Frontend instance removal on broadcast (#1522) * Add sid to instance deregistration broadcast * Remove instances from upon instance deregistration broadcast * Apply various cleanups to the database state * Use Repo.get! where possible * Improve notification message * Fix wrong test placement * Add test for instances removal notify * Fix sap system domain apply ordering issues (#1550) * Move application instances (#1554) * Add Deregistration modal (#1537) * Allow rollup on deregistered aggregates (#1551) * Complete tests on command acceptance/rejection (#1563) * Sap system restoration (#1545) * Database restoration in sap system aggregate * Database restore projection * Sap system restore domain * Sap system restore projection * Emit application instance registered or moved in restore * Removed health multi change on sap system restoration * Remove sap_system_id setting in database restored apply * Removed sid from DatabaseRestored event * Removed sid field from SapSytemRestored event --------- Co-authored-by: Carmine Di Monaco <carmine.dimonaco@suse.com> Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> Co-authored-by: Fabrizio Sestito <fabrizio.sestito@suse.com> Co-authored-by: Jamie Rodriguez <jamie.rodriguez@suse.com> Co-authored-by: Carmine Di Monaco <carmine.dimonaco@gmail.com> Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Eugen Maksymenko <eugen.maksymenko@suse.com> Co-authored-by: Jurgen Goldschmidt <jurgen.goldschmidt@suse.com>

jamie-suse added enhancement New feature or request javascript Pull requests that update Javascript code labels Jun 16, 2023

jamie-suse force-pushed the deregistration-modal branch from f53f81e to 37bc509 Compare June 16, 2023 08:29

Add Deregistration Modal

c95a968

jamie-suse force-pushed the deregistration-modal branch from 37bc509 to c95a968 Compare June 16, 2023 08:31

jamie-suse commented Jun 16, 2023

View reviewed changes

jamie-suse requested review from dottorblaster, jagabomb, arbulu89, CDimonaco and rtorrero June 16, 2023 08:49

jamie-suse marked this pull request as ready for review June 16, 2023 08:50

rtorrero force-pushed the deregistration branch 2 times, most recently from c9bf2b8 to f0f39dd Compare June 16, 2023 12:07

arbulu89 requested changes Jun 19, 2023

View reviewed changes

CDimonaco reviewed Jun 19, 2023

View reviewed changes

CDimonaco requested changes Jun 19, 2023

View reviewed changes

jamie-suse added 6 commits June 19, 2023 13:51

Reorder Button style classes

f76dafc

Use partial text match instead of RegExp in tests

b631588

Add ability for Storybook component to pass in args

e7f2305

Rename onClose prop -> onCancel

b2f4bb1

Rename hostName -> hostname

7e9a2fa

Missed one rename onClose -> onCancel from b2f4bb1

8ab314b

rtorrero approved these changes Jun 19, 2023

View reviewed changes

Change warning text

5ac0ad6

dottorblaster approved these changes Jun 19, 2023

View reviewed changes

jamie-suse requested review from CDimonaco and arbulu89 June 19, 2023 14:25

arbulu89 approved these changes Jun 21, 2023

View reviewed changes

assets/js/components/DeregistrationModal/DeregistrationModal.test.jsx Show resolved Hide resolved

CDimonaco approved these changes Jun 22, 2023

View reviewed changes

jamie-suse merged commit e98f4b5 into deregistration Jun 22, 2023

jamie-suse deleted the deregistration-modal branch June 22, 2023 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Deregistration modal #1537

Add Deregistration modal #1537

jamie-suse commented Jun 16, 2023 •

edited

Loading

jamie-suse Jun 16, 2023 •

edited

Loading

jamie-suse Jun 16, 2023

jagabomb Jun 19, 2023 •

edited by rtorrero

Loading

arbulu89 left a comment

arbulu89 Jun 19, 2023

arbulu89 Jun 19, 2023

arbulu89 Jun 19, 2023

arbulu89 Jun 19, 2023

arbulu89 Jun 19, 2023

jamie-suse Jun 19, 2023

CDimonaco Jun 19, 2023

CDimonaco Jun 19, 2023

CDimonaco left a comment

CDimonaco Jun 19, 2023

CDimonaco Jun 19, 2023

rtorrero left a comment

dottorblaster left a comment

dottorblaster Jun 19, 2023

jamie-suse Jun 19, 2023

dottorblaster Jun 19, 2023

arbulu89 left a comment

		case 'primary-white-fit':
		return 'bg-white hover:opacity-75 focus:outline-none text-jungle-green-500 w-fit transition ease-in duration-200 text-center font-semibold rounded shadow';

Add Deregistration modal #1537

Add Deregistration modal #1537

Conversation

jamie-suse commented Jun 16, 2023 • edited Loading

Description

How was this tested?

jamie-suse Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagabomb Jun 19, 2023 • edited by rtorrero Loading

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CDimonaco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtorrero left a comment

Choose a reason for hiding this comment

dottorblaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

jamie-suse commented Jun 16, 2023 •

edited

Loading

jamie-suse Jun 16, 2023 •

edited

Loading

jagabomb Jun 19, 2023 •

edited by rtorrero

Loading