[MM-50467] Change usage of anchor tag in favour of new ExternalLink #12277

nickmisasi · 2023-02-27T15:54:40Z

Summary

We want to be able to enrich link-outs from mattermost to *.mattermost.com with contextual information. This will help us to identify PQL's. This PR implements changes required to satisfy the the new linter rule enforcing usage of this new component mattermost/eslint-plugin-mattermost#6

Ticket Link

https://mattermost.atlassian.net/browse/MM-50467

Related Pull Requests

Has server changes (please link here)
Has mobile changes (please link here)

Screenshots

Release Note

None

…o use ExternalLink

nickmisasi · 2023-02-27T15:57:39Z

components/external_link/index.tsx

+
+/* eslint-disable mattermost/use-external-link */
+
+import React from 'react';


The component itself

github-actions · 2023-02-27T17:47:31Z

Test Results

      1 files   816 suites 14m 54s ⏱️
6 938 tests 6 937 ✔️ 1 💤 0 ❌
7 114 runs 7 113 ✔️ 1 💤 0 ❌

Results for commit 128db87.

♻️ This comment has been updated with latest results.

nickmisasi · 2023-03-01T14:54:05Z

package.json

@@ -39,6 +39,7 @@
 "emoji-datasource": "6.1.1",
 "emoji-datasource-apple": "6.0.1",
 "emoji-regex": "10.2.1",
+ "eslint-plugin-mattermost": "github:mattermost/eslint-plugin-mattermost#9372e95228f2517c45f83418ea9c4d5b1e390cde",


Will need to replace this with the tip of master

neallred

LGTM overall! The only blocking things to me are:

There was an extra/double paste of a link
I think we need support for if the href prop already contains a query parameter.

Not sure if we want to support utils/markdown/link_only_renderer.tsx or utils/markdown/renderer.tsx ever (might be hard to do in any case), but wanted to raise that its possible we could get markdown linking off to mattermost.com and it wouldn't have the ExternalLink enhancement. I think we're ok with that since dynamic content isn't really part of a campaign per se, and I don't think we'd know what "location" to meaningfully attribute it to.

Found a few places that maybe should be external links:

components/seats_calculator/consequences.tsx , seeHowBillingWorks link

This one is an external link, but formulated in javascript instead of as plain links because of the cloud condition. Not sure how we want to handle it.

components/youtube_video/youtube_video.tsx

when noopener is used, nonempty target names other than _top, _self, and _parent are all treated like _blank in terms of deciding whether to open a new window/tab.

https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel/noopener

