From c043db0ed99ece44c4135f273cdf7d322d3b21ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Mon, 16 May 2022 10:02:17 +0200 Subject: [PATCH] Merge pull request #7792 from overleaf/ta-settings-fixes-4 [SettingsPage] Wording and Error Handling GitOrigin-RevId: 1e2445a68e0d32cbec558832892f2ce5a051d729 --- .../src/Features/User/UserPagesController.js | 6 +- .../app/views/project/list/notifications.pug | 2 +- .../web/app/views/user/settings-react.pug | 2 +- .../settings/components/linking-section.tsx | 8 ++- .../components/linking/integration-widget.tsx | 19 ++++++- .../components/linking/sso-widget.tsx | 11 +++- .../settings/components/linking/status.tsx | 57 +++++++++++++++++++ .../js/features/settings/components/root.tsx | 7 --- .../stories/settings/helpers/linking.js | 1 + .../stories/settings/linking.stories.js | 23 ++++++++ .../stylesheets/app/account-settings.less | 4 ++ services/web/locales/en.json | 10 ++-- .../components/linking-section.test.tsx | 6 ++ .../components/linking/sso-widget.test.tsx | 11 +++- 14 files changed, 143 insertions(+), 24 deletions(-) create mode 100644 services/web/frontend/js/features/settings/components/linking/status.tsx diff --git a/services/web/app/src/Features/User/UserPagesController.js b/services/web/app/src/Features/User/UserPagesController.js index 4a86c0395f..873110c0dc 100644 --- a/services/web/app/src/Features/User/UserPagesController.js +++ b/services/web/app/src/Features/User/UserPagesController.js @@ -18,6 +18,10 @@ async function settingsPage(req, res) { if (ssoError) { delete req.session.ssoError } + const ssoErrorMessage = req.session.ssoErrorMessage + if (ssoErrorMessage) { + delete req.session.ssoErrorMessage + } // Institution SSO let institutionLinked = _.get(req.session, ['saml', 'linked']) if (institutionLinked) { @@ -97,7 +101,7 @@ async function settingsPage(req, res) { reconfirmedViaSAML, reconfirmationRemoveEmail, samlBeta: req.session.samlBeta, - ssoError: ssoError, + ssoErrorMessage, thirdPartyIds: UserPagesController._restructureThirdPartyIds(user), }) } else { diff --git a/services/web/app/views/project/list/notifications.pug b/services/web/app/views/project/list/notifications.pug index 28a1ab2d7f..a8672d9c36 100644 --- a/services/web/app/views/project/list/notifications.pug +++ b/services/web/app/views/project/list/notifications.pug @@ -126,7 +126,7 @@ include ../../_mixins/reconfirm_affiliation .notification-body if user.features.dropbox | !{translate("dropbox_unlinked_premium_feature", {}, ['strong'])} - | !{translate("can_now_relink_dropbox", {}, [{name: 'a', attrs: {href: '/user/settings#dropboxSettings' }}])} + | !{translate("can_now_relink_dropbox", {}, [{name: 'a', attrs: {href: '/user/settings#project-sync' }}])} else | !{translate("dropbox_unlinked_premium_feature", {}, ['strong'])} | !{translate("confirm_affiliation_to_relink_dropbox")} diff --git a/services/web/app/views/user/settings-react.pug b/services/web/app/views/user/settings-react.pug index 2775dfb50c..e5db2fae3c 100644 --- a/services/web/app/views/user/settings-react.pug +++ b/services/web/app/views/user/settings-react.pug @@ -14,7 +14,7 @@ block append meta meta(name="ol-reconfirmedViaSAML", content=reconfirmedViaSAML) meta(name="ol-reconfirmationRemoveEmail", content=reconfirmationRemoveEmail) meta(name="ol-samlBeta", content=samlBeta) - meta(name="ol-ssoError", content=ssoError) + meta(name="ol-ssoErrorMessage", content=ssoErrorMessage) meta(name="ol-thirdPartyIds", data-type="json", content=thirdPartyIds || {}) meta(name="ol-passwordStrengthOptions", data-type="json", content=settings.passwordStrengthOptions || {}) meta(name="ol-isExternalAuthenticationSystemUsed" data-type="boolean" content=externalAuthenticationSystemUsed()) diff --git a/services/web/frontend/js/features/settings/components/linking-section.tsx b/services/web/frontend/js/features/settings/components/linking-section.tsx index 7807e0d726..7a6073c7eb 100644 --- a/services/web/frontend/js/features/settings/components/linking-section.tsx +++ b/services/web/frontend/js/features/settings/components/linking-section.tsx @@ -1,4 +1,5 @@ import { useState } from 'react' +import { Alert } from 'react-bootstrap' import { useTranslation } from 'react-i18next' import importOverleafModules from '../../../../macros/import-overleaf-module.macro' import { useSSOContext, SSOSubscription } from '../context/sso-context' @@ -8,7 +9,7 @@ import getMeta from '../../../utils/meta' function LinkingSection() { const { t } = useTranslation() const { subscriptions } = useSSOContext() - + const ssoErrorMessage = getMeta('ol-ssoErrorMessage') as string const [integrationLinkingWidgets] = useState( () => getMeta('integrationLinkingWidgets') || @@ -77,6 +78,11 @@ function LinkingSection() {

