Skip to content

Commit 4f1eac6

Browse files
t3chguyt3chguy
authored
Fix share button in discovery settings being disabled incorrectly (#29151)
* Fix share button in discovery settings being disabled incorrectly Signed-off-by: Michael Telatynski <[email protected]> * Improve types & add tests Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Improve coverage Signed-off-by: Michael Telatynski <[email protected]> * Add missing snapshot Signed-off-by: Michael Telatynski <[email protected]> --------- Signed-off-by: Michael Telatynski <[email protected]>
1 parent aa01b17 commit 4f1eac6

File tree

13 files changed

+332
-94
lines changed

13 files changed

+332
-94
lines changed

‎src/Terms.ts

Lines changed: 12 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,12 @@ Please see LICENSE files in the repository root for full details.
77
*/
88

99
import classNames from "classnames";
10-
import { SERVICE_TYPES, MatrixClient } from "matrix-js-sdk/src/matrix";
10+
import { SERVICE_TYPES, MatrixClient, Terms, Policy, InternationalisedPolicy } from "matrix-js-sdk/src/matrix";
1111
import { logger } from "matrix-js-sdk/src/logger";
1212

1313
import Modal from "./Modal";
1414
import TermsDialog from "./components/views/dialogs/TermsDialog";
15+
import { pickBestLanguage } from "./languageHandler.tsx";
1516

1617
export class TermsNotSignedError extends Error {}
1718

@@ -32,23 +33,8 @@ export class Service {
3233
) {}
3334
}
3435

35-
export interface LocalisedPolicy {
36-
name: string;
37-
url: string;
38-
}
39-
40-
export interface Policy {
41-
// @ts-ignore: No great way to express indexed types together with other keys
42-
version: string;
43-
[lang: string]: LocalisedPolicy;
44-
}
45-
46-
export type Policies = {
47-
[policy: string]: Policy;
48-
};
49-
5036
export type ServicePolicyPair = {
51-
policies: Policies;
37+
policies: Terms["policies"];
5238
service: Service;
5339
};
5440

@@ -58,6 +44,11 @@ export type TermsInteractionCallback = (
5844
extraClassNames?: string,
5945
) => Promise<string[]>;
6046

47+
export function pickBestPolicyLanguage(policy: Policy): InternationalisedPolicy | undefined {
48+
const termsLang = pickBestLanguage(Object.keys(policy).filter((k) => k !== "version"));
49+
return <InternationalisedPolicy>policy[termsLang];
50+
}
51+
6152
/**
6253
* Start a flow where the user is presented with terms & conditions for some services
6354
*
@@ -96,7 +87,7 @@ export async function startTermsFlow(
9687
* }
9788
*/
9889

