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.

Sign up for GitHub

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

Jump to bottom

[MM-57726] Convert AddIncomingWebhook from Class Component to Function Component #26992

Merged
merged 4 commits into from May 24, 2024
Merged

[MM-57726] Convert AddIncomingWebhook from Class Component to Function Component #26992

merged 4 commits into from May 24, 2024

Conversation

MeHow25
Copy link
Contributor

@MeHow25 MeHow25 commented May 11, 2024

Summary

Convert AddIncomingWebhook from Class Component to Function Component

Ticket Link

Fixes #26694
Jira https://mattermost.atlassian.net/browse/MM-57726

Release Note

NONE

@mm-cloud-bot
Copy link

@MeHow25: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@mattermost-build
Copy link
Contributor

Hello @MeHow25,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@MeHow25 MeHow25 changed the title Convert AddIncomingWebhook from Class Component to Function Component [MM-57726] Convert AddIncomingWebhook from Class Component to Function Component May 12, 2024
larkox
larkox requested changes May 13, 2024
View reviewed changes
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

Great work! One minor change, and a question.

webapp/channels/src/components/integrations/add_incoming_webhook/add_incoming_webhook.test.tsx Outdated
@@ -26,16 +63,29 @@ describe('components/integrations/AddIncomingWebhook', () => {
});

test('should have called createIncomingHook', () => {
const hook = TestHelper.getIncomingWebhookMock({
channel_id: 'channel_id',
const hook = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we remove the call to the TestHelper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This call was returning an object with some additional properties like create_at or delete_at that I assumed they are not relevant in this test. Do you think this call should stay?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I am not mistaken, it should stay, yes. The idea of this call is to mock the default values so it looks like the object you would naturally receive out in the wild. If those extra values are creating any issue in terms of types or anything, we can look a bit deeper why that is, but it is better if we use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I get it :) I've added these additional properties. Now the test is comparing whole objects, and it seems to work 👍.

webapp/channels/src/components/integrations/add_incoming_webhook/add_incoming_webhook.tsx Outdated

addIncomingHook = async (hook: IncomingWebhook) => {
this.setState({serverError: ''});
const addIncomingHook = async (hook: IncomingWebhook) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use a useCallback so we don't pass a new function on every render.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@larkox larkox requested a review from M-ZubairAhmed May 17, 2024 15:43
@M-ZubairAhmed M-ZubairAhmed added the 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review label May 20, 2024
@unified-ci-app
Copy link
Contributor

Not triggering E2E tests: this PR has 2 commit status(es) or check-runs that are not passing. Ensure all statuses aside from the E2E testing ones are green, before triggering E2E tests.

@M-ZubairAhmed
Copy link
Member

/release-note-none

@mm-cloud-bot mm-cloud-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed labels May 20, 2024
@larkox
Copy link
Contributor

larkox commented May 22, 2024

/update-branch

@yasserfaraazkhan yasserfaraazkhan added the Setup Cloud Test Server Setup an on-prem test server label May 23, 2024
@yasserfaraazkhan
Copy link
Contributor

/e2e-tests

@unified-ci-app
Copy link
Contributor

E2E test triggered successfully for PR #26992. The corresponding commit's status check will be available shortly.

@github-actions GitHub Actions
Copy link

E2E test run is starting for commit e235a8c0e8a1197a46c1449e87369f98b5370907.
You can check its progress by either:

yasserfaraazkhan
yasserfaraazkhan approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@yasserfaraazkhan yasserfaraazkhan left a comment •
edited

Choose a reason for hiding this comment

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

adding incoming webhook is working as expected.

@larkox larkox added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels May 24, 2024
@larkox larkox merged commit 3cac94a into mattermost:master May 24, 2024
16 checks passed
@mattermost-build mattermost-build removed the Setup Cloud Test Server Setup an on-prem test server label May 24, 2024
@mm-cloud-bot
Copy link

Test server destroyed

@mm-cloud-bot
Copy link

Test server destroyed

@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larkox larkox larkox approved these changes

@yasserfaraazkhan yasserfaraazkhan yasserfaraazkhan approved these changes

@M-ZubairAhmed M-ZubairAhmed M-ZubairAhmed approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Contributor Docs/Not Needed Does not require documentation release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Milestone
No milestone
7 participants
@MeHow25 @mm-cloud-bot @mattermost-build @M-ZubairAhmed @larkox @yasserfaraazkhan @amyblais