compose_box: Replace compose box with a banner when cannot post in a channel

[sm-sayedi]

When a user cannot post in a channel based on ZulipStream.channelPostPolicy, all parts of the compose box (in both channel and topic narrow) are replaced with a banner, saying: You do not have permission to post in this channel.

Screenshot

Screen recording

channel-policy-based.compose.box.mp4

Fixes: #674

[rajveermalviya]

Thanks for working on this @sm-sayedi, this looks great!

Moving on to the mentor review from @hackerkid.

[chrisbobbe]

Thanks! Comments below, covering everything except the tests added in the main, last commit:

compose_box: Replace compose box with a banner when cannot post in a channel

because I've suggested a cleanup that should help me review those tests more efficiently; see below. 🙂

[chrisbobbe]

Suggested change

	/// Search for "realm_waiting_period_threshold" in https://zulip.com/api/register-queue.
	///
	/// For how to determine if a user is a full member, see:
	/// Search for "realm_waiting_period_threshold" in https://zulip.com/api/register-queue.
	///
	/// For how to determine if a user is a full member, see:

[chrisbobbe]

Suggested change

	final int realmWaitingPeriodThreshold;
	final int realmWaitingPeriodThreshold; // TODO(#668): update this realm setting

[chrisbobbe]

This method's name isn't quite a match for what it does. Compare zulip-mobile:

/**
 * Whether the user has passed the realm's waiting period to be a full member.
 *
 * See:
 *   https://zulip.com/api/roles-and-permissions#determining-if-a-user-is-a-full-member
 *
 * To determine if a user is a full member, callers must also check that the
 * user's role is at least Role.Member.
 *
 * […]
 */
export function getHasUserPassedWaitingPeriod(state: PerAccountState, userId: UserId): boolean {
  const { waitingPeriodThreshold } = getRealm(state);
  const { date_joined } = getUserForId(state, userId);

  const intervalLengthInDays = (Date.now() - Date.parse(date_joined)) / 86400_000;

  // […]
  // TODO(?): […]
  return intervalLengthInDays >= waitingPeriodThreshold;
}

How about we call it hasPassedWaitingPeriod, and give it a dartdoc along the lines of the jsdoc in zulip-mobile.

[chrisbobbe]

