-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Added telemetry, test and bug fix for true up notifications #12170
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @Afsoon ! Just a few typos, some verbiage to sync up, and a possible simplification on the overage hook.
expandableLink, | ||
trackEventFn, | ||
getRequestState, | ||
isExpendable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo nit: Change the various usages of expendable
to expandable
@@ -85,15 +98,24 @@ const OverageUsersBannerNotice = () => { | |||
id='licensingPage.overageUsersBanner.noticeDescription' | |||
defaultMessage='Notify your Customer Success Manager on your next true-up check. <a>Contact Sales</a>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Since we are setting the inner link text dynamically, I tihnk we should change the defaultMessage and i18n/en.json string to say ... check. <a></a>
} | ||
}, [dispatch, getRequestState, licenseId, shouldRequest]); | ||
|
||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: since the value of isExpandable depends on the subscription stats call being made and defaults to false, and since the default state of the isExpandable value is already false, I think we can simplify cta to be a useMemo
without any useState
or useEffect
, like this:
const cta = useMemo(() => (
isExpandable ? formatMessage({
id: 'licensingPage.overageUsersBanner.ctaExpandSeats',
defaultMessage: 'Purchase additional seats',
})
: formatMessage({
id: 'licensingPage.overageUsersBanner.cta',
defaultMessage: 'Contact Sales',
})
), [isExpandable])
We can use whatever formatting and forms of return for the useMemo
, just picked the ternary because it was easy to type in the Github review comment box.
components/invitation_modal/overage_users_banner_notice/overage_users_banner_notice.test.tsx
Show resolved
Hide resolved
Successfully triggered E2E testing! |
/update-branch |
/e2e-test |
Successfully triggered E2E testing! |
Hi @neallred , |
Thanks for the callout! I'll take a look. |
…p into true-up-telemetry-and-bug
…p into true-up-telemetry-and-bug
Hi @neallred |
@yasserfaraazkhan I believe that this PR was fixing a bug in the invite modal where it also shows a CTA if they are over the amount of licensed seats. From the original PR description:
|
Test server destroyed |
1 similar comment
Test server destroyed |
/cherry-pick cloud |
Cherry pick is scheduled. |
…12247) * Added telemetry, test and bug fix for true up notifications * gov skus should not show true-up UI --------- Co-authored-by: neallred <neallred@protonmail.com> Co-authored-by: Mattermost Build <build@mattermost.com> (cherry picked from commit d727121) Co-authored-by: Said Atrahouch <said.atrahouch@gmail.com>
@neallred, can you confirm whether there are changes to the telemetry actions that should be documented? Thank you! |
I don't see any existing telemetry category covering this, so I think changes are needed. I would place it underneath https://docs.mattermost.com/manage/telemetry.html#event-data, under the "Non-personally Identifiable Diagnostic Information, distinguished by end users and System Admins" section, as its own item, maybe something like |
* Update conf.py * Update bulk-export-data.rst (#6269) Update for mattermost/mattermost#22145 * Update audit-log.rst (#6268) * Update audit-log.rst Updated incorrect badge. * Update source/comply/audit-log.rst * Update true-up review documentation. (#6191) * Update true-up review documentation. * Change wording, make plural. * update wording again. * Add back old true up section, noting the version with the new process, add a section going over the new process for mattermost versions 7.9 or higher. * Update title formatting * Update title * Fix tips. * Add reporting period image to new section. * Update self-hosted-subscriptions.rst * change registered user to active user. * Update self-hosted-subscriptions.rst --------- Co-authored-by: Justine Geffen <justinegeffen@users.noreply.github.com> Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update send-messages.rst (#6272) Docs for mattermost/mattermost-webapp#11069 * Update insights.rst (#6276) * Update insights.rst * Update source/welcome/insights.rst Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> --------- Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update insights.rst * Update send-messages.rst (#6277) * v7.9 Changelog (#6246) * Update self-managed-changelog.md * Update open-source-components.rst * Update important-upgrade-notes.rst * Update release-lifecycle.rst * Update version-archive.rst * Update extended-support-release.rst * Update prepare-to-upgrade-mattermost.rst * Update important-upgrade-notes.rst * Update self-managed-changelog.md * Update important-upgrade-notes.rst * Update self-managed-changelog.md * Apply suggestions from code review Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> * Update important-upgrade-notes.rst * Update self-managed-changelog.md * Update self-managed-changelog.md * Update self-managed-changelog.md * Update important-upgrade-notes.rst * Update self-managed-changelog.md * Update software-hardware-requirements.rst * Update self-managed-changelog.md * Update self-managed-changelog.md * Update self-managed-changelog.md * Update source/install/self-managed-changelog.md Co-authored-by: Winson Wu <93531870+wuwinson@users.noreply.github.com> * Update self-managed-changelog.md * Update self-managed-changelog.md * Update important-upgrade-notes.rst * Update self-managed-changelog.md * Update self-managed-changelog.md * Update self-managed-changelog.md * Update self-managed-changelog.md * Update important-upgrade-notes.rst * Update self-managed-changelog.md --------- Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> Co-authored-by: Winson Wu <93531870+wuwinson@users.noreply.github.com> * Update share-and-collaborate.rst (#6281) * Create trouble-postgres.rst (#6275) * Create trouble-postgres.rst * Update trouble-postgres.rst * Update trouble-postgres.rst * Update trouble-postgres.rst * Update telemetry.rst (#6283) Update for mattermost/mattermost-webapp#12170 --------- Co-authored-by: Justine Geffen <justinegeffen@users.noreply.github.com> Co-authored-by: Conor Macpherson <116016004+ConorMacpherson@users.noreply.github.com> Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> Co-authored-by: Winson Wu <93531870+wuwinson@users.noreply.github.com>
Summary
This PR includes:
Ticket Link
Bug: https://mattermost.atlassian.net/browse/MM-50380
Telemetry: https://mattermost.atlassian.net/browse/MM-50343
Release Note