realm_plan_type: Restrict settings by plan type #12554

Hypro999 · 2019-06-11T16:29:57Z

A WIP PR for issue #12541. A pre-requisite step for this feature would be to do a send_event whenever a realm's plan is changed via. do_change_plan_type. As of right now, that is all that this PR contains. I'm currently stuck on a bug for making this small change.

zerver/tests/test_events.py

timabbott · 2019-06-11T17:41:27Z

So the bug here is relates to the do_test code having a performance optimization to use self.user_profile and self.user_profile.realm as the object to modify to avoid needing to do potentially unnecessary re-fetches of objects from the database.

For avoiding that sort of issue in the future, one possible solution is to lint-ban get_realm from that test file, to avoid folks re-fetching a separate realm object from the database. But before we decide on a way to improve that test system, @pragatiagrawal31 ran into a similar issue with a test_events test a few months ago; can you remind me of the PR involved?

Hypro999 · 2019-06-12T06:10:18Z

So the bug here is relates to the do_test code having a performance optimization to use self.user_profile and self.user_profile.realm as the object to modify to avoid needing to do potentially unnecessary re-fetches of objects from the database.

Ah, I see. Thanks for letting me know!

When a realm's plan type is updated using "do_change_plan_type" we notify active users of the realm. This way certain plan features could be enabled instantaneously for active users.

Hypro999 · 2019-06-12T14:05:04Z

Ok, this PR is now ready for a serious round of review. @rishig and @timabbott could you please review this?

pragatiagrawal31 · 2019-06-12T15:16:47Z

@timabbott the PR where this was fixed is #11861 .

rishig · 2019-06-12T18:18:36Z

zerver/lib/events.py

@@ -216,6 +216,10 @@ def fetch_initial_state_data(user_profile: UserProfile,
        state['realm_push_notifications_enabled'] = push_notifications_enabled()
        state['realm_upload_quota'] = realm.upload_quota_bytes()
        state['realm_plan_type'] = realm.plan_type
+        state['plan_includes_wide_organization_logo'] = realm.plan_type != Realm.LIMITED
+        state['upgrade_text_for_wide_organization_logo'] = "Available on all paid plans. Upgrade "


let's make this string a constant UPGRADE_TEXT_STANDARD, and also mark the whole string for translation.

Not sure the best way to make Upgrade a link to /plans. Doing it in the template as it is currently isn't ideal; maybe Tim will have ideas. (Also, I see in the html you've linked to /upgrade. I should have specified that the link should be to /plans).

let's make this string a constant UPGRADE_TEXT_STANDARD, and also mark the whole string for translation.

Right, I've made this variable as a member of the Realm class.

Not sure the best way to make Upgrade a link to /plans. Doing it in the template as it is currently isn't ideal; maybe Tim will have ideas

Agreed, I wasn't completely sure of what we wanted this string to look like with the hyperlink (because we're marking the string for translation, I don't think including HTML in it would be pretty, so I didn't implement it the way you mentioned it in the issue).

Also, I see in the html you've linked to /upgrade. I should have specified that the link should be to /plans

Change made.

zerver/lib/events.py

rishig · 2019-06-12T18:20:40Z

zerver/lib/events.py