99-
const terms: { policies: Policies }[] = await Promise.all(termsPromises);
90+
const terms: Terms[] = await Promise.all(termsPromises);
10091
const policiesAndServicePairs = terms.map((t, i) => {
10192
return { service: services[i], policies: t.policies };
10293
});
@@ -113,11 +104,11 @@ export async function startTermsFlow(
113104
// things they've not agreed to yet.
114105
const unagreedPoliciesAndServicePairs: ServicePolicyPair[] = [];
115106
for (const { service, policies } of policiesAndServicePairs) {
116-
const unagreedPolicies: Policies = {};
107+
const unagreedPolicies: Terms["policies"] = {};
117108
for (const [policyName, policy] of Object.entries(policies)) {
118109
let policyAgreed = false;
119110
for (const lang of Object.keys(policy)) {
120-
if (lang === "version") continue;
111+
if (lang === "version" || typeof policy[lang] === "string") continue;
121112
if (agreedUrlSet.has(policy[lang].url)) {
122113
policyAgreed = true;
123114
break;
@@ -154,7 +145,7 @@ export async function startTermsFlow(
154145
const urlsForService = Array.from(agreedUrlSet).filter((url) => {
155146
for (const policy of Object.values(policiesAndService.policies)) {
156147
for (const lang of Object.keys(policy)) {
157-
if (lang === "version") continue;
148+
if (lang === "version" || typeof policy[lang] === "string") continue;
158149
if (policy[lang].url === url) return true;
159150
}
160151
}

‎src/components/views/auth/InteractiveAuthEntryComponents.tsx

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ Please see LICENSE files in the repository root for full details.
77
*/
88

99
import classNames from "classnames";
10-
import { MatrixClient } from "matrix-js-sdk/src/matrix";
10+
import { InternationalisedPolicy, Terms, MatrixClient } from "matrix-js-sdk/src/matrix";
1111
import { AuthType, AuthDict, IInputs, IStageStatus } from "matrix-js-sdk/src/interactive-auth";
1212
import { logger } from "matrix-js-sdk/src/logger";
1313
import React, { ChangeEvent, createRef, FormEvent, Fragment } from "react";
@@ -16,14 +16,13 @@ import PopOutIcon from "@vector-im/compound-design-tokens/assets/web/icons/pop-o
1616

1717
import EmailPromptIcon from "../../../../res/img/element-icons/email-prompt.svg";
1818
import { _t } from "../../../languageHandler";
19-
import SettingsStore from "../../../settings/SettingsStore";
20-
import { LocalisedPolicy, Policies } from "../../../Terms";
2119
import { AuthHeaderModifier } from "../../structures/auth/header/AuthHeaderModifier";
2220
import AccessibleButton, { AccessibleButtonKind, ButtonEvent } from "../elements/AccessibleButton";
2321
import Field from "../elements/Field";
2422
import Spinner from "../elements/Spinner";
2523
import CaptchaForm from "./CaptchaForm";
2624
import { Flex } from "../../utils/Flex";
25+
import { pickBestPolicyLanguage } from "../../../Terms.ts";
2726

2827
/* This file contains a collection of components which are used by the
2928
* InteractiveAuth to prompt the user to enter the information needed
@@ -235,12 +234,10 @@ export class RecaptchaAuthEntry extends React.Component<IRecaptchaAuthEntryProps
235234
}
236235

237236
interface ITermsAuthEntryProps extends IAuthEntryProps {
238-
stageParams?: {
239-
policies?: Policies;
240-
};
237+
stageParams?: Partial<Terms>;
241238
}
242239

243-
interface LocalisedPolicyWithId extends LocalisedPolicy {
240+
interface LocalisedPolicyWithId extends InternationalisedPolicy {
244241
id: string;
245242
}
246243

@@ -278,7 +275,6 @@ export class TermsAuthEntry extends React.Component<ITermsAuthEntryProps, ITerms
278275
// }
279276

280277
const allPolicies = this.props.stageParams?.policies || {};
281-
const prefLang = SettingsStore.getValue("language");
282278
const initToggles: Record<string, boolean> = {};
283279
const pickedPolicies: {
284280
id: string;
@@ -287,17 +283,7 @@ export class TermsAuthEntry extends React.Component<ITermsAuthEntryProps, ITerms
287283
}[] = [];
288284
for (const policyId of Object.keys(allPolicies)) {
289285
const policy = allPolicies[policyId];
290-
291-
// Pick a language based on the user's language, falling back to english,
292-
// and finally to the first language available. If there's still no policy
293-
// available then the homeserver isn't respecting the spec.
294-
let langPolicy: LocalisedPolicy | undefined = policy[prefLang];
295-
if (!langPolicy) langPolicy = policy["en"];
296-
if (!langPolicy) {
297-
// last resort
298-
const firstLang = Object.keys(policy).find((e) => e !== "version");
299-
langPolicy = firstLang ? policy[firstLang] : undefined;
300-
}
286+
const langPolicy = pickBestPolicyLanguage(policy);
301287
if (!langPolicy) throw new Error("Failed to find a policy to show the user");
302288

303289
initToggles[policyId] = false;

‎src/components/views/dialogs/TermsDialog.tsx

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,10 @@ Please see LICENSE files in the repository root for full details.
99
import React from "react";
1010
import { SERVICE_TYPES } from "matrix-js-sdk/src/matrix";
1111

12-
import { _t, pickBestLanguage } from "../../../languageHandler";
12+
import { _t } from "../../../languageHandler";
1313
import DialogButtons from "../elements/DialogButtons";
1414
import BaseDialog from "./BaseDialog";
15-
import { ServicePolicyPair } from "../../../Terms";
15+
import { pickBestPolicyLanguage, ServicePolicyPair } from "../../../Terms";
1616
import ExternalLink from "../elements/ExternalLink";
1717
import { parseUrl } from "../../../utils/UrlUtils";
1818

@@ -126,8 +126,8 @@ export default class TermsDialog extends React.PureComponent<ITermsDialogProps,
126126

127127
const policyValues = Object.values(policiesAndService.policies);
128128
for (let i = 0; i < policyValues.length; ++i) {
129-
const termDoc = policyValues[i];
130-
const termsLang = pickBestLanguage(Object.keys(termDoc).filter((k) => k !== "version"));
129+
const internationalisedPolicy = pickBestPolicyLanguage(policyValues[i]);
130+
if (!internationalisedPolicy) continue;
131131
let serviceName: JSX.Element | undefined;
132132
let summary: JSX.Element | undefined;
133133
if (i === 0) {
@@ -136,19 +136,19 @@ export default class TermsDialog extends React.PureComponent<ITermsDialogProps,
136136
}
137137

138138
rows.push(
139-
<tr key={termDoc[termsLang].url}>
139+
<tr key={internationalisedPolicy.url}>
140140
<td className="mx_TermsDialog_service">{serviceName}</td>
141141
<td className="mx_TermsDialog_summary">{summary}</td>
142142
<td>
143-
<ExternalLink rel="noreferrer noopener" target="_blank" href={termDoc[termsLang].url}>
144-
{termDoc[termsLang].name}
143+
<ExternalLink rel="noreferrer noopener" target="_blank" href={internationalisedPolicy.url}>
144+
{internationalisedPolicy.name}
145145
</ExternalLink>
146146
</td>
147147
<td>
148148
<TermsCheckbox
149-
url={termDoc[termsLang].url}
149+
url={internationalisedPolicy.url}
150150
onChange={this.onTermsCheckboxChange}
151-
checked={Boolean(this.state.agreedUrls[termDoc[termsLang].url])}
151+
checked={Boolean(this.state.agreedUrls[internationalisedPolicy.url])}
152152
/>
153153
</td>
154154
</tr>,
@@ -164,7 +164,7 @@ export default class TermsDialog extends React.PureComponent<ITermsDialogProps,
164164
for (const terms of Object.values(policiesAndService.policies)) {
165165
let docAgreed = false;
166166
for (const lang of Object.keys(terms)) {
167-
if (lang === "version") continue;
167+
if (lang === "version" || typeof terms[lang] === "string") continue;
168168
if (this.state.agreedUrls[terms[lang].url]) {
169169
docAgreed = true;
170170
break;

‎src/components/views/settings/discovery/DiscoverySettings.tsx

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ export const DiscoverySettings: React.FC = () => {
5858
agreedUrls: null, // From the startTermsFlow callback
5959
resolve: null, // Promise resolve function for startTermsFlow callback
6060
});
61-
const [hasTerms, setHasTerms] = useState<boolean>(false);
61+
const [mustAgreeToTerms, setMustAgreeToTerms] = useState<boolean>(false);
6262

6363
const getThreepidState = useCallback(async () => {
6464
setIsLoadingThreepids(true);
@@ -103,7 +103,7 @@ export const DiscoverySettings: React.FC = () => {
103103
(policiesAndServices, agreedUrls, extraClassNames) => {
104104
return new Promise((resolve) => {
105105
setIdServerName(abbreviateUrl(idServerUrl));
106-
setHasTerms(true);
106+
setMustAgreeToTerms(true);
107107
setRequiredPolicyInfo({
108108
policiesAndServices,
109109
agreedUrls,
@@ -113,7 +113,7 @@ export const DiscoverySettings: React.FC = () => {
113113
},
114114
);
115115
// User accepted all terms
116-
setHasTerms(false);
116+
setMustAgreeToTerms(false);
117117
} catch (e) {
118118
logger.warn(
119119
`Unable to reach identity server at ${idServerUrl} to check ` + `for terms in Settings`,
@@ -126,7 +126,7 @@ export const DiscoverySettings: React.FC = () => {
126126

127127
if (!SettingsStore.getValue(UIFeature.ThirdPartyID)) return null;
128128

129-
if (hasTerms && requiredPolicyInfo.policiesAndServices) {
129+
if (mustAgreeToTerms && requiredPolicyInfo.policiesAndServices) {
130130
const intro = (
131131
<Alert type="info" title={_t("settings|general|discovery_needs_terms_title")}>
132132
{_t("settings|general|discovery_needs_terms", { serverName: idServerName })}
@@ -160,7 +160,7 @@ export const DiscoverySettings: React.FC = () => {
160160
medium={ThreepidMedium.Email}
161161
threepids={emails}
162162
onChange={getThreepidState}
163-
disabled={!hasTerms}
163+
disabled={mustAgreeToTerms}
164164
isLoading={isLoadingThreepids}
165165
/>
166166
</SettingsSubsection>
@@ -174,7 +174,7 @@ export const DiscoverySettings: React.FC = () => {
174174
medium={ThreepidMedium.Phone}
175175
threepids={phoneNumbers}
176176
onChange={getThreepidState}
177-
disabled={!hasTerms}
177+
disabled={mustAgreeToTerms}
178178
isLoading={isLoadingThreepids}
179179
/>
180180
</SettingsSubsection>

‎src/components/views/terms/InlineTermsAgreement.tsx

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,11 @@ Please see LICENSE files in the repository root for full details.
88

99
import React from "react";
1010

11-
import { _t, pickBestLanguage } from "../../../languageHandler";
11+
import { _t } from "../../../languageHandler";
1212
import { objectClone } from "../../../utils/objects";
1313
import StyledCheckbox from "../elements/StyledCheckbox";
1414
import AccessibleButton from "../elements/AccessibleButton";
15-
import { ServicePolicyPair } from "../../../Terms";
15+
import { pickBestPolicyLanguage, ServicePolicyPair } from "../../../Terms";
1616

1717
interface IProps {
1818
policiesAndServicePairs: ServicePolicyPair[];
@@ -47,11 +47,12 @@ export default class InlineTermsAgreement extends React.Component<IProps, IState
4747
for (const servicePolicies of this.props.policiesAndServicePairs) {
4848
const availablePolicies = Object.values(servicePolicies.policies);
4949
for (const policy of availablePolicies) {
50-
const language = pickBestLanguage(Object.keys(policy).filter((p) => p !== "version"));
50+
const internationalisedPolicy = pickBestPolicyLanguage(policy);
51+
if (!internationalisedPolicy) continue;
5152
const renderablePolicy: Policy = {
5253
checked: false,
53-
url: policy[language].url,
54-
name: policy[language].name,
54+
url: internationalisedPolicy.url,
55+
name: internationalisedPolicy.name,
5556
};
5657
policies.push(renderablePolicy);
5758
}

‎src/utils/IdentityServerUtils.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,10 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com
66
Please see LICENSE files in the repository root for full details.
77
*/
88

9-
import { SERVICE_TYPES, HTTPError, MatrixClient } from "matrix-js-sdk/src/matrix";
9+
import { SERVICE_TYPES, HTTPError, MatrixClient, Terms } from "matrix-js-sdk/src/matrix";
1010
import { logger } from "matrix-js-sdk/src/logger";
1111

1212
import SdkConfig from "../SdkConfig";
13-
import { Policies } from "../Terms";
1413

1514
export function getDefaultIdentityServerUrl(): string | undefined {
1615
return SdkConfig.get("validated_server_config")?.isUrl;
@@ -25,7 +24,7 @@ export function setToDefaultIdentityServer(matrixClient: MatrixClient): void {
2524
}
2625

2726
export async function doesIdentityServerHaveTerms(matrixClient: MatrixClient, fullUrl: string): Promise<boolean> {
28-
let terms: { policies?: Policies } | null;
27+
let terms: Partial<Terms> | null;
2928
try {
3029
terms = await matrixClient.getTerms(SERVICE_TYPES.IS, fullUrl);
3130
} catch (e) {

‎test/test-utils/test-utils.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ export function createTestClient(): MatrixClient {
217217
registerWithIdentityServer: jest.fn().mockResolvedValue({}),
218218
getIdentityAccount: jest.fn().mockResolvedValue({}),
219219
getTerms: jest.fn().mockResolvedValue({ policies: [] }),
220+
agreeToTerms: jest.fn(),
220221
doesServerSupportUnstableFeature: jest.fn().mockResolvedValue(undefined),
221222
isVersionSupported: jest.fn().mockResolvedValue(undefined),
222223
getPushRules: jest.fn().mockResolvedValue(undefined),

‎test/unit-tests/ScalarAuthClient-test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ describe("ScalarAuthClient", function () {
9797
body: { errcode: "M_TERMS_NOT_SIGNED" },
9898
});
9999
sac.exchangeForScalarToken = jest.fn(() => Promise.resolve("testtoken1"));
100-
mocked(client.getTerms).mockResolvedValue({ policies: [] });
100+
mocked(client.getTerms).mockResolvedValue({ policies: {} });
101101

102102
await expect(sac.registerForToken()).resolves.toBe("testtoken1");
103103
});

0 commit comments

Comments
 (0)