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

✨ Applications: Update Notification and refactors update/create modal #1207

Merged
merged 3 commits into from
Jul 27, 2023
Merged

✨ Applications: Update Notification and refactors update/create modal #1207

merged 3 commits into from
Jul 27, 2023

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Jul 26, 2023

While resolving the related notification issues (see associated MTA-1024) this refactors create/update modal.

Includes following changes :

  • Push edit/create notifications down to ApplicationForm
  • Use onClose to replace onSave and onCancel
  • Consolidate New/Edit Modal duplication

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

@gildub gildub self-assigned this Jul 26, 2023
@gildub gildub changed the title ✨ Applications: Update Notification and refactors update/create modal and its queries ✨ Applications: Update Notification and refactors update/create modal Jul 26, 2023
@gildub gildub changed the title ✨ Applications: Update Notification and refactors update/create modal ✨[WIP] Applications: Update Notification and refactors update/create modal Jul 26, 2023
@gildub gildub changed the title ✨[WIP] Applications: Update Notification and refactors update/create modal ✨Applications: Update Notification and refactors update/create modal Jul 26, 2023
@gildub gildub changed the title ✨Applications: Update Notification and refactors update/create modal ✨ Applications: Update Notification and refactors update/create modal Jul 26, 2023
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Patch coverage: 40.00% and no project coverage change.

Comparison is base (97005b7) 44.10% compared to head (b4c2e1c) 44.10%.
Report is 1 commits behind head on main.

❗ Current head b4c2e1c differs from pull request most recent head e81316f. Consider uploading reports for the commit e81316f to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1207   +/-   ##
=======================================
  Coverage   44.10%   44.10%           
=======================================
  Files         177      177           
  Lines        4501     4507    +6     
  Branches     1007     1007           
=======================================
+ Hits         1985     1988    +3     
- Misses       2505     2508    +3     
  Partials       11       11           
Flag Coverage Δ
unitests 44.10% <40.00%> (+<0.01%) ⬆️

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

Files Changed Coverage Δ
client/src/app/queries/applications.ts 49.01% <0.00%> (ø)
...s/components/application-form/application-form.tsx 55.81% <50.00%> (-0.17%) ⬇️

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

const onCreateUpdateApplicationSuccess = (response: any) => {
onSaved(response);
const onCreateUpdateApplicationSuccess = (
response: AxiosResponse<Application>
Copy link
Member

@ibolton336 ibolton336 Jul 26, 2023

Choose a reason for hiding this comment

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

I think we could use our custom useUpdateApplicationMutation & useCreateApplicationMutation hooks to 1) narrow this type to something like "appName: string" & 2) Pass the name from the mutation function variable object param to the success handler.

e.g.

export const useUpdateApplicationMutation = (
  onSuccess: (appName: string) => void,
  onError: (err: AxiosError) => void
) => {
  const queryClient = useQueryClient();
  return useMutation({
    mutationFn: ({ application }: { application: Application }) =>
      updateApplication(application),

    onSuccess: (_, vars) => {
      onSuccess(vars.application.name);
      queryClient.invalidateQueries([ApplicationsQueryKey]);
    },
    onError: onError,
  });
};

& then in the success handler:

  const onCreateUpdateApplicationSuccess = (appName: string) => {
    if (application) {
      pushNotification({
        title: t("toastr.success.createWhat", {
          type: t("terms.application"),
          what: appName,
        }),
        variant: "success",
      });
    } else {
  title: t("toastr.success.saveWhat", {
          type: t("terms.application"),
          what: appName,
        }),
        variant: "success",
    }
    onClose();
  };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ibolton336, generalizing this approach is, I believe out of scope of this PR, please let's discuss how we are going to do that here : #1214.

Copy link
Member

@ibolton336 ibolton336 Jul 27, 2023

Choose a reason for hiding this comment

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

Np - we can address the missing entity name in the update notification later. However, I think it would make more sense to have two separate handlers for create / update here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ibolton336, I agree it makes sense to have separate handlers. Let me change that.

Copy link
Member

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

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

See inline.

Signed-off-by: Gilles Dubreuil <[email protected]>
Signed-off-by: Gilles Dubreuil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants