Skip to content

Use max_topic_length instead of hardcoded limit of 60 #1413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -526,9 +526,12 @@
"@loginErrorMissingUsername": {
"description": "Error message when an empty username was provided."
},
"topicValidationErrorTooLong": "Topic length shouldn't be greater than 60 characters.",
"topicValidationErrorTooLong": "Topic length shouldn't be greater than {num, plural, =1{1 character} other{{num} characters}}.",
"@topicValidationErrorTooLong": {
"description": "Topic validation error when topic is too long."
"description": "Topic validation error when topic is too long.",
"placeholders": {
"num": {"type": "int", "example": "60"}
}
},
"topicValidationErrorMandatoryButEmpty": "Topics are required in this organization.",
"@topicValidationErrorMandatoryButEmpty": {
Expand Down
3 changes: 3 additions & 0 deletions lib/api/model/initial_snapshot.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ class InitialSnapshot {

final List<CustomProfileField> customProfileFields;

final int maxTopicLength;

/// The realm-level policy, on pre-FL 163 servers, for visibility of real email addresses.
///
/// Search for "email_address_visibility" in https://zulip.com/api/register-queue.
Expand Down Expand Up @@ -123,6 +125,7 @@ class InitialSnapshot {
required this.zulipMergeBase,
required this.alertWords,
required this.customProfileFields,
required this.maxTopicLength,
required this.emailAddressVisibility,
required this.serverTypingStartedExpiryPeriodMilliseconds,
required this.serverTypingStoppedWaitPeriodMilliseconds,
Expand Down
2 changes: 2 additions & 0 deletions lib/api/model/initial_snapshot.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions lib/api/route/messages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,6 @@ class GetMessagesResult {
Map<String, dynamic> toJson() => _$GetMessagesResultToJson(this);
}

// https://zulip.com/api/send-message#parameter-topic
const int kMaxTopicLengthCodePoints = 60;

// https://zulip.com/api/send-message#parameter-content
const int kMaxMessageLengthCodePoints = 10000;
Expand Down
4 changes: 2 additions & 2 deletions lib/generated/l10n/zulip_localizations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -810,8 +810,8 @@ abstract class ZulipLocalizations {
/// Topic validation error when topic is too long.
///
/// In en, this message translates to:
/// **'Topic length shouldn\'t be greater than 60 characters.'**
String get topicValidationErrorTooLong;
/// **'Topic length shouldn\'t be greater than {num, plural, =1{1 character} other{{num} characters}}.'**
String topicValidationErrorTooLong(int num);

/// Topic validation error when topic is required but was empty.
///
Expand Down
10 changes: 9 additions & 1 deletion lib/generated/l10n/zulip_localizations_ar.dart
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,15 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
String get loginErrorMissingUsername => 'Please enter your username.';

@override
String get topicValidationErrorTooLong => 'Topic length shouldn\'t be greater than 60 characters.';
String topicValidationErrorTooLong(int num) {
String _temp0 = intl.Intl.pluralLogic(
num,
locale: localeName,
other: '$num characters',
one: '1 character',
);
return 'Topic length shouldn\'t be greater than $_temp0.';
}

@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
Expand Down
10 changes: 9 additions & 1 deletion lib/generated/l10n/zulip_localizations_en.dart
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,15 @@ class ZulipLocalizationsEn extends ZulipLocalizations {
String get loginErrorMissingUsername => 'Please enter your username.';

@override
String get topicValidationErrorTooLong => 'Topic length shouldn\'t be greater than 60 characters.';
String topicValidationErrorTooLong(int num) {
String _temp0 = intl.Intl.pluralLogic(
num,
locale: localeName,
other: '$num characters',
one: '1 character',
);
return 'Topic length shouldn\'t be greater than $_temp0.';
}

@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
Expand Down
10 changes: 9 additions & 1 deletion lib/generated/l10n/zulip_localizations_ja.dart
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,15 @@ class ZulipLocalizationsJa extends ZulipLocalizations {
String get loginErrorMissingUsername => 'Please enter your username.';

@override
String get topicValidationErrorTooLong => 'Topic length shouldn\'t be greater than 60 characters.';
String topicValidationErrorTooLong(int num) {
String _temp0 = intl.Intl.pluralLogic(
num,
locale: localeName,
other: '$num characters',
one: '1 character',
);
return 'Topic length shouldn\'t be greater than $_temp0.';
}

@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
Expand Down
10 changes: 9 additions & 1 deletion lib/generated/l10n/zulip_localizations_nb.dart
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,15 @@ class ZulipLocalizationsNb extends ZulipLocalizations {
String get loginErrorMissingUsername => 'Please enter your username.';

@override
String get topicValidationErrorTooLong => 'Topic length shouldn\'t be greater than 60 characters.';
String topicValidationErrorTooLong(int num) {
String _temp0 = intl.Intl.pluralLogic(
num,
locale: localeName,
other: '$num characters',
one: '1 character',
);
return 'Topic length shouldn\'t be greater than $_temp0.';
}

@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
Expand Down
4 changes: 3 additions & 1 deletion lib/generated/l10n/zulip_localizations_pl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,9 @@ class ZulipLocalizationsPl extends ZulipLocalizations {
String get loginErrorMissingUsername => 'Proszę podaj nazwę użytkownika.';

@override
String get topicValidationErrorTooLong => 'Tytuł nie może być dłuższy niż 60 znaków.';
String topicValidationErrorTooLong(int num) {
return 'Tytuł nie może być dłuższy niż 60 znaków.';
}

@override
String get topicValidationErrorMandatoryButEmpty => 'Wątki są wymagane przez tę organizację.';
Expand Down
4 changes: 3 additions & 1 deletion lib/generated/l10n/zulip_localizations_ru.dart
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,9 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
String get loginErrorMissingUsername => 'Пожалуйста, введите ваше имя пользователя.';

@override
String get topicValidationErrorTooLong => 'Длина темы не должна превышать 60 символов.';
String topicValidationErrorTooLong(int num) {
return 'Длина темы не должна превышать 60 символов.';
}

@override
String get topicValidationErrorMandatoryButEmpty => 'Темы обязательны в этой организации.';
Expand Down
10 changes: 9 additions & 1 deletion lib/generated/l10n/zulip_localizations_sk.dart
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,15 @@ class ZulipLocalizationsSk extends ZulipLocalizations {
String get loginErrorMissingUsername => 'Prosím zadajte prihlasovacie meno.';

@override
String get topicValidationErrorTooLong => 'Topic length shouldn\'t be greater than 60 characters.';
String topicValidationErrorTooLong(int num) {
String _temp0 = intl.Intl.pluralLogic(
num,
locale: localeName,
other: '$num characters',
one: '1 character',
);
return 'Topic length shouldn\'t be greater than $_temp0.';
}

@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
Expand Down
4 changes: 4 additions & 0 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
realmEmptyTopicDisplayName: initialSnapshot.realmEmptyTopicDisplayName,
realmDefaultExternalAccounts: initialSnapshot.realmDefaultExternalAccounts,
customProfileFields: _sortCustomProfileFields(initialSnapshot.customProfileFields),
maxTopicLength: initialSnapshot.maxTopicLength,
emailAddressVisibility: initialSnapshot.emailAddressVisibility,
emoji: EmojiStoreImpl(
realmUrl: realmUrl, allRealmEmoji: initialSnapshot.realmEmoji),
Expand Down Expand Up @@ -389,6 +390,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
required String? realmEmptyTopicDisplayName,
required this.realmDefaultExternalAccounts,
required this.customProfileFields,
required this.maxTopicLength,
required this.emailAddressVisibility,
required EmojiStoreImpl emoji,
required this.accountId,
Expand Down Expand Up @@ -470,6 +472,8 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
}
final String? _realmEmptyTopicDisplayName; // TODO(#668): update this realm setting

final int maxTopicLength;

final Map<String, RealmDefaultExternalAccount> realmDefaultExternalAccounts;
List<CustomProfileField> customProfileFields;
/// For docs, please see [InitialSnapshot.emailAddressVisibility].
Expand Down
15 changes: 7 additions & 8 deletions lib/widgets/compose_box.dart
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,10 @@ enum TopicValidationError {
mandatoryButEmpty,
tooLong;

String message(ZulipLocalizations zulipLocalizations) {
String message(ZulipLocalizations zulipLocalizations, {required int maxTopicLength}) {
switch (this) {
case tooLong:
return zulipLocalizations.topicValidationErrorTooLong;
return zulipLocalizations.topicValidationErrorTooLong(maxTopicLength);
case mandatoryButEmpty:
return zulipLocalizations.topicValidationErrorMandatoryButEmpty;
}
Expand All @@ -143,6 +143,7 @@ enum TopicValidationError {

class ComposeTopicController extends ComposeController<TopicValidationError> {
ComposeTopicController({required this.store}) {
maxLengthUnicodeCodePoints = store.maxTopicLength;
_update();
}

Expand All @@ -151,8 +152,7 @@ class ComposeTopicController extends ComposeController<TopicValidationError> {
// TODO(#668): listen to [PerAccountStore] once we subscribe to this value
bool get mandatory => store.realmMandatoryTopics;

// TODO(#307) use `max_topic_length` instead of hardcoded limit
@override final maxLengthUnicodeCodePoints = kMaxTopicLengthCodePoints;
@override late int maxLengthUnicodeCodePoints;

@override
String _computeTextNormalized() {
Expand Down Expand Up @@ -1105,10 +1105,9 @@ class _SendButtonState extends State<_SendButton> {
if (_hasValidationErrors) {
final zulipLocalizations = ZulipLocalizations.of(context);
List<String> validationErrorMessages = [
for (final error in (controller is StreamComposeBoxController
? controller.topic.validationErrors
: const <TopicValidationError>[]))
error.message(zulipLocalizations),
if (controller is StreamComposeBoxController)
for (final error in controller.topic.validationErrors)
error.message(zulipLocalizations, maxTopicLength: controller.topic.maxLengthUnicodeCodePoints),
for (final error in controller.content.validationErrors)
error.message(zulipLocalizations),
];
Expand Down
2 changes: 2 additions & 0 deletions test/example_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,7 @@ InitialSnapshot initialSnapshot({
String? zulipMergeBase,
List<String>? alertWords,
List<CustomProfileField>? customProfileFields,
int? maxTopicLength,
EmailAddressVisibility? emailAddressVisibility,
int? serverTypingStartedExpiryPeriodMilliseconds,
int? serverTypingStoppedWaitPeriodMilliseconds,
Expand Down Expand Up @@ -927,6 +928,7 @@ InitialSnapshot initialSnapshot({
zulipMergeBase: zulipMergeBase ?? recentZulipVersion,
alertWords: alertWords ?? ['klaxon'],
customProfileFields: customProfileFields ?? [],
maxTopicLength: maxTopicLength ?? 60,
emailAddressVisibility: emailAddressVisibility ?? EmailAddressVisibility.everyone,
serverTypingStartedExpiryPeriodMilliseconds:
serverTypingStartedExpiryPeriodMilliseconds ?? 15000,
Expand Down
30 changes: 19 additions & 11 deletions test/widgets/compose_box_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ void main() {
List<ZulipStream> streams = const [],
bool? mandatoryTopics,
int? zulipFeatureLevel,
int? maxTopicLength,
}) async {
if (narrow case ChannelNarrow(:var streamId) || TopicNarrow(: var streamId)) {
assert(streams.any((stream) => stream.streamId == streamId),
Expand All @@ -60,6 +61,7 @@ void main() {
await testBinding.globalStore.add(selfAccount, eg.initialSnapshot(
zulipFeatureLevel: zulipFeatureLevel,
realmMandatoryTopics: mandatoryTopics,
maxTopicLength: maxTopicLength,
));

store = await testBinding.globalStore.perAccount(selfAccount.id);
Expand Down Expand Up @@ -286,41 +288,47 @@ void main() {
});

group('topic', () {
Future<void> prepareWithTopic(WidgetTester tester, String topic) async {
Future<void> prepareWithTopic(WidgetTester tester, String topic, int maxTopicLength) async {
TypingNotifier.debugEnable = false;
addTearDown(TypingNotifier.debugReset);

final narrow = ChannelNarrow(channel.streamId);
await prepareComposeBox(tester, narrow: narrow, streams: [channel]);
await prepareComposeBox(tester, narrow: narrow, streams: [channel],
maxTopicLength: maxTopicLength);
await enterTopic(tester, narrow: narrow, topic: topic);
await enterContent(tester, 'some content');
}

Future<void> checkErrorResponse(WidgetTester tester) async {
Future<void> checkErrorResponse(WidgetTester tester, {required int maxTopicLength}) async {
await tester.tap(find.byWidget(checkErrorDialog(tester,
expectedTitle: 'Message not sent',
expectedMessage: 'Topic length shouldn\'t be greater than 60 characters.')));
expectedMessage: 'Topic length shouldn\'t be greater than $maxTopicLength characters.')));
}

final ValueVariant<int> maxTopicLengthVariants = ValueVariant<int>({50, 60, 70});

testWidgets('too-long topic is rejected', (tester) async {
final maxTopicLength = maxTopicLengthVariants.currentValue!;
await prepareWithTopic(tester,
makeStringWithCodePoints(kMaxTopicLengthCodePoints + 1));
makeStringWithCodePoints(maxTopicLength + 1), maxTopicLength);
await tapSendButton(tester);
await checkErrorResponse(tester);
});
await checkErrorResponse(tester, maxTopicLength: maxTopicLength);
}, variant: maxTopicLengthVariants);

testWidgets('max-length topic not rejected', (tester) async {
final maxTopicLength = maxTopicLengthVariants.currentValue!;
await prepareWithTopic(tester,
makeStringWithCodePoints(kMaxTopicLengthCodePoints));
makeStringWithCodePoints(maxTopicLength), maxTopicLength);
await tapSendButton(tester);
checkNoErrorDialog(tester);
});
}, variant: maxTopicLengthVariants);

testWidgets('code points not counted unnecessarily', (tester) async {
await prepareWithTopic(tester, 'a' * kMaxTopicLengthCodePoints);
final maxTopicLength = maxTopicLengthVariants.currentValue!;
await prepareWithTopic(tester, 'a' * maxTopicLength, maxTopicLength);
check((controller as StreamComposeBoxController)
.topic.debugLengthUnicodeCodePointsIfLong).isNull();
});
}, variant: maxTopicLengthVariants);
});
});

Expand Down