I think a helpful simplification here would be to add a method on UserRole:

  bool isAtLeast(UserRole threshold) {

which could be used here, but also anywhere else we need to do role checks. In zulip-mobile it's simple:

export function roleIsAtLeast(thisRole: Role, thresholdRole: Role): boolean {
  return (thisRole: number) <= (thresholdRole: number); // Roles with more privilege have lower numbers.
}

I think I remember concluding that "roles with more privilege have lower numbers" is basically an API guarantee. It would be pretty odd if it weren't guaranteed, given the values in the current API.

And with that encapsulated in UserRole, I think I'd feel pretty comfortable making an improvement over the logic here: when servers give an API value we don't recognize—say, 350, which would be between 300 "moderator" and 400 "member"—we could still store that value, and use it in the new isAtLeast method. (I'd be less comfortable passing around apiValue values outside the UserRole implementation itself, but it seems very appropriate to do within it.)

[chrisbobbe]

This means exactly the same thing as realmWaitingPeriodThreshold, right? Let's just call it realmWaitingPeriodThreshold, so we don't have to think about another name and decide if it means something subtly different.

[chrisbobbe]

Instead of calling DateTime.now() inside this API-model code, how about we compute it in UI code and pass the value down to here? A possible post-launch refinement might be to recheck, at regular time intervals, if enough time has passed that the user has become a full member. The natural place to do that will be near the UI code responsible for choosing whether to show the error banner.

(This caused me to think of a small, similar improvement we'll want to make eventually. I've filed that just now as #891.)

[chrisbobbe]

Suggested change

	"description": "Label text for error banner when sending a message in a channel with no posting permission."
	"description": "Error-banner text replacing the compose box when you do not have permission to send a message to the channel."

[chrisbobbe]

There's already an _errorBanner method, in _FixedDestinationComposeBoxState, and it duplicates a lot of this logic. Can we centralize the computation, perhaps in an early-return style in the build method of ComposeBox?

[chrisbobbe]

compose_box: Replace compose box with a banner when cannot post in a channel

This seems like a reasonable change to the action-sheet tests, but why is it needed in this commit, which is about the compose box?

…Ah, I think I understand: it's a boring but necessary bit of setup so that the simulated action sheet doesn't crash in the code that decides whether to show the error banner. Is that right?

There are quite a few other changes in this commit that look like they're made for the same reason. Would you move those to a prep commit before this commit? That should make it easier to focus on the interesting changes here. 🙂

[sm-sayedi]

Thanks @chrisbobbe for the review! Revision pushed with the tests cleaned up. PTAL!

[chrisbobbe]

Thanks! Ah, it looks like this has gathered some conflicts—would you mind rebasing and resolving those please?

[sm-sayedi]

Conflicts resolved @chrisbobbe! Please have a look!

[chrisbobbe]

Thanks! Comments below, all small.

[chrisbobbe]

If not implementing it now, let's add a TODO for this part that I mentioned at #886 (comment) :

[…] when servers give an API value we don't recognize—say, 350, which would be between 300 "moderator" and 400 "member"—we could still store that value, and use it in the new isAtLeast method. (I'd be less comfortable passing around apiValue values outside the UserRole implementation itself, but it seems very appropriate to do within it.)

In this revision, where we treat all unrecognized values as though they were 0, we'll give wrong results when the unrecognized role from the server is meant to be less privileged than the threshold role.

[sm-sayedi]

when servers give an API value we don't recognize—say, 350, which would be between 300 "moderator" and 400 "member"—we could still store that value, and use it in the new isAtLeast method.

I think in the current code for UserRole we get UserRole.unknown for all the unrecognized API values. With that being said, we cannot get that new unrecognized value to compare it with the threshold apiValue. 🙂

[chrisbobbe]

The proposal is to store the unrecognized API value so that isAtLeast can get its hands on it :). Looking closer, I think that would need some preparation, though, such as converting UserRole from an enum to a plain class.

Greg reminds me that we don't actually expect any more API values to be added, though, because the "role" concept is on track to be replaced by "user groups". So the work I've described for handling unrecognized values won't be worth it.

[chrisbobbe]

:)

[chrisbobbe]

To make this a bit easier to scan:

