permission selectors: Add, with getCanCreateWebPublicStreams

[chrisbobbe]

Still TODO: Make User.role live-update with events. I've run into a Flow snag with that; see discussion EDIT: Done, in #5386!

Two commits in the branch are cherry-picked from Greg's #5382.

[gnprice]

Thanks! This looks good; just small comments.

[gnprice]

Let's include in this jsdoc a link to somewhere in the API docs that describes these values. (I think there are several places it appears; any will do.)

Quite possibly #5382 should do that too, but it's less essential there because the type definition is right next to a place where it's used, which has such a link.

[gnprice]

Hmm, yeah, awkward.

I guess this is the same Flow issue as in 04c5a03.

Though for that other instance, Flow enums aren't a solution, because there we're using the values as the "type" field of a disjoint object union, and Flow enums don't support that (because the values all have the same type, the enum type, instead of distinct types like string literals or number literals have.)

OTOH for that other instance, switching to literals was an OK workaround, because they're meaningful strings. Here, probably better to keep the names (as this branch does) and bite the bullet of having to carefully look for use sites whenever we add a new value to one of these enums.

[gnprice]

Same issue also as in #5382 (comment) . I guess there the option I went for was:

  switch (policy) {
    // FlowIssue: sad that we end up having to write numeric literals here :-/
    //   But the most important thing to get from the type-checker here is
    //   that the ensureUnreachable works -- that ensures that when we add a
    //   new possible value, we'll add a case for it here.  Couldn't find a
    //   cleaner way to write this that still accomplished that.
    case 0: // UserTopicVisibilityPolicy.None
      return false;
    case 1: // UserTopicVisibilityPolicy.Muted
      return true;
    default:
      ensureUnreachable(policy);
      throw new Error(`invalid UserTopicVisibilityPolicy: ${policy}`);
  }

Maybe that's better. There's still a bullet to bite: you have to hope, or check, that the numbers agree with the names in the comments. But it has the advantage that it's something you only have to redo when you're already looking at this code: unless we were to change the meaning of an existing value (which is much less likely than adding a new one), any faraway changes wouldn't silently invalidate this and would instead cause a type error here if it needs an update.

[chrisbobbe]

Yeah, that makes sense—I'll do that, with your FlowIssue comment copied over, if you don't mind, and a link to this discussion.

[gnprice]

Is there a particular piece of the web code that this is modeled on, and/or is meant to have the same behavior as? It'd be helpful to point to it if so.

[chrisbobbe]

Ah, right, yeah. So I did look at user_can_create_web_public_streams in settings_data—

export function user_can_create_web_public_streams(): boolean {
    if (
        !page_params.server_web_public_streams_enabled ||
        !page_params.realm_enable_spectator_access
    ) {
        return false;
    }

    return user_has_permission(page_params.realm_create_web_public_stream_policy);
}

function user_has_permission(policy_value: number): boolean {
    /* At present, nobody and by_owners_only is not present in
     * common_policy_values, but we include a check for it here,
     * so that code using create_web_public_stream_policy_values
     * or other supersets can use this function. */
    if (policy_value === settings_config.create_web_public_stream_policy_values.nobody.code) {
        return false;
    }

    if (page_params.is_owner) {
        return true;
    }

    if (
        policy_value === settings_config.create_web_public_stream_policy_values.by_owners_only.code
    ) {
        return false;
    }

    if (page_params.is_admin) {
        return true;
    }

    if (page_params.is_spectator) {
        return false;
    }

    /* At present, by_everyone is not present in common_policy_values,
     * but we include a check for it here, so that code using
     * common_message_policy_values or other supersets can use this function. */
    if (policy_value === settings_config.common_message_policy_values.by_everyone.code) {
        return true;
    }

    if (page_params.is_guest) {
        return false;
    }

    if (policy_value === settings_config.common_policy_values.by_admins_only.code) {
        return false;
    }

    if (page_params.is_moderator) {
        return true;
    }

    if (policy_value === settings_config.common_policy_values.by_moderators_only.code) {
        return false;
    }

    if (policy_value === settings_config.common_policy_values.by_members.code) {
        return true;
    }

    const current_datetime = new Date();
    const person_date_joined = new Date(user_join_date);
    const user_join_days =
        (current_datetime.getTime() - person_date_joined.getTime()) / 1000 / 86400;
    return user_join_days >= page_params.realm_waiting_period_threshold;
}

