Skip to content

Commit b3fa397

Browse files
committed
api: Simplify move data parsing
This drops the separate if-statements in favor of null-checks and tries to rely on the natural semantics of the API. This drops an expcetion that would have been thrown when propagate mode is not present but the topic name/stream id changes. See Greg's comment on this: zulip#1311 (comment) Signed-off-by: Zixuan James Li <[email protected]>
1 parent 4bf6503 commit b3fa397

File tree

2 files changed

+15
-42
lines changed

2 files changed

+15
-42
lines changed

lib/api/model/events.dart

+13-30
Original file line numberDiff line numberDiff line change
@@ -717,7 +717,7 @@ class UpdateMessageMoveData {
717717
required this.propagateMode,
718718
required this.origTopic,
719719
required this.newTopic,
720-
});
720+
}) : assert(origStreamId != newStreamId || origTopic != newTopic);
721721

722722
/// Try to extract [UpdateMessageMoveData] from the JSON object for an
723723
/// [UpdateMessageEvent].
@@ -730,50 +730,33 @@ class UpdateMessageMoveData {
730730
// This may matter if we ever need 'stream_id' when no message move occurred.
731731
static UpdateMessageMoveData? tryParseFromJson(Object? json) {
732732
json as Map<String, Object?>;
733-
final origStreamId = (json['stream_id'] as num?)?.toInt();
734-
final newStreamId = (json['new_stream_id'] as num?)?.toInt();
735733
final propagateModeString = json['propagate_mode'] as String?;
736734
final propagateMode = propagateModeString == null ? null
737735
: PropagateMode.fromRawString(propagateModeString);
738-
final origTopic = json['orig_subject'] == null ? null
739-
: TopicName.fromJson(json['orig_subject'] as String);
740-
final newTopic = json['subject'] == null ? null
741-
: TopicName.fromJson(json['subject'] as String);
742736

743-
if (origTopic == null) {
737+
if (propagateMode == null) {
744738
// There was no move.
745-
assert(() {
746-
if (newStreamId != null && origStreamId != null
747-
&& newStreamId != origStreamId) {
748-
// This should be impossible; `orig_subject` (aka origTopic) is
749-
// documented to be present when either the stream or topic changed.
750-
throw FormatException('stream move but no origTopic');
751-
}
752-
return true;
753-
}());
754739
return null;
755740
}
756741

757-
if (newStreamId == null && newTopic == null) {
742+
final origStreamId = (json['stream_id'] as num).toInt();
743+
final newStreamId = (json['new_stream_id'] as num?)?.toInt() ?? origStreamId;
744+
final origTopic = TopicName.fromJson(json['orig_subject'] as String);
745+
final newTopic = json['subject'] == null ? origTopic
746+
: TopicName.fromJson(json['subject'] as String);
747+
748+
if (origStreamId == newStreamId && origTopic == newTopic) {
758749
// If neither the channel nor topic name changed, nothing moved.
759-
// In that case `orig_subject` (aka origTopic) should have been null.
760-
throw FormatException('move but no newStreamId or newTopic');
761-
}
762-
if (origStreamId == null) {
763-
// The `stream_id` field (aka origStreamId) is documented to be present on moves.
764-
throw FormatException('move but no origStreamId');
765-
}
766-
if (propagateMode == null) {
767-
// The `propagate_mode` field (aka propagateMode) is documented to be present on moves.
768-
throw FormatException('move but no propagateMode');
750+
// In that case `propagate_mode` (aka propagateMode) should have been null.
751+
throw FormatException('move but unchanged newStreamId and newTopic');
769752
}
770753

771754
return UpdateMessageMoveData(
772755
origStreamId: origStreamId,
773-
newStreamId: newStreamId ?? origStreamId,
756+
newStreamId: newStreamId,
774757
propagateMode: propagateMode,
775758
origTopic: origTopic,
776-
newTopic: newTopic ?? origTopic,
759+
newTopic: newTopic,
777760
);
778761
}
779762

test/api/model/events_test.dart

+2-12
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ void main() {
161161
'orig_subject': null,
162162
'subject': null,
163163
'propagate_mode': 'change_all',
164-
})).throws<FormatException>();
164+
})).throws<void>();
165165
});
166166

167167
test('move but no subject or new_stream_id', () {
@@ -181,17 +181,7 @@ void main() {
181181
'orig_subject': 'foo',
182182
'subject': 'bar',
183183
'propagate_mode': 'change_all',
184-
})).throws<FormatException>();
185-
});
186-
187-
test('move but no propagate_mode', () {
188-
check(() => Event.fromJson({...baseJson,
189-
'stream_id': 1,
190-
'new_stream_id': 2,
191-
'orig_subject': 'foo',
192-
'subject': 'bar',
193-
'propagate_mode': null,
194-
})).throws<FormatException>();
184+
})).throws<void>();
195185
});
196186
});
197187

0 commit comments

Comments
 (0)