Suggested change

	final testCases = [
	(ChannelPostPolicy.unknown, UserRole.unknown, true),
	(ChannelPostPolicy.unknown, UserRole.guest, true),
	(ChannelPostPolicy.unknown, UserRole.member, true),
	(ChannelPostPolicy.unknown, UserRole.moderator, true),
	(ChannelPostPolicy.unknown, UserRole.administrator, true),
	(ChannelPostPolicy.unknown, UserRole.owner, true),
	(ChannelPostPolicy.any, UserRole.unknown, true),
	(ChannelPostPolicy.any, UserRole.guest, true),
	(ChannelPostPolicy.any, UserRole.member, true),
	(ChannelPostPolicy.any, UserRole.moderator, true),
	(ChannelPostPolicy.any, UserRole.administrator, true),
	(ChannelPostPolicy.any, UserRole.owner, true),
	(ChannelPostPolicy.fullMembers, UserRole.unknown, true),
	(ChannelPostPolicy.fullMembers, UserRole.guest, false),
	(ChannelPostPolicy.fullMembers, UserRole.member, true),
	(ChannelPostPolicy.fullMembers, UserRole.moderator, true),
	(ChannelPostPolicy.fullMembers, UserRole.administrator, true),
	(ChannelPostPolicy.fullMembers, UserRole.owner, true),
	(ChannelPostPolicy.moderators, UserRole.unknown, true),
	(ChannelPostPolicy.moderators, UserRole.guest, false),
	(ChannelPostPolicy.moderators, UserRole.member, false),
	(ChannelPostPolicy.moderators, UserRole.moderator, true),
	(ChannelPostPolicy.moderators, UserRole.administrator, true),
	(ChannelPostPolicy.moderators, UserRole.owner, true),
	(ChannelPostPolicy.administrators, UserRole.unknown, true),
	(ChannelPostPolicy.administrators, UserRole.guest, false),
	(ChannelPostPolicy.administrators, UserRole.member, false),
	(ChannelPostPolicy.administrators, UserRole.moderator, false),
	(ChannelPostPolicy.administrators, UserRole.administrator, true),
	(ChannelPostPolicy.administrators, UserRole.owner, true),
	];
	final testCases = [
	(ChannelPostPolicy.unknown, UserRole.unknown, true),
	(ChannelPostPolicy.unknown, UserRole.guest, true),
	(ChannelPostPolicy.unknown, UserRole.member, true),
	(ChannelPostPolicy.unknown, UserRole.moderator, true),
	(ChannelPostPolicy.unknown, UserRole.administrator, true),
	(ChannelPostPolicy.unknown, UserRole.owner, true),
	(ChannelPostPolicy.any, UserRole.unknown, true),
	(ChannelPostPolicy.any, UserRole.guest, true),
	(ChannelPostPolicy.any, UserRole.member, true),
	(ChannelPostPolicy.any, UserRole.moderator, true),
	(ChannelPostPolicy.any, UserRole.administrator, true),
	(ChannelPostPolicy.any, UserRole.owner, true),
	(ChannelPostPolicy.fullMembers, UserRole.unknown, true),
	(ChannelPostPolicy.fullMembers, UserRole.guest, false),
	(ChannelPostPolicy.fullMembers, UserRole.member, true),
	(ChannelPostPolicy.fullMembers, UserRole.moderator, true),
	(ChannelPostPolicy.fullMembers, UserRole.administrator, true),
	(ChannelPostPolicy.fullMembers, UserRole.owner, true),
	(ChannelPostPolicy.moderators, UserRole.unknown, true),
	(ChannelPostPolicy.moderators, UserRole.guest, false),
	(ChannelPostPolicy.moderators, UserRole.member, false),
	(ChannelPostPolicy.moderators, UserRole.moderator, true),
	(ChannelPostPolicy.moderators, UserRole.administrator, true),
	(ChannelPostPolicy.moderators, UserRole.owner, true),
	(ChannelPostPolicy.administrators, UserRole.unknown, true),
	(ChannelPostPolicy.administrators, UserRole.guest, false),
	(ChannelPostPolicy.administrators, UserRole.member, false),
	(ChannelPostPolicy.administrators, UserRole.moderator, false),
	(ChannelPostPolicy.administrators, UserRole.administrator, true),
	(ChannelPostPolicy.administrators, UserRole.owner, true),
	];

[chrisbobbe]

Suggested change

	group('only "full member" user can post in channel with "fullMembers" policy', (){
	group('only "full member" user can post in channel with "fullMembers" policy', () {

[chrisbobbe]

Suggested change

	});
	}
	});
	}

[chrisbobbe]

Suggested change

	streams: [eg.stream(streamId: 1, channelPostPolicy: policy)],
	);
	streams: [eg.stream(streamId: 1, channelPostPolicy: policy)]);

(here and in in many places later in this file)

[sm-sayedi]

Thanks @chrisbobbe for the review. Pushed the new changes. Also added #886 comment. PTAL.

cc @PIG208

[PIG208]

Looks like this should come before maxFileUploadSizeMib.

[sm-sayedi]

Moved it before the line for maxFileUploadSizeMib.

Also, looking at these "realm and server" section properties, I'm unsure if they follow a specific order. If they're meant to follow the order of InitialSnapshot, then they'll have an order like this:

customProfileFields
realmWaitingPeriodThreshold
realmUrl
realmDefaultExternalAccounts
maxFileUploadSizeMib
emailAddressVisiblity

[gnprice]

The constructor arguments at this call site should match the order of the parameters in the constructor's declaration, which in turn should match the order the fields are declared in.