—but I found this way (the one in the branch) to be less awkward than a direct translation of that. For a few reasons:

1. A direct translation of user_has_permission can't really work if we want Flow to help us confirm that we've handled all possible CreateWebPublicStreamPolicy values. I know there's a bug with the ensureUnreachable, discussed upthread, but a fix or workaround will still probably depend on the switch-case structure in the branch.
2. user_has_permission is shared among other user_can_ functions, and it has what would be dead code for us. For example, it checks if the the policy value is settings_config.common_message_policy_values.by_everyone.code. That's 5, and 5 is not even a value that the create-web-public-stream policy can take. (Because of the product decision that it's too risky to even let org owners permit anyone to create web-public streams.) The bit at the end, where it makes a new Date(), would also be dead code, since all of this policy's possible values are exhausted before then. For dead code, it seems pretty effortful to keep that code lying around such that it'd behave correctly if we ever actually invoke it (managing React re-rendering, etc.).

Then, I figured that clients are free to write code in a way that makes sense, as long as they respect the documented API that they're using. I believe I've done so, and I didn't find anything unexpected or warty in the API from reading this web-app code.

[chrisbobbe]

For the record, I do think we've got the same behavior as the server's user_can_create_web_public_streams (modulo some inputs aren't live-updating yet). But I'm also looking to the API documentation (what the different values mean for this policy, what the different roles are, etc.), and I'm glad to be aligning with that too.

[gnprice]

Cool, that reasoning makes sense.

I think it's still helpful to point at that web implementation just for comparison, or to say it's loosely based on / simplified from that one.

If you're thinking of the API docs as the primary thing this code is written against (which is good! better that than an implementation, as long as the docs are clear enough), then a link to the relevant bit of API docs would also be helpful.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Ah, and I hadn't actually read the test code yet. :-) Just a couple of comments on that, below.

[gnprice]

Neat!

[gnprice]

This should make sure the other conditions are in place so that it would be allowed if not for this setting.

From a goals perspective: the purpose of this test is to exercise the webPublicStreamsEnabled condition in the function. If I comment that condition right out:

-  if (!webPublicStreamsEnabled || !enableSpectatorAccess) {
-    return false;
-  }
+  // if (!webPublicStreamsEnabled || !enableSpectatorAccess) {
+  //   return false;
+  // }

then the test still passes. So the test isn't accomplishing its purpose.

Concretely, fixing this means something like make the self-user an owner and the policy ModeratorOrAbove.

(If the test did successfully fail on knocking that code out, then it'd be because of values we happen to have in the default example data, and we'd still want to explicitly set them here.)

[chrisbobbe]

Thanks for the review! Revision pushed.

[chrisbobbe]

I think it might be nice to use a completely empty initial state here, then apply changes like createWebPublicStreamPolicy in a minimally modified eg.action.register_complete. The problem is that eg.reduxStatePlus gives us boring things that we need, like putting selfUser in state.users, that we don't get from eg.action.register_complete.

What do we think about a TODO to change eg.action.register_complete's interface such that it would make a blank state look more like eg.plusReduxState? We could even construct eg.plusReduxState using this modified eg.action.register_complete.

[gnprice]

to change eg.action.register_complete's interface such that it would make a blank state look more like eg.plusReduxState? We could even construct eg.plusReduxState using this modified eg.action.register_complete.

Yeah, that sounds like a good direction.

I'm not sure if we have a use case that needs a "minimal" version of the action, which is what that's currently documented as. Would have to look at the use sites of it to see. If we do, we could potentially have both -- first the minimal, then also one that's defined by starting from that and adding things we need to add.

[gnprice]

Thanks! Looks good; merging.

[chrisbobbe]

Thanks for the review!