Skip to content

Respect realm setting for mandatory topics #1296

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 30, 2025
Merged

Respect realm setting for mandatory topics #1296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8decc4e
store test: Check for data consistency when adding account
PIG208 Jan 21, 2025
3098349
store: Add realmMandatoryTopics realm setting
PIG208 Jan 21, 2025
9e5f803
compose test [nfc]: Remove unused prepareComposeBox param
PIG208 Jan 21, 2025
f76b003
compose test [nfc]: Rename param from user to otherUsers
PIG208 Jan 27, 2025
f1646c4
compose: Respect realm setting for mandatory topics
PIG208 Jan 21, 2025
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
3 changes: 3 additions & 0 deletions lib/api/model/initial_snapshot.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ class InitialSnapshot {

final List<UserTopicItem>? userTopics; // TODO(server-6)

final bool realmMandatoryTopics;

/// The number of days until a user's account is treated as a full member.
///
/// Search for "realm_waiting_period_threshold" in https://zulip.com/api/register-queue.
Expand Down Expand Up @@ -125,6 +127,7 @@ class InitialSnapshot {
required this.streams,
required this.userSettings,
required this.userTopics,
required this.realmMandatoryTopics,
required this.realmWaitingPeriodThreshold,
required this.realmDefaultExternalAccounts,
required this.maxFileUploadSizeMib,
Expand Down
2 changes: 2 additions & 0 deletions lib/api/model/initial_snapshot.g.dart
View file
Open in desktop

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

3 changes: 3 additions & 0 deletions lib/model/store.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
globalStore: globalStore,
connection: connection,
realmUrl: realmUrl,
realmMandatoryTopics: initialSnapshot.realmMandatoryTopics,
realmWaitingPeriodThreshold: initialSnapshot.realmWaitingPeriodThreshold,
maxFileUploadSizeMib: initialSnapshot.maxFileUploadSizeMib,
realmDefaultExternalAccounts: initialSnapshot.realmDefaultExternalAccounts,
Expand Down Expand Up @@ -311,6 +312,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
required GlobalStore globalStore,
required this.connection,
required this.realmUrl,
required this.realmMandatoryTopics,
required this.realmWaitingPeriodThreshold,
required this.maxFileUploadSizeMib,
required this.realmDefaultExternalAccounts,
Expand Down Expand Up @@ -375,6 +377,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
Uri? tryResolveUrl(String reference) => _tryResolveUrl(realmUrl, reference);

String get zulipVersion => account.zulipVersion;
final bool realmMandatoryTopics; // TODO(#668): update this realm setting
/// For docs, please see [InitialSnapshot.realmWaitingPeriodThreshold].
final int realmWaitingPeriodThreshold; // TODO(#668): update this realm setting
final int maxFileUploadSizeMib; // No event for this.
Expand Down
39 changes: 24 additions & 15 deletions lib/widgets/compose_box.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,14 @@ enum TopicValidationError {
}

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

// TODO: subscribe to this value:
// https://zulip.com/help/require-topics
final mandatory = true;
PerAccountStore store;

// 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;
Expand Down Expand Up @@ -1227,7 +1228,10 @@ sealed class ComposeBoxController {
}

class StreamComposeBoxController extends ComposeBoxController {
final topic = ComposeTopicController();
StreamComposeBoxController({required PerAccountStore store})
: topic = ComposeTopicController(store: store);

final ComposeTopicController topic;
final topicFocusNode = FocusNode();

@override
Expand Down Expand Up @@ -1308,16 +1312,20 @@ abstract class ComposeBoxState extends State<ComposeBox> {
ComposeBoxController get controller;
}

class _ComposeBoxState extends State<ComposeBox> implements ComposeBoxState {
@override ComposeBoxController get controller => _controller;
late final ComposeBoxController _controller;
class _ComposeBoxState extends State<ComposeBox> with PerAccountStoreAwareStateMixin<ComposeBox> implements ComposeBoxState {
@override ComposeBoxController get controller => _controller!;
ComposeBoxController? _controller;

@override
void initState() {
super.initState();
void onNewStore() {
switch (widget.narrow) {
case ChannelNarrow():
_controller = StreamComposeBoxController();
final store = PerAccountStoreWidget.of(context);
if (_controller == null) {
_controller = StreamComposeBoxController(store: store);
} else {
(controller as StreamComposeBoxController).topic.store = store;
}
case TopicNarrow():
case DmNarrow():
_controller = FixedDestinationComposeBoxController();
Expand All @@ -1330,7 +1338,7 @@ class _ComposeBoxState extends State<ComposeBox> implements ComposeBoxState {

@override
void dispose() {
_controller.dispose();
controller.dispose();
super.dispose();
}

Expand Down Expand Up @@ -1370,15 +1378,16 @@ class _ComposeBoxState extends State<ComposeBox> implements ComposeBoxState {
return _ComposeBoxContainer(body: null, errorBanner: errorBanner);
}

final controller = this.controller;
final narrow = widget.narrow;
switch (_controller) {
switch (controller) {
case StreamComposeBoxController(): {
narrow as ChannelNarrow;
body = _StreamComposeBoxBody(controller: _controller, narrow: narrow);
body = _StreamComposeBoxBody(controller: controller, narrow: narrow);
}
case FixedDestinationComposeBoxController(): {
narrow as SendableNarrow;
body = _FixedDestinationComposeBoxBody(controller: _controller, narrow: narrow);
body = _FixedDestinationComposeBoxBody(controller: controller, narrow: narrow);
}
}

Expand Down
2 changes: 2 additions & 0 deletions test/example_data.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,7 @@ InitialSnapshot initialSnapshot({
List<ZulipStream>? streams,
UserSettings? userSettings,
List<UserTopicItem>? userTopics,
bool? realmMandatoryTopics,
int? realmWaitingPeriodThreshold,
Map<String, RealmDefaultExternalAccount>? realmDefaultExternalAccounts,
int? maxFileUploadSizeMib,
Expand Down Expand Up @@ -890,6 +891,7 @@ InitialSnapshot initialSnapshot({
emojiset: Emojiset.google,
),
userTopics: userTopics,
realmMandatoryTopics: realmMandatoryTopics ?? true,
realmWaitingPeriodThreshold: realmWaitingPeriodThreshold ?? 0,
realmDefaultExternalAccounts: realmDefaultExternalAccounts ?? {},
maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25,
Expand Down
3 changes: 2 additions & 1 deletion test/model/autocomplete_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,8 @@ void main() {

final description = 'topic-input with text: $markedText produces: ${expectedQuery?.raw ?? 'No Query!'}';
test(description, () {
final controller = ComposeTopicController();
final store = eg.store();
final controller = ComposeTopicController(store: store);
controller.value = parsed.value;
if (expectedQuery == null) {
check(controller).autocompleteIntent.isNull();
Expand Down
1 change: 1 addition & 0 deletions test/model/store_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ extension PerAccountStoreChecks on Subject<PerAccountStore> {
Subject<bool> get isLoading => has((x) => x.isLoading, 'isLoading');
Subject<Uri> get realmUrl => has((x) => x.realmUrl, 'realmUrl');
Subject<String> get zulipVersion => has((x) => x.zulipVersion, 'zulipVersion');
Subject<bool> get realmMandatoryTopics => has((x) => x.realmMandatoryTopics, 'realmMandatoryTopics');
Subject<int> get maxFileUploadSizeMib => has((x) => x.maxFileUploadSizeMib, 'maxFileUploadSizeMib');
Subject<Map<String, RealmDefaultExternalAccount>> get realmDefaultExternalAccounts => has((x) => x.realmDefaultExternalAccounts, 'realmDefaultExternalAccounts');
Subject<List<CustomProfileField>> get customProfileFields => has((x) => x.customProfileFields, 'customProfileFields');
Expand Down
3 changes: 3 additions & 0 deletions test/model/test_store.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ class TestGlobalStore extends GlobalStore {
/// [PerAccountStore] when [perAccount] is subsequently called for this
/// account, in particular when a [PerAccountStoreWidget] is mounted.
Future<void> add(Account account, InitialSnapshot initialSnapshot) async {
assert(initialSnapshot.zulipVersion == account.zulipVersion);
assert(initialSnapshot.zulipMergeBase == account.zulipMergeBase);
assert(initialSnapshot.zulipFeatureLevel == account.zulipFeatureLevel);
await insertAccount(account.toCompanion(false));
assert(!_initialSnapshots.containsKey(account.id));
_initialSnapshots[account.id] = initialSnapshot;
Expand Down
75 changes: 65 additions & 10 deletions test/widgets/compose_box_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ void main() {
Future<void> prepareComposeBox(WidgetTester tester, {
required Narrow narrow,
User? selfUser,
int? realmWaitingPeriodThreshold,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit-message nit:

    compose test [nfc]: Remove unused prepareComposeBox param
    
    The parameter `realmWaitingPeriodThreshold` was added in
    commit 157418c26f003b783036dd5184364434cee7d197 but it was never used.
    
    Signed-off-by: Zixuan James Li <[email protected]>

For readability, I like use just the first 9 characters of a commit ID.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump on truncating commit IDs to 9 characters :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, missed that. Fixed!

List<User> users = const [],
List<User> otherUsers = const [],
Comment on lines -48 to +47
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the commit message says otherUser which doesn't match this name

List<ZulipStream> streams = const [],
bool? mandatoryTopics,
}) async {
if (narrow case ChannelNarrow(:var streamId) || TopicNarrow(: var streamId)) {
assert(streams.any((stream) => stream.streamId == streamId),
Expand All @@ -56,11 +56,12 @@ void main() {
selfUser ??= eg.selfUser;
final selfAccount = eg.account(user: selfUser);
await testBinding.globalStore.add(selfAccount, eg.initialSnapshot(
realmWaitingPeriodThreshold: realmWaitingPeriodThreshold));
realmMandatoryTopics: mandatoryTopics,
));

store = await testBinding.globalStore.perAccount(selfAccount.id);

await store.addUsers([selfUser, ...users]);
await store.addUsers([selfUser, ...otherUsers]);
await store.addStreams(streams);
connection = store.connection as FakeApiConnection;

Expand Down Expand Up @@ -560,6 +561,60 @@ void main() {
});
});

group('sending to empty topic', () {
late ZulipStream channel;

Future<void> setupAndTapSend(WidgetTester tester, {
required String topicInputText,
required bool mandatoryTopics,
}) async {
TypingNotifier.debugEnable = false;
addTearDown(TypingNotifier.debugReset);

channel = eg.stream();
final narrow = ChannelNarrow(channel.streamId);
await prepareComposeBox(tester,
narrow: narrow, streams: [channel],
mandatoryTopics: mandatoryTopics);

await enterTopic(tester, narrow: narrow, topic: topicInputText);
await tester.enterText(contentInputFinder, 'test content');
await tester.tap(find.byIcon(ZulipIcons.send));
await tester.pump();
}

void checkMessageNotSent(WidgetTester tester) {
check(connection.takeRequests()).isEmpty();
checkErrorDialog(tester,
expectedTitle: 'Message not sent',
expectedMessage: 'Topics are required in this organization.');
}

testWidgets('empty topic -> "(no topic)"', (tester) async {
await setupAndTapSend(tester,
topicInputText: '',
mandatoryTopics: false);
check(connection.lastRequest).isA<http.Request>()
..method.equals('POST')
..url.path.equals('/api/v1/messages')
..bodyFields['topic'].equals('(no topic)');
});

testWidgets('if topics are mandatory, reject empty topic', (tester) async {
await setupAndTapSend(tester,
topicInputText: '',
mandatoryTopics: true);
checkMessageNotSent(tester);
});

testWidgets('if topics are mandatory, reject "(no topic)"', (tester) async {
await setupAndTapSend(tester,
topicInputText: '(no topic)',
mandatoryTopics: true);
checkMessageNotSent(tester);
});
});

group('uploads', () {
void checkAppearsLoading(WidgetTester tester, bool expected) {
final sendButtonElement = tester.element(find.ancestor(
Expand Down Expand Up @@ -748,15 +803,15 @@ void main() {
testWidgets('compose box replaced with a banner', (tester) async {
final deactivatedUser = eg.user(isActive: false);
await prepareComposeBox(tester, narrow: dmNarrowWith(deactivatedUser),
users: [deactivatedUser]);
otherUsers: [deactivatedUser]);
checkComposeBox(isShown: false);
});

testWidgets('active user becomes deactivated -> '
'compose box is replaced with a banner', (tester) async {
final activeUser = eg.user(isActive: true);
await prepareComposeBox(tester, narrow: dmNarrowWith(activeUser),
users: [activeUser]);
otherUsers: [activeUser]);
checkComposeBox(isShown: true);

await changeUserStatus(tester, user: activeUser, isActive: false);
Expand All @@ -767,7 +822,7 @@ void main() {
'banner is replaced with the compose box', (tester) async {
final deactivatedUser = eg.user(isActive: false);
await prepareComposeBox(tester, narrow: dmNarrowWith(deactivatedUser),
users: [deactivatedUser]);
otherUsers: [deactivatedUser]);
checkComposeBox(isShown: false);

await changeUserStatus(tester, user: deactivatedUser, isActive: true);
Expand All @@ -779,15 +834,15 @@ void main() {
testWidgets('compose box replaced with a banner', (tester) async {
final deactivatedUsers = [eg.user(isActive: false), eg.user(isActive: false)];
await prepareComposeBox(tester, narrow: groupDmNarrowWith(deactivatedUsers),
users: deactivatedUsers);
otherUsers: deactivatedUsers);
checkComposeBox(isShown: false);
});

testWidgets('at least one user becomes deactivated -> '
'compose box is replaced with a banner', (tester) async {
final activeUsers = [eg.user(isActive: true), eg.user(isActive: true)];
await prepareComposeBox(tester, narrow: groupDmNarrowWith(activeUsers),
users: activeUsers);
otherUsers: activeUsers);
checkComposeBox(isShown: true);

await changeUserStatus(tester, user: activeUsers[0], isActive: false);
Expand All @@ -798,7 +853,7 @@ void main() {
'banner is replaced with the compose box', (tester) async {
final deactivatedUsers = [eg.user(isActive: false), eg.user(isActive: false)];
await prepareComposeBox(tester, narrow: groupDmNarrowWith(deactivatedUsers),
users: deactivatedUsers);
otherUsers: deactivatedUsers);
checkComposeBox(isShown: false);

await changeUserStatus(tester, user: deactivatedUsers[0], isActive: true);
Expand Down