Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

Add type='button' to buttons in system console, to prevent unintentional button clicks #6600

Merged
merged 9 commits into from
Oct 26, 2020
Merged

Add type='button' to buttons in system console, to prevent unintentional button clicks #6600

merged 9 commits into from
Oct 26, 2020

Conversation

outofgamut
Copy link
Contributor

@outofgamut outofgamut commented Oct 1, 2020 •
edited by prapti
Loading

Summary

Add types to buttons in admin_console, so that actions are not unintentionally triggered.

Ticket Link

mattermost/mattermost#15682
Jira ticket: https://mattermost.atlassian.net/browse/MM-29178

Related Pull Requests

None

Screenshots

None

@jasonblais jasonblais added the 2: Dev Review Requires review by a core commiter label Oct 1, 2020
@jasonblais
Copy link
Contributor

@outofgamut thanks for contributing!

hahmadia
hahmadia suggested changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@hahmadia hahmadia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey! Code looks good to me. Just small changes to styling otherwise all else looks good to me.
Lets make the styling of the code consistent across the code. You make also want to run a make check-style to fix the linting error that the CI build is showing below.

components/admin_console/admin_sidebar_header/admin_sidebar_header.tsx Outdated Show resolved Hide resolved
components/admin_console/billing/billing_subscriptions.tsx Outdated Show resolved Hide resolved
components/admin_console/billing/billing_subscriptions.tsx Outdated Show resolved Hide resolved
components/admin_console/billing/billing_subscriptions.tsx Outdated Show resolved Hide resolved
components/admin_console/billing/billing_subscriptions.tsx Outdated Show resolved Hide resolved
@outofgamut
Copy link
Contributor Author

@hahmadia Thanks for the feedback! It looks like the tests are failing. Anything I need to do on my end to update those snapshots?

@hahmadia
Copy link
Contributor

hahmadia commented Oct 1, 2020

@outofgamut
Can you run npm run updatesnapshot in your web-app directory and the push those changes? Let's see if that solves the problem. Thanks!

hahmadia
hahmadia approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@hahmadia hahmadia left a comment

Choose a reason for hiding this comment

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

Alright my man. Thanks for the quick changes! LGTM!!
abi5fb1we7rbtg69mn3jr4hwto-THANKYOU-1

mickmister
mickmister approved these changes Oct 2, 2020
View reviewed changes
Copy link
Member

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the great work @outofgamut!!

@mickmister mickmister added the 3: QA Review Requires review by a QA tester label Oct 2, 2020
hmhealey
hmhealey approved these changes Oct 5, 2020
View reviewed changes
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Looks good to me. I didn't know that <button> elements were treated as submit buttons by default, so that's good to know for the future.

@hahmadia
Copy link
Contributor

hahmadia commented Oct 6, 2020

/update-branch

@hmhealey hmhealey removed the 2: Dev Review Requires review by a core commiter label Oct 6, 2020
@mickmister mickmister mentioned this pull request Oct 9, 2020
Closed
@mickmister mickmister changed the title add button types to prevent unintentional actions Add type='button' to buttons in system console, to prevent unintentional button clicks Oct 9, 2020
@mickmister mickmister added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Oct 9, 2020
@mickmister
Copy link
Member

mickmister commented Oct 9, 2020 •
edited
Loading

Repro steps can be found in the issue's description. The test will need to be run on every page of the system console that has a button within its form, such as the Test Live URL button described in the description.

One side effect of this change is that pressing Enter now submits the page's form in some cases, when it would have normally done something unexpected.

@hanzei
Copy link
Contributor

hanzei commented Oct 13, 2020

@mickmister Heads up that there is a merge conflict to resolve

@snyk-bot snyk-bot mentioned this pull request Oct 20, 2020
Open
@jasonblais jasonblais removed the request for review from DHaussermann October 21, 2020 11:48
@hahmadia
Copy link
Contributor

/update-branch

@mattermod
Copy link
Contributor

Error trying to update the PR.
Please do it manually.

@hahmadia hahmadia self-assigned this Oct 22, 2020
@hahmadia
Copy link
Contributor

@mickmister
Any advice for how to QA this particular PR? I'm not even sure how this PR should be properly QA'ed. Thanks!

@hahmadia
Copy link
Contributor

/update-branch

@mickmister
Copy link
Member

mickmister commented Oct 26, 2020 •
edited
Loading

@hahmadia I've put some qa test steps in the Jira ticket https://mattermost.atlassian.net/browse/MM-29178 Feel free to ask me any questions

@prapti
Copy link
Contributor

prapti commented Oct 26, 2020

Thank you for the test steps @hahmadia and @mickmister! I tested the said form in the environment/web_server page as well as a couple other forms in admin console that I could recognize.
@mickmister Do you suspect this PR could break any cypress tests at all? Wondering if I should keep an eye out on the E2E tests as well.

@mickmister
Copy link
Member

Do you suspect this PR could break any cypress tests at all? Wondering if I should keep an eye out on the E2E tests as well.

@prapti I don't think it would break them, as we are just adding an attribute to the buttons. I'd be very surprised if any existing document element queries in the cypress tests are affected. The only tests I can think would be affected are snapshot tests, which have been updated in this PR.

prapti
prapti approved these changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@prapti prapti left a comment

Choose a reason for hiding this comment

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

Thank you for confirming Michael!
Thank you for this contribution @outofgamut! Looks great 🎉

@prapti prapti added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Oct 26, 2020
@hahmadia hahmadia removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Oct 26, 2020
@mm-cloud-bot
Copy link

Test server destroyed

@hahmadia hahmadia merged commit cee5911 into mattermost:master Oct 26, 2020
@amyblais amyblais added this to the v5.30.0 milestone Oct 26, 2020
@hanzei hanzei mentioned this pull request Oct 27, 2020
Closed
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Nov 16, 2020
This was referenced Sep 29, 2022
Open
Open
@giuesposbit giuesposbit mentioned this pull request Oct 8, 2022
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@prapti prapti prapti approved these changes

@hmhealey hmhealey hmhealey approved these changes

@mickmister mickmister mickmister approved these changes

@hahmadia hahmadia hahmadia 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 Docs/Not Needed Does not require documentation Hacktoberfest hacktoberfest-accepted
Projects
None yet
Milestone
v5.30.0
10 participants
@outofgamut @jasonblais @hahmadia @mickmister @hanzei @mattermod @prapti @mm-cloud-bot @hmhealey @amyblais