refactor(coral): Replace hacky notification solution in ProfileDropdown

[dotarjun]

Linked issue

Resolves: #1810

What kind of change does this PR introduce?

• Bug fix
• New feature
• Refactor
• Docs update
• CI update

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes / features)

[aindriu-aiven]

Hey @dotarjun thanks a million for opening this PR! Just wanted to let you know that the Klaw teams experts and reviewers in this area are off today and tomorrow but will review it as soon as they are back :) Thanks a million!

[mathieu-anderson]

Thank you for the PR! Reviewing it now. Regarding this requirement of a PR:

The commit message follows our guidelines

This also applies to the title of the PR, as we squash all commits when merging to main, the title of the PR becomes the commit message ^^

[mathieu-anderson]

The feature works as expected, however the tests are failing: https://github.com/Aiven-Open/klaw/actions/runs/6371740002/job/17306861820?pr=1824#step:4:22

You may want to update the snapshots

pnpm jest --updateSnapshot

But I think there may be more tests failing.

[programmiri]

Hi! I checked the tests - the mock for useToastContext needs a little update, since it does not return one function (like useToast does) but an array with functions. I can mark the changes in the code if you want @dotarjun, but I leave it up to you in case you want to figure out for yourself.

[dotarjun]

@programmiri I think I have fixed it now

[programmiri]

Great job 🎉 I left two smoool comments :)

[programmiri]

@dotarjun saw you updated the PR, looks great! The snapshot test still needs to be updated, as you can see, the pipeline is failing. This is based in the update of the design system and the change is correct, so you can run pnpm jest --updateSnapshot or pnpm test -- -u in /coral to update the snapshots.

[dotarjun]

I get this error.

● ProfileDropdown › handles error in logout process › shows error notification to the user

    expect(jest.fn()).toHaveBeenCalledWith(...expected)

    Expected: "logout"

    Number of calls: 0

      224 |       await user.click(logout);
      225 |
    > 226 |       expect(mockDismiss).toHaveBeenCalledWith("logout");
          |                           ^
      227 |
      228 |       expect(mockToast).toHaveBeenCalledWith(
      229 |         expect.objectContaining({

      at Object.<anonymous> (src/app/layout/header/ProfileDropdown.test.tsx:226:27)

[programmiri]

@dotarjun ah, sorry I didn't notice that sooner.

In the mock for toast, you returning an object, but useToastContext needs to return an array:

jest.mock("@aivenio/aquarium", () => ({
  ...jest.requireActual("@aivenio/aquarium"),
  useToastContext: () => ([
    mockToast,
    mockDismiss,
  ]),
}));

I checked out your branch and tested it there with the array and for me, it works. Can you try that? Thanks for hanging in there 🙏

Also, your branch needs to be updated manually as it has a conflict.

[mathieu-anderson]

@dotarjun please fix the conflict so that the github actions can run ^^

[dotarjun]

The forced push was done as I accidentally pushed the changes I made for #1845 on this branch.

I cherry picked the relevant commits to the correct branch and then removed the unnecessary commits by interactive rebase.

Then I had to force push to remove the misleading commits on this branch.

[mathieu-anderson]

@dotarjun there are many CI failures regarding the TS compiler and tests, do you need assistance?

[dotarjun]

@dotarjun there are many CI failures regarding the TS compiler and tests, do you need assistance?

@mathieu-anderson Yeah. I dont understand why. I think it's because of incorrectly resolving merge conflicts for pnpm-lock.yaml.

[mathieu-anderson]

@dotarjun If you suspect this is the case, I would try:

• delete node_modules and pnpm-lock.yaml
• pull latest main
• run pnpm install

Then run pnpm run tsc (TS check) and pnpm test to verify is errors are still present.

[mathieu-anderson]

@dotarjun you may also want to address the lst issue I pointed out on this PR: #1845 (comment)

And then after it's merged you won't have to update aquarium in this PR because it will already have been updated ^^

[dotarjun]

"All checks have passed"

🎉🎉

[mathieu-anderson]

@dotarjun so now there is a new conflict because this PR was merged: #1845

Probably pull the latest main on your local branch and make it so that this branch uses aquarium 1.43.0 as well ^^

[mathieu-anderson]

Thank you!

Too hasty

[mathieu-anderson]

Was there a reason to move this in the body of the onDropdownClick function?

Because in this state, clicking Logout throws an error and does nothing, as hooks can only be called in the body of a component:

Screen.Recording.2023-10-24.at.15.36.43.mov

I think moving it back where it was before (under const authUser = useAuthContext();) should fix the issue and still preserve the behaviour we want.

[mathieu-anderson]

@dotarjun , I was going to merge but I found that there was still an oversight in the code ^^ #1824 (comment)

[mathieu-anderson]

@dotarjun I hope you don't mind but I pushed a commit to fix the broken tests, as this is the last day of Hacktoberfest and it would be nice to merge this PR today ^^

[dotarjun]

@mathieu-anderson thanks a ton. I've been quite busy with some personal obligations. It's the festive season here in India so I've been busy with that. I'll be getting back to contributing soon 🚀

I really appreciate you covering up for me 😄

[mathieu-anderson]

@dotarjun merged ^^ Thank you!