For the field declarations, generally we've tried to keep them in a logical order (which is our default approach for ordering things). So realmUrl comes before all the miscellaneous settings, because it's the value that identifies the whole realm in the first place. I don't think there's a strong reason the other five you listed are ordered in the way they currently are, though.

That logical ordering may differ from what's in InitialSnapshot, because:

class InitialSnapshot {
  // Keep these fields in the order they appear in the API docs.
  // (For many API types we choose a more logical order than the docs.
  // But this one is so long that that'd make it become impossible to
  // compare the lists by hand.)

[PIG208]

We could simplify this by parsing dateJoined as we deserialize the User JSON object.

[gnprice]

We could, but I'd prefer to keep that parsing deferred: converting user objects from JSON is a hot spot performance-wise, because there can be so many of them, and parsing a DateTime is the sort of thing that I feel could easily be slow.

And conversely I think there's only a few places like here where we care about the value of dateJoined, so it's not much burden on code complexity for these to have to handle the parsing; and they're all for just one user at a time, so there's no performance concern.

[PIG208]

nit:

Suggested change

	int realmWaitingPeriodThreshold = 0,
	int? realmWaitingPeriodThreshold,

which allows us to reuse the default on eg.initialSnapshot.

[PIG208]

Looks like this belongs to the previous commit.

[PIG208]

Looking at this, I was thinking about why 0 is chosen as the default value for both operands. Considering the context this helper is used, I suppose that we want to fallback to the behavior that, when the role has an unknown value, we allow the user to post.

This consideration is specific to the posting permission feature we are implementing here. I think the best thing to do is to assert that both api values are non-null; then we handle the case when either of them is null in hasPostingPermission.

[sm-sayedi]

Right. If we're going to allow the user to post when threshold.apiValue == null, then it should be:

return (apiValue ?? 0) <= (threshold.apiValue ?? double.maxFinite);

Also, as Chris mentioned in #886 comment, we're not going to expect any new values thus none of these will become null.

[PIG208]

nit: let's extract ZulipLocalizations.of(context) as a variable

[PIG208]

Then we can simplify this to:

Suggested change

	case ChannelNarrow narrow:
	final channel = store.streams[narrow.streamId]!;
	return channel.hasPostingPermission(selfUser, byDate: DateTime.now(), realmWaitingPeriodThreshold: store.realmWaitingPeriodThreshold)
	? null : _ErrorBanner(label: ZulipLocalizations.of(context).errorBannerCannotPostInChannelLabel);
	case TopicNarrow narrow:
	final channel = store.streams[narrow.streamId]!;
	return channel.hasPostingPermission(selfUser, byDate: DateTime.now(), realmWaitingPeriodThreshold: store.realmWaitingPeriodThreshold)
	? null : _ErrorBanner(label: ZulipLocalizations.of(context).errorBannerCannotPostInChannelLabel);
	case ChannelNarrow(:final streamId):
	case TopicNarrow(:final streamId):
	final channel = store.streams[streamId]!;
	if (channel.hasPostingPermission(selfUser, byDate: DateTime.now(),
	realmWaitingPeriodThreshold: store.realmWaitingPeriodThreshold)) {
	return _ErrorBanner(
	label: localizations.errorBannerCannotPostInChannelLabel);
	}

[PIG208]

nit: to make the lines shorter:

Suggested change

	return hasDeactivatedUser ? _ErrorBanner(label: ZulipLocalizations.of(context)
	.errorBannerDeactivatedDmLabel) : null;
	if (hasDeactivatedUser) {
	return _ErrorBanner(
	label: localizations.errorBannerDeactivatedDmLabel);
	}

[PIG208]

Looks like we are both moving tests to groups and adding the new tests.
The grouping part can be done in a prep commit so the diffs will be easier to read.

[PIG208]

nit:

Suggested change

