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

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
9 changes: 9 additions & 0 deletions lib/api/model/initial_snapshot.dart
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ class InitialSnapshot {

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

/// 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.
///
/// For how to determine if a user is a full member, see:
/// https://zulip.com/api/roles-and-permissions#determining-if-a-user-is-a-full-member
final int realmWaitingPeriodThreshold;

final Map<String, RealmDefaultExternalAccount> realmDefaultExternalAccounts;

final int maxFileUploadSizeMib;
Expand Down Expand Up @@ -115,6 +123,7 @@ class InitialSnapshot {
required this.streams,
required this.userSettings,
required this.userTopics,
required this.realmWaitingPeriodThreshold,
required this.realmDefaultExternalAccounts,
required this.maxFileUploadSizeMib,
required this.realmUsers,
Expand Down
3 changes: 3 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.

30 changes: 30 additions & 0 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,18 @@ class User {
factory User.fromJson(Map<String, dynamic> json) => _$UserFromJson(json);

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

/// 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.
bool hasPassedWaitingPeriod(DateTime byDate, int realmWaitingPeriodThreshold) {
final dateJoined = DateTime.parse(this.dateJoined);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

return byDate.difference(dateJoined).inDays >= realmWaitingPeriodThreshold;
}
}

/// As in [User.profileData].
Expand Down Expand Up @@ -303,6 +315,11 @@ enum UserRole{
final int? apiValue;

int? toJson() => apiValue;

bool isAtLeast(UserRole threshold) {
// Roles with more privilege have lower [apiValue].
return (apiValue ?? 0) <= (threshold.apiValue ?? 0);
Copy link
Member

Choose a reason for hiding this comment

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

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.

}
}

/// As in `streams` in the initial snapshot.
Expand Down Expand Up @@ -370,6 +387,19 @@ class ZulipStream {
_$ZulipStreamFromJson(json);

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

bool hasPostingPermission(User user, {required DateTime byDate, required int realmWaitingPeriodThreshold}) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's break this into multiple lines

final role = user.role;
return switch (channelPostPolicy) {
ChannelPostPolicy.any => true,
ChannelPostPolicy.fullMembers => role.isAtLeast(UserRole.member) && (role == UserRole.member
Copy link
Member

@PIG208 PIG208 Oct 10, 2024

Choose a reason for hiding this comment

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

nit: by turning this into || we can replace the ... ? ... : true clause with ... && ...

? user.hasPassedWaitingPeriod(byDate, realmWaitingPeriodThreshold)
: true),
ChannelPostPolicy.moderators => role.isAtLeast(UserRole.moderator),
ChannelPostPolicy.administrators => role.isAtLeast(UserRole.administrator),
ChannelPostPolicy.unknown => true,
};
}
}

/// The name of a property of [ZulipStream] that gets updated
Expand Down
4 changes: 4 additions & 0 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
connection: connection,
realmUrl: realmUrl,
maxFileUploadSizeMib: initialSnapshot.maxFileUploadSizeMib,
realmWaitingPeriodThreshold: initialSnapshot.realmWaitingPeriodThreshold,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this should come before maxFileUploadSizeMib.

realmDefaultExternalAccounts: initialSnapshot.realmDefaultExternalAccounts,
customProfileFields: _sortCustomProfileFields(initialSnapshot.customProfileFields),
emailAddressVisibility: initialSnapshot.emailAddressVisibility,
Expand Down Expand Up @@ -270,6 +271,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
required this.connection,
required this.realmUrl,
required this.maxFileUploadSizeMib,
required this.realmWaitingPeriodThreshold,
required this.realmDefaultExternalAccounts,
required this.customProfileFields,
required this.emailAddressVisibility,
Expand Down Expand Up @@ -324,6 +326,8 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess

String get zulipVersion => account.zulipVersion;
final int maxFileUploadSizeMib; // No event for this.
/// For docs, please see [InitialSnapshot.realmWaitingPeriodThreshold].
final int realmWaitingPeriodThreshold; // TODO(#668): update this realm setting
final Map<String, RealmDefaultExternalAccount> realmDefaultExternalAccounts;
List<CustomProfileField> customProfileFields;
/// For docs, please see [InitialSnapshot.emailAddressVisibility].
Expand Down
2 changes: 2 additions & 0 deletions test/example_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -789,6 +789,7 @@ InitialSnapshot initialSnapshot({
List<ZulipStream>? streams,
UserSettings? userSettings,
List<UserTopicItem>? userTopics,
int? realmWaitingPeriodThreshold,
Map<String, RealmDefaultExternalAccount>? realmDefaultExternalAccounts,
int? maxFileUploadSizeMib,
List<User>? realmUsers,
Expand Down Expand Up @@ -821,6 +822,7 @@ InitialSnapshot initialSnapshot({
emojiset: Emojiset.google,
),
userTopics: userTopics,
realmWaitingPeriodThreshold: realmWaitingPeriodThreshold ?? 0,
realmDefaultExternalAccounts: realmDefaultExternalAccounts ?? {},
maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25,
realmUsers: realmUsers ?? [],
Expand Down