components/admin_console/admin_definition.jsx

                id: 'CustomIntegrationSettings',
                name: t('admin.integrations.integrationManagement.title'),
                name_default: 'Integration Management',
                settings: [
                    {
                        type: Constants.SettingsTypes.TYPE_BOOL,
                        key: 'ServiceSettings.EnableIncomingWebhooks',
                        label: t('admin.service.webhooksTitle'),
                        label_default: 'Enable Incoming Webhooks: ',
                        help_text: t('admin.service.webhooksDescription'),
                        help_text_default: 'When true, incoming webhooks will be allowed. To help combat phishing attacks, all posts from webhooks will be labelled by a BOT tag. See <link>documentation</link> to learn more.',
                        help_text_values: {
                            link: (msg) => (
                                <a
                                    href='https://developers.mattermost.com/integrate/admin-guide/admin-webhooks-incoming/'

components/admin_console/license_settings/trial_banner/trial_banner.tsx

export const EmbargoedEntityTrialError = () => {
    return (
        <FormattedMessage
            id='admin.license.trial-request.embargoed'
            defaultMessage='We were unable to process the request due to limitations for embargoed countries. <link>Learn more in our documentation</link>, or reach out to [email protected] for questions around export limitations.'
            values={{
                link: (text: string) => (
                    <a href={LicenseLinks.EMBARGOED_COUNTRIES}>
                        {text}
                    </a>
                ),
            }}
        />
    );
};

components/external_link/index.tsx

components/activity_and_insights/insights/insights_title/insights_title.tsx

components/external_link/index.tsx

components/create_team/components/team_url/team_url.tsx

components/external_link/external_link.test.tsx

components/warn_metric_ack_modal/warn_metric_ack_modal.tsx

components/widgets/menu/menu_items/menu_item_external_link.tsx

…f master

marianunez · 2023-03-08T16:30:01Z

components/external_link/index.tsx

+}
+
+type Props = React.AnchorHTMLAttributes<HTMLAnchorElement> & {
+ href?: string;


Should this be the minimum required?

marianunez · 2023-03-08T16:31:06Z

components/external_link/index.tsx

+ let href = props.href;
+ let queryParams = {};
+ if (href?.includes('mattermost.com')) {
+ // encode this stuff so it's not so transparent to the user?


We can remove this comment if we are not taking that direction

marianunez · 2023-03-08T18:20:35Z

components/external_link/index.tsx

+ utm_source: 'mattermost',
+ utm_medium: license.Cloud === 'true' ? 'in-product-cloud' : 'in-product',
+ utm_content: props.location || '',
+ uid: userId,


Is user id necessary?

Kevin mentioned it in lieu of PII. I think it would be useful for the data team since we'd be able to map this value to the user_actual_id from rudder events.

marianunez

Thanks @nickmisasi! Didn't do a review on all the replacements but the components LGTM with some non-blocking comments

marianunez · 2023-03-08T18:26:35Z

components/external_link/external_link.test.tsx

+ it('should attach parameters if telemetry is enabled', () => {
+ const state = {


Is this test still applicable?

I updated the name of the test, it's applicable now!

hmhealey

Looks good to me. There's just one thing that I think needs addressing (the dependency vs dev dependency one), but hopefully it's quick

hmhealey · 2023-03-08T19:57:34Z

components/product_notices_modal/product_notices_modal.tsx

 className='GenericModal__button actionButton'
- href={presentNoticeInfo.actionParam}
+ location='product_notices_modal'
+ href={presentNoticeInfo.actionParam || ''}


This is the first place I've noticed this during my review, but if we end up doing this a lot, we could always consider making this prop optional

hmhealey · 2023-03-08T20:01:17Z

package.json

@@ -39,6 +39,7 @@
 "emoji-datasource": "6.1.1",
 "emoji-datasource-apple": "6.0.1",
 "emoji-regex": "10.2.1",
+ "eslint-plugin-mattermost": "github:mattermost/eslint-plugin-mattermost#5b0c972eacf19286e4c66221b39113bf8728a99e",


Could you move this back to being a dev dependency? Strictly speaking, it doesn't matter for the web app (it would only really have an effect if we uploaded mattermost-webapp as an NPM package), but I think Security Team had some process around new non-dev dependencies

I'm not sure how this happened tbh. I think maybe the npm link from testing? Fixed

hmhealey · 2023-03-08T20:04:06Z

components/admin_console/database_settings.jsx

@@ -280,13 +282,12 @@ export default class DatabaseSettings extends AdminSettings {
 defaultMessage='Minimum number of characters in a hashtag. This must be greater than or equal to 2. MySQL databases must be configured to support searching strings shorter than three characters, <link>see documentation</link>.'
 values={{
 link: (msg) => (
- <a
+ <ExternalLink
+ location='database_settings'


Nothing to change here, but seeing these location things repeated does make me wonder if we'd benefit from wrapping the entire contents of these screens/modals/components in a Context that provides this. That'd be overkill to just save us from having to pass the same location repeatedly for multiple ExternalLinks, but I could see that sort of pattern being useful for other things in the future

Something like a "breadcrumb" in a context would be pretty cool for sure. I know there are some patterns around Cloud telemetry that sort of mimics that by appending 'location1 > location2' + 'location3' through props, which is a bit messy imo

mm-cloud-bot · 2023-03-09T16:42:56Z

Test server destroyed

nickmisasi added 2 commits February 24, 2023 10:39

Add ExternalLink component, update references to <a target='_blank' t…

1a93aa2

…o use ExternalLink

fix linter

d943d8b

nickmisasi added the Work in Progress Not yet ready for review label Feb 27, 2023

mm-cloud-bot added the release-note-none label Feb 27, 2023

Merge branch 'master' into MM-50467

e78d857

nickmisasi commented Feb 27, 2023

View reviewed changes

nickmisasi changed the title ~~[MM-50467] Change usage of anchor tag in favour of new ExternalLink~~ [MM-50467] WIP: Change usage of anchor tag in favour of new ExternalLink Feb 27, 2023

nickmisasi added 3 commits February 27, 2023 11:09

fix duplicate type

69e3895

use proper dep version for eslint-plugin-mattermost

532dab3

More fixes

2941ffc

nickmisasi added 4 commits February 27, 2023 14:46

Various fixes for test cases

147b715

Add some tests for the external link component

42339c3

Add snapshot

6685de5

Merge branch 'master' into MM-50467

4a75f8a

nickmisasi commented Mar 1, 2023

View reviewed changes

nickmisasi added 2 commits March 1, 2023 09:54

Revert change to package.json

8bbc8a8

add a few more tests

99a538f

nickmisasi marked this pull request as ready for review March 2, 2023 16:57

nickmisasi requested a review from a team as a code owner March 2, 2023 16:57

nickmisasi requested review from hmhealey and removed request for a team March 2, 2023 16:57

nickmisasi changed the title ~~[MM-50467] WIP: Change usage of anchor tag in favour of new ExternalLink~~ [MM-50467] Change usage of anchor tag in favour of new ExternalLink Mar 2, 2023

nickmisasi requested review from neallred and marianunez March 2, 2023 16:57

nickmisasi added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core commiter and removed Work in Progress Not yet ready for review labels Mar 2, 2023

neallred suggested changes Mar 2, 2023

View reviewed changes

PR Feedback

79eb8b1

nickmisasi added 2 commits March 3, 2023 14:56

Fix a slew of tests

d1cef6d

Fix linter

d4d1fcc

hmhealey added the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Mar 3, 2023

Update package version for eslint-plugin-mattermost to point at tip o…

97a2c22

…f master

nickmisasi removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Mar 6, 2023

marianunez reviewed Mar 8, 2023

View reviewed changes

marianunez approved these changes Mar 8, 2023

View reviewed changes

hmhealey suggested changes Mar 8, 2023

View reviewed changes

nickmisasi added 2 commits March 8, 2023 15:48

PR Feedback

fcc25c5

Feedback that didn't make it into last commit

8bc3113

nickmisasi requested a review from hmhealey March 8, 2023 20:55

hmhealey approved these changes Mar 9, 2023

View reviewed changes

hmhealey assigned nickmisasi Mar 9, 2023

hmhealey removed the 2: Dev Review Requires review by a core commiter label Mar 9, 2023

nickmisasi added 2 commits March 9, 2023 10:05

Fix test, missed occurence of anchor tag

b126ba2

Fix linter

128db87

nickmisasi requested a review from johndavidlugtu March 9, 2023 15:30

nickmisasi added Setup Cloud Test Server Setup a test server using Mattermost Cloud and removed Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Mar 9, 2023

mm-cloud-bot removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Mar 9, 2023

nickmisasi added Setup Cloud Test Server Setup a test server using Mattermost Cloud 4: Reviews Complete All reviewers have approved the pull request and removed 1: PM Review Requires review by a product manager labels Mar 9, 2023

nickmisasi merged commit 780bbe8 into master Mar 9, 2023

nickmisasi deleted the MM-50467 branch March 9, 2023 16:42

mattermost-build removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Mar 9, 2023

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-50467] Change usage of anchor tag in favour of new ExternalLink #12277

[MM-50467] Change usage of anchor tag in favour of new ExternalLink #12277

nickmisasi commented Feb 27, 2023 •

edited

Loading

nickmisasi Feb 27, 2023

github-actions bot commented Feb 27, 2023 •

edited

Loading

nickmisasi Mar 1, 2023

neallred left a comment

marianunez Mar 8, 2023

marianunez Mar 8, 2023

marianunez Mar 8, 2023

nickmisasi Mar 8, 2023

marianunez left a comment

marianunez Mar 8, 2023

nickmisasi Mar 8, 2023

hmhealey left a comment

hmhealey Mar 8, 2023

hmhealey Mar 8, 2023

nickmisasi Mar 8, 2023 •

edited

Loading

hmhealey Mar 8, 2023

nickmisasi Mar 8, 2023

mm-cloud-bot commented Mar 9, 2023


		/* eslint-disable mattermost/use-external-link */

		import React from 'react';

		it('should attach parameters if telemetry is enabled', () => {
		const state = {

[MM-50467] Change usage of anchor tag in favour of new ExternalLink #12277

[MM-50467] Change usage of anchor tag in favour of new ExternalLink #12277

Conversation

nickmisasi commented Feb 27, 2023 • edited Loading

Summary

Ticket Link

Related Pull Requests

Screenshots

Release Note

Choose a reason for hiding this comment

github-actions bot commented Feb 27, 2023 • edited Loading

Test Results

Choose a reason for hiding this comment

neallred left a 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

marianunez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmhealey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickmisasi Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mm-cloud-bot commented Mar 9, 2023

nickmisasi commented Feb 27, 2023 •

edited

Loading

github-actions bot commented Feb 27, 2023 •

edited

Loading

nickmisasi Mar 8, 2023 •

edited

Loading