✨ BusinessServices: Update Notification and refactors update/create modal and queries #1210

gildub · 2023-07-26T16:42:09Z

While resolving the related notification issues (see associated MTA-1024) this also refactors create/update modal and tries to bring more consistency.

Includes following changes :

Push edit/create notifications down to BusinessServiceForm
Use onClose to replace onSave and onCancel
Replace REST functions to use axios instead of deprecated APIClient().
Use React Query mutations instead of legacy queries
Consolidate New/Edit Modal duplication

Partially address https://issues.redhat.com/browse/MTA-1024

codecov · 2023-07-26T16:50:08Z

Codecov Report

Patch coverage: 33.33% and project coverage change: -0.08% ⚠️

Comparison is base (97005b7) 44.10% compared to head (b10c025) 44.02%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1210      +/-   ##
==========================================
- Coverage   44.10%   44.02%   -0.08%     
==========================================
  Files         177      177              
  Lines        4501     4511      +10     
  Branches     1007     1007              
==========================================
+ Hits         1985     1986       +1     
- Misses       2505     2514       +9     
  Partials       11       11

Flag	Coverage Δ
unitests	`44.02% <33.33%> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
client/src/app/queries/businessservices.ts	`30.76% <20.00%> (-11.54%)`	⬇️
client/src/app/api/rest.ts	`54.46% <60.00%> (+0.46%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ibolton336 · 2023-07-26T20:21:44Z

client/src/app/api/rest.ts

+): Promise<BusinessService> => axios.delete(`${BUSINESS_SERVICES}/${id}`);
+
+export const createBusinessService = (
+  obj: New<BusinessService>


I would like to see a more descriptive param name here.

@ibolton336, the entire rest.ts file is using obj as parameter for passing the payload.
I don't see the gain to explicitly name the payload but that might be opinionated.
And if we do that then we're going to be inconsistent across the file.

ibolton336 · 2023-07-26T20:24:13Z

client/src/app/pages/controls/business-services/components/business-service-form.tsx

+    onClose();
+  };
+
+  const onUpdateBusinessServiceSuccess = (


We should be using a combined success handler here like we are for applications. Or at least we should agree on a style and be consistent across slices of the app.

@ibolton336,

The Applications case is actually more an exception.

To have combined success handlers for both create/edit means both needs to provide the name of the modified resource which I believe is out of scope. To discuss the matter, please seee #1214.

Thx. I read the issue but I still don't see why the application handler is a special case. Maybe it would just be easier to move the on create / on update success handlers to their own functions for the applications? Just for consistency.

@ibolton336,
you're right, the Application isn't a special case, I confused it with the MigrationTargets where we have to pass the reference to scroll down the page.
So revisiting this, we want to be consistent and have separate handlers, I'm changing the handlers in #1207.

We might eventually revisit that, consistently of course, or not once we decide to pass along the object name to the save/edit.

ibolton336 · 2023-07-26T20:24:59Z

client/src/app/queries/businessservices.ts

+};
+
+export const useUpdateBusinessServiceMutation = (
+  onSuccess: (res: any) => void,


Suggested change

onSuccess: (res: any) => void,

onSuccess: (businessServiceName: string) => void,

@ibolton336, all queries are using res as parameter for onSuccess().
Therefore I don't see the value of such change and the gain in the meaning.
I'm not necessarily against it meanwhile if we decide such change then we'd have to make it consistent across the all queries.

Np - My main gripe here was the use of the 'any' type. Seems like since we are working on tech debt, it would be a good time to address. And by narrowing what we are passing from the response ( we aren't using the entire response), we can reduce complexity in the handlers which are using this param up the chain.

@ibolton336, you had a good point with the any type.
This isn't the case anymore as there is no need to pass anything for now (see update) until we eventually address that later if we decide to pass object name to update queries.

ibolton336 · 2023-07-26T20:26:44Z

client/src/app/queries/businessservices.ts

+    onSuccess: (res) => {
+      onSuccess(res);


Suggested change

onSuccess: (res) => {

onSuccess(res);

onSuccess: (_, vars) => {

onSuccess(vars.businessService.name);

@ibolton336,

This seems to be out of scope, please see why here #1214 to following-up.

I don't mind not doing this. Just seemed like an easy solution that would help standardize notifications with more info.

@ibolton336, I agree but I would like to have an agreed overall approach to address this across the board.

client/src/app/pages/controls/business-services/business-services.tsx

client/src/app/queries/businessservices.ts

ibolton336 · 2023-07-27T12:55:15Z

client/src/app/pages/controls/business-services/components/business-service-form.tsx

@@ -35,7 +35,7 @@ export interface FormValues {
 }

 export interface BusinessServiceFormProps {
-  businessService?: BusinessService;
+  businessService: BusinessService | null;


This seems to go against what we have done across the app for optional props. Having the prop be undefined makes sense as it is omitted for new businessServices & doesn't need to be explicitly set.

@ibolton336, I thought about that but this isn't an optional prop anymore as it was before.
The Form requires the businessService prop.
The modal state value provides either the object value itself or null:

const businessServiceToUpdate = createUpdateModalState !== "create" ? createUpdateModalState : null;

Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>

gildub self-assigned this Jul 26, 2023

gildub mentioned this pull request Jul 26, 2023

✨ [DO NOT MERGE] Fix edit notifications along simplification and consistency refactors #1199

Closed

ibolton336 reviewed Jul 26, 2023

View reviewed changes

ibolton336 requested changes Jul 26, 2023

View reviewed changes

ibolton336 reviewed Jul 26, 2023

View reviewed changes

client/src/app/pages/controls/business-services/business-services.tsx Outdated Show resolved Hide resolved

ibolton336 reviewed Jul 26, 2023

View reviewed changes

client/src/app/queries/businessservices.ts Show resolved Hide resolved

gildub requested a review from ibolton336 July 27, 2023 11:18

ibolton336 reviewed Jul 27, 2023

View reviewed changes

gildub requested a review from ibolton336 July 27, 2023 13:45

gildub added 4 commits July 27, 2023 15:46

BusinessServiceForm to use React Query

5e49a61

Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>

cleanup

798dc2b

Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>

Replace <Array> with [] type for consistency

0fbd2d5

Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>

Cleaner params

b10c025

Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>

ibolton336 approved these changes Jul 27, 2023

View reviewed changes

gildub merged commit 864658d into konveyor:main Jul 27, 2023

gildub deleted the notification-modal-refactor-businessService branch July 27, 2023 13:55

This was referenced Nov 22, 2023

operator konveyor-operator (0.3.0-beta.3) k8s-operatorhub/community-operators#3570

Merged

operator konveyor-operator (0.3.0-beta.3) redhat-openshift-ecosystem/community-operators-prod#3652

Merged

This was referenced Jan 23, 2024

operator konveyor-operator (0.3.0) k8s-operatorhub/community-operators#3793

Merged

operator konveyor-operator (0.3.0) redhat-openshift-ecosystem/community-operators-prod#3927

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ BusinessServices: Update Notification and refactors update/create modal and queries #1210

✨ BusinessServices: Update Notification and refactors update/create modal and queries #1210

gildub commented Jul 26, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

ibolton336 Jul 26, 2023

gildub Jul 27, 2023

ibolton336 Jul 26, 2023

gildub Jul 27, 2023 •

edited

Loading

ibolton336 Jul 27, 2023

gildub Jul 27, 2023

ibolton336 Jul 26, 2023

gildub Jul 27, 2023

ibolton336 Jul 27, 2023 •

edited

Loading

gildub Jul 27, 2023

ibolton336 Jul 26, 2023

gildub Jul 27, 2023

ibolton336 Jul 27, 2023

gildub Jul 27, 2023

ibolton336 Jul 27, 2023

gildub Jul 27, 2023

	onSuccess: (res: any) => void,
	onSuccess: (businessServiceName: string) => void,

✨ BusinessServices: Update Notification and refactors update/create modal and queries #1210

✨ BusinessServices: Update Notification and refactors update/create modal and queries #1210

Conversation

gildub commented Jul 26, 2023 • edited Loading

codecov bot commented Jul 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gildub Jul 27, 2023 • edited Loading

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

ibolton336 Jul 27, 2023 • edited Loading

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

gildub commented Jul 26, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

gildub Jul 27, 2023 •

edited

Loading

ibolton336 Jul 27, 2023 •

edited

Loading