Skip to content
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

Respect realm setting for mandatory topics #1296

Merged
merged 5 commits into from
Jan 30, 2025
Merged
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
3 changes: 3 additions & 0 deletions lib/api/model/initial_snapshot.dart
Original file line number Diff line number Diff line change
@@ -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.
@@ -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,
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.

3 changes: 3 additions & 0 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
@@ -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,
@@ -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,
@@ -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.
39 changes: 24 additions & 15 deletions lib/widgets/compose_box.dart
Original file line number Diff line number Diff line change
@@ -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;
@@ -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
@@ -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();
@@ -1330,7 +1338,7 @@ class _ComposeBoxState extends State<ComposeBox> implements ComposeBoxState {

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

@@ -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);
}
}

2 changes: 2 additions & 0 deletions test/example_data.dart
Original file line number Diff line number Diff line change
@@ -856,6 +856,7 @@ InitialSnapshot initialSnapshot({
List<ZulipStream>? streams,
UserSettings? userSettings,
List<UserTopicItem>? userTopics,
bool? realmMandatoryTopics,
int? realmWaitingPeriodThreshold,
Map<String, RealmDefaultExternalAccount>? realmDefaultExternalAccounts,
int? maxFileUploadSizeMib,
@@ -890,6 +891,7 @@ InitialSnapshot initialSnapshot({
emojiset: Emojiset.google,
),
userTopics: userTopics,
realmMandatoryTopics: realmMandatoryTopics ?? true,
realmWaitingPeriodThreshold: realmWaitingPeriodThreshold ?? 0,
realmDefaultExternalAccounts: realmDefaultExternalAccounts ?? {},
maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25,
3 changes: 2 additions & 1 deletion test/model/autocomplete_test.dart
Original file line number Diff line number Diff line change
@@ -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();
1 change: 1 addition & 0 deletions test/model/store_checks.dart
Original file line number Diff line number Diff line change
@@ -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');
3 changes: 3 additions & 0 deletions test/model/test_store.dart
Original file line number Diff line number Diff line change
@@ -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;
75 changes: 65 additions & 10 deletions test/widgets/compose_box_test.dart
Original file line number Diff line number Diff line change
@@ -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),
@@ -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;

@@ -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(
@@ -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);
@@ -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);
@@ -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);
@@ -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);