Skip to content

Commit

Permalink
api: Add user_settings/update events, with a test for exhaustiveness
Browse files Browse the repository at this point in the history
It's been tricky to find a way to verify that the event-handling
code keeps up with the settings we add in [UserSettings], the data
class we use in the initial snapshot. See #261 for some alternatives
we considered.

But at least this solution works, with type-checking of the event at
the edge, and a mechanism to ensure that all user settings we store
in our initial snapshot get updated by the user_settings/update
event.
  • Loading branch information
chrisbobbe authored and gnprice committed Aug 14, 2023
1 parent ee0504a commit 9cb2e4d
Show file tree
Hide file tree
Showing 6 changed files with 149 additions and 4 deletions.
53 changes: 53 additions & 0 deletions lib/api/model/events.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import 'package:json_annotation/json_annotation.dart';

import 'initial_snapshot.dart';
import 'model.dart';

part 'events.g.dart';
Expand All @@ -16,6 +17,11 @@ sealed class Event {
factory Event.fromJson(Map<String, dynamic> json) {
switch (json['type'] as String) {
case 'alert_words': return AlertWordsEvent.fromJson(json);
case 'user_settings':
switch (json['op'] as String) {
case 'update': return UserSettingsUpdateEvent.fromJson(json);
default: return UnexpectedEvent.fromJson(json);
}
case 'realm_user':
switch (json['op'] as String) {
case 'add': return RealmUserAddEvent.fromJson(json);
Expand Down Expand Up @@ -74,6 +80,53 @@ class AlertWordsEvent extends Event {
Map<String, dynamic> toJson() => _$AlertWordsEventToJson(this);
}

/// A Zulip event of type `user_settings` with op `update`.
@JsonSerializable(fieldRename: FieldRename.snake)
class UserSettingsUpdateEvent extends Event {
@override
@JsonKey(includeToJson: true)
String get type => 'user_settings';

@JsonKey(includeToJson: true)
String get op => 'update';

/// The name of the setting, or null if we don't recognize it.
@JsonKey(unknownEnumValue: JsonKey.nullForUndefinedEnumValue)
final UserSettingName? property;

/// The new value, or null if we don't recognize the setting.
///
/// This will have the type appropriate for [property]; for example,
/// if the setting is boolean, then `value is bool` will always be true.
/// This invariant is enforced by [UserSettingsUpdateEvent.fromJson].
@JsonKey(readValue: _readValue)
final Object? value;

/// [value], with a check that its type corresponds to [property]
/// (e.g., `value as bool`).
static Object? _readValue(Map json, String key) {
final value = json['value'];
switch (UserSettingName.fromRawString(json['property'] as String)) {
case UserSettingName.displayEmojiReactionUsers:
return value as bool;
case null:
return null;
}
}

UserSettingsUpdateEvent({
required super.id,
required this.property,
required this.value,
});

factory UserSettingsUpdateEvent.fromJson(Map<String, dynamic> json) =>
_$UserSettingsUpdateEventFromJson(json);

@override
Map<String, dynamic> toJson() => _$UserSettingsUpdateEventToJson(this);
}

/// A Zulip event of type `realm_user`.
///
/// The corresponding API docs are in several places for
Expand Down
23 changes: 23 additions & 0 deletions lib/api/model/events.g.dart

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

35 changes: 32 additions & 3 deletions lib/api/model/initial_snapshot.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'package:flutter/foundation.dart';
import 'package:json_annotation/json_annotation.dart';

import 'model.dart';
Expand Down Expand Up @@ -116,11 +117,14 @@ class RecentDmConversation {
///
/// For docs, search for "user_settings:"
/// in <https://zulip.com/api/register-queue>.
@JsonSerializable(fieldRename: FieldRename.snake)
@JsonSerializable(fieldRename: FieldRename.snake, createFieldMap: true)
class UserSettings {
final bool? displayEmojiReactionUsers; // TODO(server-6)
bool? displayEmojiReactionUsers; // TODO(server-6)

// TODO more, as needed
// TODO more, as needed. When adding a setting here, please also:
// (1) add it to the [UserSettingName] enum below
// (2) then re-run the command to refresh the .g.dart files
// (3) handle the event that signals an update to the setting

UserSettings({
required this.displayEmojiReactionUsers,
Expand All @@ -130,4 +134,29 @@ class UserSettings {
_$UserSettingsFromJson(json);

Map<String, dynamic> toJson() => _$UserSettingsToJson(this);

/// A list of [UserSettings]'s properties, as strings.
// _$…FieldMap is thanks to `createFieldMap: true`
@visibleForTesting
static final Iterable<String> debugKnownNames = _$UserSettingsFieldMap.keys;
}

/// The name of a user setting that has a property in [UserSettings].
///
/// In Zulip event-handling code (for [UserSettingsUpdateEvent]),
/// we switch exhaustively on a value of this type
/// to ensure that every setting in [UserSettings] responds to the event.
@JsonEnum(fieldRename: FieldRename.snake, alwaysCreate: true)
enum UserSettingName {
displayEmojiReactionUsers;

/// Get a [UserSettingName] from a raw, snake-case string we recognize, else null.
///
/// Example:
/// 'display_emoji_reaction_users' -> UserSettingName.displayEmojiReactionUsers
static UserSettingName? fromRawString(String raw) => _byRawString[raw];

// _$…EnumMap is thanks to `alwaysCreate: true` and `fieldRename: FieldRename.snake`
static final _byRawString = _$UserSettingNameEnumMap
.map((key, value) => MapEntry(value, key));
}
8 changes: 8 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.

13 changes: 12 additions & 1 deletion lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ class PerAccountStore extends ChangeNotifier {
final int maxFileUploadSizeMib; // No event for this.

// Data attached to the self-account on the realm.
final UserSettings? userSettings; // TODO(#135) update with user_settings/update event
final UserSettings? userSettings; // TODO(server-5)

// Users and data about them.
final Map<int, User> users;
Expand Down Expand Up @@ -219,6 +219,17 @@ class PerAccountStore extends ChangeNotifier {
} else if (event is AlertWordsEvent) {
assert(debugLog("server event: alert_words"));
// We don't yet store this data, so there's nothing to update.
} else if (event is UserSettingsUpdateEvent) {
assert(debugLog("server event: user_settings/update ${event.property?.name ?? '[unrecognized]'}"));
if (event.property == null) {
// unrecognized setting; do nothing
return;
}
switch (event.property!) {
case UserSettingName.displayEmojiReactionUsers:
userSettings?.displayEmojiReactionUsers = event.value as bool;
}
notifyListeners();
} else if (event is RealmUserAddEvent) {
assert(debugLog("server event: realm_user/add"));
users[event.person.userId] = event.person;
Expand Down
21 changes: 21 additions & 0 deletions test/api/model/events_test.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'package:checks/checks.dart';
import 'package:test/scaffolding.dart';
import 'package:zulip/api/model/events.dart';
import 'package:zulip/api/model/initial_snapshot.dart';

import '../../example_data.dart' as eg;
import '../../stdlib_checks.dart';
Expand All @@ -20,4 +21,24 @@ void main() {
check(mkEvent([])).message.flags.deepEquals([]);
check(mkEvent(['read'])).message.flags.deepEquals(['read']);
});

test('user_settings: all known settings have event handling', () {
final dataClassFieldNames = UserSettings.debugKnownNames;
final enumNames = UserSettingName.values.map((n) => n.name);
final missingEnumNames = dataClassFieldNames.where((key) => !enumNames.contains(key)).toList();
check(
missingEnumNames,
because:
'You have added these fields to [UserSettings]\n'
'without handling the corresponding forms of the\n'
'user_settings/update event in [PerAccountStore]:\n'
' $missingEnumNames\n'
'To do that, please follow these steps:\n'
' (1) Add corresponding members to the [UserSettingName] enum.\n'
' (2) Then, re-run the command to refresh the .g.dart files.\n'
' (3) Resolve the Dart analysis errors about not exhaustively\n'
' matching on that enum, by adding new `switch` cases\n'
' on the pattern of the existing cases.'
).isEmpty();
});
}

0 comments on commit 9cb2e4d

Please sign in to comment.