	for (final testCase in testCases) {
	final (ChannelPostPolicy policy, UserRole role, bool canPost) = testCase;
	for (final (ChannelPostPolicy policy, UserRole role, bool canPost) in testCases) {

[PIG208]

Looks good. Thanks! Moving this to Greg's review.

[gnprice]

Thanks @sm-sayedi for building this, and thanks @chrisbobbe and @PIG208 for the previous reviews!

Generally this looks good; comments below. I've read the first 8 of the 9 commits in full, and all but the tests of the final main commit.

[gnprice]

Let's have this, and the related hasPostingPermission method, live in the model code instead of the API code. That way they can find for themselves the contextual information they need, like realmWaitingPeriodThreshold.

That means they'll be methods on PerAccountStore, instead of on User or ZulipStream. That's slightly less convenient for hasPassedWaitingPeriod, but it's fine. And for the other method, "has posting permission" method is really a fact about the user just as much as it is about the stream/channel, so having it live on either one of those is a little awkward anyway.

For an example of the desired pattern, see isTopicVisible and isTopicVisibleInStream.

These will be slightly different from those just in the way the code is organized, because we don't have a UserStore yet. For now these method implementations can just live directly on PerAccountStore. They can go at the end of the "Users and data about them" section and the "Streams, topics, and stuff about them" section, respectively.

[gnprice]

What do these date_joined strings look like from the server? It seems that's not really documented — the docs just say:

The time the user account was created.

and that it's a string.

If the string doesn't have an explicit timezone, DateTime.parse will default to interpreting it as the client's local time. That may differ from the intended value by a large fraction of a day, which is substantial given that the threshold is only like 3 days.

[gnprice]

In general we should arrange all our handling of dates and times so that it's clear whether it's in UTC, or the local time zone, or some other time zone — otherwise it's easy to end up with subtle bugs from mismatches.

And where possible we should keep things in UTC, because that's the one canonical time for interchange between devices that have different local times, and because it's a time zone that is guaranteed never to have wrinkles like a day that is 23 hours or 25 hours long.

[sm-sayedi]

What do these date_joined strings look like from the server?

It looks like they're in the UTC format. For my account it is 2024-02-24T11:18+00:00, although my time zone offset is +04:30. Even if it has an offset other than +00:00, DateTime.parse will still convert it to its equivalent UTC time. This is the relevant part of its documentation:

The result is always in either local time or UTC.
If a time zone offset other than UTC is specified,
the time is converted to the equivalent UTC time.

[gnprice]

Great, that works, then. Would you start a thread in #api documentation about making that format explicit in the docs?

Even if it has an offset other than +00:00, DateTime.parse will still convert it to its equivalent UTC time.

Yeah, which would be fine. But if it didn't have an explicit offset at all, I believe DateTime.parse would interpret it as the client's local time, which would be the wrong result (unless the client's time zone happened to coincide with UTC).

[sm-sayedi]

Yeah sure.

...I believe DateTime.parse would interpret it as the client's local time...

Yeah, looking at its implementation and some logging, it is in the client's local time.

[gnprice]

This logic is pretty deeply indented — a lot of the important information is past 80 columns, so should be brought to the left of that threshold.

I think this would also become easier to read by breaking it out into statements with control flow.

[gnprice]

Related to my comment about time zones: this method should get unit tests. That way we can be sure to exercise the various corner cases.

Other than time zones, one class of corner cases to test is: the behavior should be in terms of "at least N days" meaning "at least 24 * N hours", not meaning "spanning at least N midnights". I believe this implementation is correct — but only after I went and looked at the docs of these difference and inDays methods. It'd be easy to have a bug of that kind, so good to test.

[gnprice]

Similarly, this method should also get unit tests.

[gnprice]

This can be tightened with a fun use of pattern-matching:

Suggested change

	if (narrow is ChannelNarrow || narrow is TopicNarrow) {
	final channelId = narrow is ChannelNarrow ? narrow.streamId : (narrow as TopicNarrow).streamId;
	if (narrow case ChannelNarrow(:var streamId) || TopicNarrow(:var streamId)) {

[gnprice]

Because the same self-user object is needed below, best to do the ?? eg.selfUser fallback just once, to avoid accidental mismatch on future changes to this code. So:

Suggested change

	final account = eg.account(user: selfUser ?? eg.selfUser);
	selfUser ??= eg.selfUser;
	final account = eg.account(user: selfUser);

[gnprice]

compose_box test [nfc]: Have `prepareComposeBox` accept additional params

The additional params are: selfUser, streams, and realmWaitingPeriodThreshold.

Let's squash the streams part into the commit that needs that (which is the next one), and the other two into the commit that needs those (which is the main commit at the end).

That way they're each appearing in the context of the changes that show how and why they're used.

The changes are also basically independent of each other but are happening here in closely neighboring lines of code; so having them in separate commits from each other will I think make both changes easier to read. (The commits they'll be getting squashed into don't have any other changes in this particular spot of the code.)

[gnprice]

This will crash if we find ourselves at a narrow for a stream/channel we don't know about.

We should avoid crashing in that case. For example it can happen if the user tries to follow a link that goes to a channel they don't have access to.

The principle is the same as the one described for users in #716. If you look at the other references to store.streams in this file, you'll see they follow that principle.

[gnprice]

These can be adapted to produce those unit tests that hasPostingPermission should get.

Then once that has unit tests, the tests for this UI code can be simpler: they only need to exercise the cases that are different as far as the UI code sees things, not cases that differ only within the implementation of the model method. So e.g. cases for a channel narrow and a topic narrow, and needing a banner and not needing one; but not for all the nuances of when one is needed and when it isn't.

[sm-sayedi]

Related to #886 comment, I wrote the tests only for the UTC format as DateTime only supports local or UTC time zone. Even if I wrote some tests for my local time zone, they'd have different (most probably incorrect) results for anyone else running the tests in their local time zone. I couldn't find a way to force the test environment to have a different time zone other than (my) local or UTC.

[gnprice]

Yeah, since we're doing the computations all in terms of UTC (which is good news), that's the right way for the tests to work too.

I couldn't find a way to force the test environment to have a different time zone other than (my) local or UTC.

Unfortunately there doesn't seem to be such a way. We have a comment on one test where that came up, in test/widgets/message_list_test.dart (testing the dates we show in recipient headers):

      // We show the dates in the user's timezone.  Dart's standard library
      // doesn't give us a way to control which timezone is used — only to
      // choose between UTC and the user's timezone, whatever that may be.
      // So we do the latter, and that means these dates come out differently
      // depending on the timezone in the environment running the tests.
      // Related upstream issues:
      //   https://github.com/dart-lang/sdk/issues/28985 (about DateTime.now, not timezone)
      //   https://github.com/dart-lang/sdk/issues/44928 (about the Dart implementation's own internal tests)
      // For this test, just accept outputs corresponding to any possible timezone.
      tester.widget(find.textContaining(RegExp("Dec 1[89], 2022")));
      tester.widget(find.textContaining(RegExp("Aug 2[23], 2022")));

[sm-sayedi]

I am not sure if the verbs "decrease" and "increase" are the proper ones to use here! 😃 I thought a lot about them, but couldn't come up with better ones. Any suggestions would be appreciated!

[gnprice]

Yeah, good question. Let's say "user loses privilege" and "user gains privilege".

Saying "increases" and "decreases" is particularly ambiguous because of the oddity noted in UserRole.isAtLeast, where roles with greater privilege are encoded as smaller numbers.

[sm-sayedi]

Thanks @gnprice for the previous review. New changes pushed. PTAL.

Also added a few comments just above and one in the previous review sub-thread.

[gnprice]

Thanks for the revision! Generally this all looks good; various small comments below.

[gnprice]

nit: use linkifying syntax for reference:

Suggested change

	/// user's role is at least `UserRole.member`.
	/// user's role is at least [UserRole.member].

[gnprice]

For one of the cases here, this test relies on some facts about the test data which it doesn't mention or assure itself: it needs the current time to be after the example user's dateJoined, by at least the number of days specified in realmWaitingPeriodThreshold in the example snapshot.

Handling that properly requires a bit of setup, which you take care of below. So the right fix is to just not include that case here, so as not to complicate these other cases.

[gnprice]

Concretely, for my previous comment:

Suggested change

	(ChannelPostPolicy.fullMembers, UserRole.member, true),
	// The fullMembers/member case gets its own tests further below.
	// (ChannelPostPolicy.fullMembers, UserRole.member, /* complicated */),

[gnprice]

nit: move this line inside the error cases

That way we're not doing the lookup in the common case where there's no error, when we only need it in the case where there's an error.

(This lookup should be cheap anyway. But I prefer to make a habit of keeping the non-error path clear of work that's needed only for error cases.)

[gnprice]

Yeah, good question. Let's say "user loses privilege" and "user gains privilege".

Saying "increases" and "decreases" is particularly ambiguous because of the oddity noted in UserRole.isAtLeast, where roles with greater privilege are encoded as smaller numbers.

[gnprice]

nit: I think this is clearer if it just inlines the definition of changeUserRole. It's only three lines, and pretty straightforward.

Similarly for changeChannelPolicy.

[gnprice]

nit: line too long — the key information is past 80 columns

(similarly the initial channelPostPolicy value above)

[gnprice]

You've already taken the trouble to write these four test cases so we may as well include them here, but for future reference: I typically wouldn't ask for tests to check that updating the data in the store causes the widgets to properly update for the new values. In general we can just rely on the assumption that Flutter's rebuild mechanisms work correctly 🙂

The situation where I might want tests for widgets getting updated is when we have a stateful widget with some nontrivial logic of its own for arranging for updates to happen, in which case that logic needs tests. But here it's just PerAccountStoreWidget.of that causes rebuilds.

(We also rely on our handleEvent implementation calling notifyListeners in all the right situations. That isn't currently as thoroughly tested as it ideally would be — instead, for these two event types in particular and some others, we rely on the fact that the implementation straightforwardly says notifyListeners(); at the end. But if or when at some point we do test that point, the tests in test/model/ that exercise handling events are the right layer for that.)

[gnprice]

Yeah, since we're doing the computations all in terms of UTC (which is good news), that's the right way for the tests to work too.

I couldn't find a way to force the test environment to have a different time zone other than (my) local or UTC.

Unfortunately there doesn't seem to be such a way. We have a comment on one test where that came up, in test/widgets/message_list_test.dart (testing the dates we show in recipient headers):

      // We show the dates in the user's timezone.  Dart's standard library
      // doesn't give us a way to control which timezone is used — only to
      // choose between UTC and the user's timezone, whatever that may be.
      // So we do the latter, and that means these dates come out differently
      // depending on the timezone in the environment running the tests.
      // Related upstream issues:
      //   https://github.com/dart-lang/sdk/issues/28985 (about DateTime.now, not timezone)
      //   https://github.com/dart-lang/sdk/issues/44928 (about the Dart implementation's own internal tests)
      // For this test, just accept outputs corresponding to any possible timezone.
      tester.widget(find.textContaining(RegExp("Dec 1[89], 2022")));
      tester.widget(find.textContaining(RegExp("Aug 2[23], 2022")));

[gnprice]

Suggested change

	// incorrect results.
	// incorrect results; but that's acceptable for now because the app
	// doesn't support viewing as a spectator.

That way the reader isn't left hanging wondering what we do about those incorrect results.

[sm-sayedi]

Thanks @gnprice for the review. New changes pushed. Please have a look.

[gnprice]

Thanks! Looks good — merging.

One commit-message tweak: I noticed the commits had two variants of what the prefix should be for compose-box changes:
388b83b compose test [nfc]: Add a parent test group
f14e32d compose_box: Replace compose box with a banner when cannot post in a channel

namely "compose" vs "compose_box". So I looked at past examples; the shorter "compose" form is what we used originally, and have still used many more times than "compose_box". So edited to "compose":
157418c compose: Replace compose box with a banner when cannot post in a channel

---

FTR here's the command I ended up using to see past usage:
git log --format='%>(20)%an %cs %h %<(26,trunc)%s' --grep compose origin | grep -P '\bcompose(.box)?|'

It's a bit more sophisticated than the quick git log --oneline --grep foo commands I've used many times.