@@ -575,6 +579,7 @@ def apply_event(state: Dict[str, Any],

            if event['property'] == 'plan_type':
                # Then there are some extra fields that also need to be set.
+                state['plan_includes_wide_organization_logo'] = event['value'] != Realm.LIMITED


hm, ideally this check would not be replicated. Not familiar with the code enough to know the right way to handle it though.

ideally this check would not be replicated.

I didn't get what you're saying, could you please elaborate a bit about what check is being replicated here?

oh, it just seems like there are two places in this file where we do the logic of
plan_includes_wide_organization_logo = plan_type != Realm.LIMITED
and I would have expected that logic to only appear once.

I'm not familiar with this code though, so maybe Tim will have a different opinion.

Oh, that's how it's supposed to be. I mean we could make a function and try extracting the logic, but IMHO, that's just really going out of our way in this case.

rishig · 2019-06-12T18:25:48Z

static/templates/settings/organization-profile-admin.handlebars

+            <span>
+                <h5>
+                    {{#unless plan_includes_wide_organization_logo}}
+                    {{t upgrade_text_for_wide_organization_logo}} <a href="https://zulipchat.com/upgrade/"> <i class="fa fa-rocket" aria-hidden="true"></i> </a>


Instead of using <h5> and adding the icon in the html, let's style the whole thing using a CSS class. I think we can potentially even add the rocket with a :before type thing?

I did this, and for the time being (until Tim Abbott's review), I've changed the hyperlink to cover the entire string (now excluding the rocket because of a ::after clause). This is just a placeholder until we decide where we want the hyperlink to go and how.

@timabbott, are we sure that the way we hyperlinked the whole string is a good idea (speaking from the perspective of aesthetics)?

rishig

Thanks @Hypro999! I've left a few comments, though Tim will have to review the events part.

Namely, here we add the "plan_includes_wide_organization_logo" and "upgrade_text_for_wide_organization_logo" to the page_params (which is set in zerver/lib/events.py). "plan_includes_wide_organization_logo" is True if the plan is not of the Realm.LIMITED type. We need to add this extra boolean parameter instead of just using "realm_plan_type" to make things a lot easier to work with on the frontend side, especially considering that handlebars won't allow checking for equality in its {{#if}} blocks.

Using the page_param variable "plan_includes_wide_organization_logo" disallow users in a realm with a "LIMITED" plan type from uploading their own wide organization logos and instead suggest that they upgrade their plan using the page_param variable "upgrade_text_for_wide_organization_logo" for the suggestion message. Backend validation for this feature already exists.

Hypro999 · 2019-06-13T06:16:50Z

Thanks for the review @rishig! I've made most of the requested changes and extended the conversation where I couldn't make a satisfactory change.

timabbott · 2019-06-14T23:27:03Z

Merged, after adding target="_blank" to the link so it opens in a new window and doing some related line-wrapping.

@rishig one question for a quick follow-up: Do we want the string to say "... all paid or sponsored plans", to make clear it's available to open source orgs?

rishig · 2019-06-16T16:20:17Z

Our current offering is that sponsored plans are identical to Zulip Standard, so I think we don't want to create a distinction between paid and sponsored.

How about "Available on Zulip Standard and Plus. Upgrade"?

zulipbot added the size: M label Jun 11, 2019

timabbott reviewed Jun 11, 2019

View reviewed changes

zerver/tests/test_events.py Outdated Show resolved Hide resolved

realm_plan_type: Send an event when the realm's plan type is updated.

65491fb

When a realm's plan type is updated using "do_change_plan_type" we notify active users of the realm. This way certain plan features could be enabled instantaneously for active users.

zulipbot added size: L and removed size: M labels Jun 12, 2019

rishig reviewed Jun 12, 2019

View reviewed changes

zerver/lib/events.py Outdated Show resolved Hide resolved

rishig reviewed Jun 12, 2019

View reviewed changes

Hypro999 added 2 commits June 13, 2019 11:36

timabbott closed this Jun 14, 2019

timabbott mentioned this pull request Jun 14, 2019

settings: Add backend and UI for restricting setings by plan type. #12541

Closed

2 tasks

Hypro999 changed the title ~~[WIP] Restrict settings by plan type~~ realm_plan_type: Restrict settings by plan type Jun 15, 2019

Hypro999 deleted the restrict-settings-by-plan-type branch June 15, 2019 03:37

Hypro999 mentioned this pull request Jun 14, 2020

Avoid sending avatar URLs of long term idle users to clients #15359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

realm_plan_type: Restrict settings by plan type #12554

realm_plan_type: Restrict settings by plan type #12554

Hypro999 commented Jun 11, 2019

timabbott commented Jun 11, 2019

Hypro999 commented Jun 12, 2019

Hypro999 commented Jun 12, 2019

pragatiagrawal31 commented Jun 12, 2019

rishig Jun 12, 2019 •

edited

Hypro999 Jun 13, 2019

rishig Jun 12, 2019 •

edited

Hypro999 Jun 13, 2019

rishig Jun 13, 2019

Hypro999 Jun 13, 2019

rishig Jun 12, 2019

Hypro999 Jun 13, 2019

Hypro999 Jun 15, 2019

rishig left a comment

Hypro999 commented Jun 13, 2019

timabbott commented Jun 14, 2019

rishig commented Jun 16, 2019 •

edited

realm_plan_type: Restrict settings by plan type #12554

realm_plan_type: Restrict settings by plan type #12554

Conversation

Hypro999 commented Jun 11, 2019

timabbott commented Jun 11, 2019

Hypro999 commented Jun 12, 2019

Hypro999 commented Jun 12, 2019

pragatiagrawal31 commented Jun 12, 2019

rishig Jun 12, 2019 • edited

Choose a reason for hiding this comment

Hypro999 Jun 13, 2019

Choose a reason for hiding this comment

rishig Jun 12, 2019 • edited

Choose a reason for hiding this comment

Hypro999 Jun 13, 2019

Choose a reason for hiding this comment

rishig Jun 13, 2019

Choose a reason for hiding this comment

Hypro999 Jun 13, 2019

Choose a reason for hiding this comment

rishig Jun 12, 2019

Choose a reason for hiding this comment

Hypro999 Jun 13, 2019

Choose a reason for hiding this comment

Hypro999 Jun 15, 2019

Choose a reason for hiding this comment

rishig left a comment

Choose a reason for hiding this comment

Hypro999 commented Jun 13, 2019

timabbott commented Jun 14, 2019

rishig commented Jun 16, 2019 • edited

rishig Jun 12, 2019 •

edited

rishig Jun 12, 2019 •

edited

rishig commented Jun 16, 2019 •

edited