{t('linked_accounts')}

+ {ssoErrorMessage ? ( + + {t('sso_link_error')}: {ssoErrorMessage} + + ) : null}
{Object.values(subscriptions).map( (subscription, subscriptionIndex) => ( diff --git a/services/web/frontend/js/features/settings/components/linking/integration-widget.tsx b/services/web/frontend/js/features/settings/components/linking/integration-widget.tsx index b588ec716d..5913a54e2b 100644 --- a/services/web/frontend/js/features/settings/components/linking/integration-widget.tsx +++ b/services/web/frontend/js/features/settings/components/linking/integration-widget.tsx @@ -16,6 +16,7 @@ type IntegrationLinkingWidgetProps = { unlinkPath: string unlinkConfirmationTitle: string unlinkConfirmationText: string + disabled?: boolean } export function IntegrationLinkingWidget({ @@ -30,6 +31,7 @@ export function IntegrationLinkingWidget({ unlinkPath, unlinkConfirmationTitle, unlinkConfirmationText, + disabled, }: IntegrationLinkingWidgetProps) { const { t } = useTranslation() @@ -59,7 +61,7 @@ export function IntegrationLinkingWidget({ {t('learn_more')}

- {linked && statusIndicator} + {hasFeature && statusIndicator}
void linkPath: string + disabled?: boolean } function ActionButton({ @@ -95,6 +99,7 @@ function ActionButton({ linked, handleUnlinkClick, linkPath, + disabled, }: ActionButtonProps) { const { t } = useTranslation() @@ -106,13 +111,21 @@ function ActionButton({ ) } else if (linked) { return ( - ) } else { return ( - + {t('link')} ) diff --git a/services/web/frontend/js/features/settings/components/linking/sso-widget.tsx b/services/web/frontend/js/features/settings/components/linking/sso-widget.tsx index 90bc1e7e94..db5e55d057 100644 --- a/services/web/frontend/js/features/settings/components/linking/sso-widget.tsx +++ b/services/web/frontend/js/features/settings/components/linking/sso-widget.tsx @@ -1,10 +1,12 @@ import { useCallback, useState } from 'react' import { useTranslation } from 'react-i18next' import { Button, Modal } from 'react-bootstrap' +import { FetchError } from '../../../../infrastructure/fetch-json' import AccessibleModal from '../../../../shared/components/accessible-modal' import IEEELogo from '../../../../shared/svgs/ieee-logo' import GoogleLogo from '../../../../shared/svgs/google-logo' import OrcidLogo from '../../../../shared/svgs/orcid-logo' +import LinkingStatus from './status' const providerLogos = { collabratec: , @@ -38,14 +40,15 @@ export function SSOLinkingWidget({ const handleUnlinkClick = useCallback(() => { setShowModal(true) + setErrorMessage('') }, []) const handleUnlinkConfirmationClick = useCallback(() => { setShowModal(false) setUnlinkRequestInflight(true) onUnlink() - .catch((error: Error) => { - setErrorMessage(error.message) + .catch((error: FetchError) => { + setErrorMessage(error.getUserFacingMessage()) }) .finally(() => { setUnlinkRequestInflight(false) @@ -71,7 +74,9 @@ export function SSOLinkingWidget({ ) : null}

- {errorMessage &&
{errorMessage}
} + {errorMessage ? ( + + ) : null}
+ + {description} + + ) +} + +type StatusIconProps = { + status: Status +} + +function StatusIcon({ status }: StatusIconProps) { + switch (status) { + case 'success': + return ( + + ) + case 'error': + return ( + + ) + case 'pending': + return ( + + ) + default: + return null + } +} diff --git a/services/web/frontend/js/features/settings/components/root.tsx b/services/web/frontend/js/features/settings/components/root.tsx index ecd913a8e4..856ac4ab15 100644 --- a/services/web/frontend/js/features/settings/components/root.tsx +++ b/services/web/frontend/js/features/settings/components/root.tsx @@ -1,5 +1,4 @@ import { useEffect } from 'react' -import { Alert } from 'react-bootstrap' import { useTranslation } from 'react-i18next' import getMeta from '../../../utils/meta' import EmailsSection from './emails-section' @@ -38,16 +37,10 @@ function SettingsPageRoot() { function SettingsPageContent() { const { t } = useTranslation() - const ssoError = getMeta('ol-ssoError') as string const { isOverleaf } = getMeta('ol-ExposedSettings') as ExposedSettings return ( - {ssoError ? ( - - {t('sso_link_error')}: {t(ssoError)} - - ) : null}

{t('account_settings')}

diff --git a/services/web/frontend/stories/settings/helpers/linking.js b/services/web/frontend/stories/settings/helpers/linking.js index 410f43a956..9400dbea1c 100644 --- a/services/web/frontend/stories/settings/helpers/linking.js +++ b/services/web/frontend/stories/settings/helpers/linking.js @@ -58,4 +58,5 @@ export function setDefaultMeta() { }) window.metaAttributesCache.delete('integrationLinkingWidgets') window.metaAttributesCache.delete('referenceLinkingWidgets') + window.metaAttributesCache.delete('ol-ssoErrorMessage') } diff --git a/services/web/frontend/stories/settings/linking.stories.js b/services/web/frontend/stories/settings/linking.stories.js index 6c45a33e91..391295c0d1 100644 --- a/services/web/frontend/stories/settings/linking.stories.js +++ b/services/web/frontend/stories/settings/linking.stories.js @@ -4,6 +4,8 @@ import { setDefaultMeta, defaultSetupMocks } from './helpers/linking' import { UserProvider } from '../../js/shared/context/user-context' import { SSOProvider } from '../../js/features/settings/context/sso-context' +const MOCK_DELAY = 1000 + export const Section = args => { useFetchMock(defaultSetupMocks) setDefaultMeta() @@ -40,6 +42,27 @@ export const SectionAllUnlinked = args => { ) } +export const SectionSSOErrors = args => { + useFetchMock(fetchMock => + fetchMock.post('/user/oauth-unlink', 500, { delay: MOCK_DELAY }) + ) + setDefaultMeta() + window.metaAttributesCache.set('integrationLinkingWidgets', []) + window.metaAttributesCache.set('referenceLinkingWidgets', []) + window.metaAttributesCache.set( + 'ol-ssoErrorMessage', + 'Account already linked to another Overleaf user' + ) + + return ( + + + + + + ) +} + export default { title: 'Account Settings / Linking', component: LinkingSection, diff --git a/services/web/frontend/stylesheets/app/account-settings.less b/services/web/frontend/stylesheets/app/account-settings.less index b757e0c342..8c3b4a3778 100644 --- a/services/web/frontend/stylesheets/app/account-settings.less +++ b/services/web/frontend/stylesheets/app/account-settings.less @@ -107,17 +107,21 @@ tbody > tr.affiliations-table-warning-row > td { margin-bottom: (@line-height-computed / 2) - @table-cell-padding; } +.settings-widget-status-icon, .dropbox-sync-icon { position: relative; font-size: 1.3em; line-height: 1.3em; vertical-align: top; + &.status-error, &.dropbox-sync-icon-error { color: @alert-danger-bg; } + &.status-success, &.dropbox-sync-icon-success { color: @alert-success-bg; } + &.status-pending, &.dropbox-sync-icon-updating { color: @alert-info-bg; &::after { diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 690e9bd695..437f50763d 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -635,7 +635,7 @@ "tracked_change_deleted": "Deleted", "show_all": "show all", "show_less": "show less", - "dropbox_sync_error": "Dropbox Sync Error", + "dropbox_sync_error": "Sorry, there was an error talking to our Dropbox service. Please try again in a few moments.", "send": "Send", "sending": "Sending", "invalid_password": "Invalid Password", @@ -762,7 +762,7 @@ "mendeley_reference_loading_error_forbidden": "Could not load references from Mendeley, please re-link your account and try again", "zotero_reference_loading_error_forbidden": "Could not load references from Zotero, please re-link your account and try again", "mendeley_integration": "Mendeley Integration", - "mendeley_sync_description": "With Mendeley integration you can import your references from Mendeley into your __appName__ projects.", + "mendeley_sync_description": "With the Mendeley integration you can import your references from Mendeley into your __appName__ projects.", "mendeley_is_premium": "Mendeley integration is a premium feature", "link_to_mendeley": "Link to Mendeley", "unlink_to_mendeley": "Unlink Mendeley", @@ -772,7 +772,7 @@ "mendeley_groups_loading_error": "There was an error loading groups from Mendeley", "mendeley_groups_relink": "There was an error accessing your Mendeley data. This was likely caused by lack of permissions. Please re-link your account and try again.", "zotero_integration": "Zotero Integration", - "zotero_sync_description": "With Zotero integration you can import your references from Zotero into your __appName__ projects.", + "zotero_sync_description": "With the Zotero integration you can import your references from Zotero into your __appName__ projects.", "zotero_is_premium": "Zotero integration is a premium feature", "link_to_zotero": "Link to Zotero", "unlink_to_zotero": "Unlink Zotero", @@ -1463,8 +1463,8 @@ "about_us": "About Us", "loading_recent_github_commits": "Loading recent commits", "no_new_commits_in_github": "No new commits in GitHub since last merge.", - "dropbox_sync_description": "Keep your __appName__ projects in sync with your Dropbox. Changes in __appName__ are automatically sent to your Dropbox, and the other way around.", - "github_sync_description": "With GitHub Sync you can link your __appName__ projects to GitHub repositories. Create new commits from __appName__, and merge with commits made offline or in GitHub.", + "dropbox_sync_description": "Keep your __appName__ projects in sync with your Dropbox account. Changes in __appName__ are automatically sent to your Dropbox account, and the other way around.", + "github_sync_description": "With GitHub Sync you can link your __appName__ projects to GitHub repositories, create new commits from __appName__, and merge commits from GitHub.", "github_import_description": "With GitHub Sync you can import your GitHub Repositories into __appName__. Create new commits from __appName__, and merge with commits made offline or in GitHub.", "link_to_github_description": "You need to authorise __appName__ to access your GitHub account to allow us to sync your projects.", "link": "Link", diff --git a/services/web/test/frontend/features/settings/components/linking-section.test.tsx b/services/web/test/frontend/features/settings/components/linking-section.test.tsx index 1492a2d75e..b70f19fa15 100644 --- a/services/web/test/frontend/features/settings/components/linking-section.test.tsx +++ b/services/web/test/frontend/features/settings/components/linking-section.test.tsx @@ -89,6 +89,12 @@ describe('', function () { expect(screen.queryByText('Twitter')).to.not.exist }) + it('shows SSO error message', async function () { + window.metaAttributesCache.set('ol-ssoErrorMessage', 'You no SSO') + renderSectionWithProviders() + screen.getByText('Error linking SSO account: You no SSO') + }) + it('does not show providers section when empty', async function () { window.metaAttributesCache.delete('ol-oauthProviders') renderSectionWithProviders() diff --git a/services/web/test/frontend/features/settings/components/linking/sso-widget.test.tsx b/services/web/test/frontend/features/settings/components/linking/sso-widget.test.tsx index 4d3cdc678d..e32bf81ec8 100644 --- a/services/web/test/frontend/features/settings/components/linking/sso-widget.test.tsx +++ b/services/web/test/frontend/features/settings/components/linking/sso-widget.test.tsx @@ -1,6 +1,7 @@ import { expect } from 'chai' import sinon from 'sinon' import { screen, fireEvent, render, waitFor } from '@testing-library/react' +import { FetchError } from '../../../../../../frontend/js/infrastructure/fetch-json' import { SSOLinkingWidget } from '../../../../../../frontend/js/features/settings/components/linking/sso-widget' describe('', function () { @@ -103,7 +104,9 @@ describe('', function () { describe('when unlinking fails', function () { beforeEach(function () { - const unlinkFunction = sinon.stub().rejects(new Error('unlinking failed')) + const unlinkFunction = sinon + .stub() + .rejects(new FetchError('unlinking failed', '')) render( ) @@ -116,7 +119,11 @@ describe('', function () { }) it('should display an error message ', async function () { - await waitFor(() => screen.getByText('unlinking failed')) + await waitFor(() => + screen.getByText( + 'Something went wrong talking to the server :(. Please try again.' + ) + ) }) it('should display the unlink button ', async function () {