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

realm: Add option to list realms publicly. #16073

Closed
wants to merge 1 commit into from
Closed

realm: Add option to list realms publicly. #16073

wants to merge 1 commit into from

Conversation

majordwarf
Copy link
Collaborator

@majordwarf majordwarf commented Aug 8, 2020

The following commit adds a feature to allow sponsored realms to be
listed publicly. This is done by adding a field in Realm model and
toggle it via front-end. The feature will disabled by default for the
production, it can be enabled by editing value of OSS_SHOWCASE in
defualt_settings.py.

Fixes: #15844.

if realm.oss_showcase == 1:
page_params["oss_showcase"] = True
else:
page_params["oss_showcase"] = False
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think this could go in zerver/lib/events.py instead.

#
# To avoid spam currently only sponsored realms can list their
# realms publicly.
OSS_SHOWCASE = False
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We don't want to document this setting here; instead document it in zproject/default_settings.py

Since we don't expect it to be useful to self-hosted systems.

label=admin_settings_label.realm_oss_showcase}}
</div>
</div>
{{/if}}
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Let's make this a dropdown to match the data model. There's a chance we'll add additional values in the future that controls details like "How does the organization get listed?".

@timabbott
Copy link
Sponsor Member

@pragatiagrawal31 @shubhamdhama @ryanreh99 do any of you have time to help debug the frontend for this? @majordwarf was asking for help with it.

@pragatiagrawal31
Copy link
Member

I can, but I don't get where exactly does he need help? @majordwarf can you explain? DM the chat link or let me know here.

@majordwarf
Copy link
Collaborator Author

@pragatiagrawal31 I'll ping you on CZO.

Comment on lines 219 to 225
if settings.OSS_SHOWCASE:
realm = user_profile.realm
page_params["plan_type"] = realm.get_plan_type
if realm.oss_showcase == 1:
page_params["current_oss_showcase_value"] = "hidden"
else:
page_params["current_oss_showcase_value"] = "visible"
Copy link
Member

Choose a reason for hiding this comment

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

The issue is with this block of code.
I think at most places we don't this type of conversion, although recently there are some settings that use this "string" value approach for APIs. But I don't think that fits here. You can remove this complete block and directly use page_params.realm_oss_showcase_policy on the frontend side. And follow as we do for other similar settings like email_address_visibility_values.

text: i18n.t("Yes"),
},
],
]);
Copy link
Member

Choose a reason for hiding this comment

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

If you follow my previous comment then you have to change this value map too, make it something like email_address_visibility_values

Copy link
Member

@pragatiagrawal31 pragatiagrawal31 left a comment

Choose a reason for hiding this comment

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

So what's happening right now, if you update any setting, it doesn't update the corresponding page_params.current_oss_showcase_value for which you have to make changes in events.py. But why do this, if we have an automatic system, where page_params.realm_oss_showcase_policy is already being sent initially and is cared properly later on when there is any update.

@akashaviator
Copy link
Collaborator

@majordwarf As this PR was not linked with #15844, I started working on this as well and opened #16193. The UI and the backend are working fine. You can cherry-pick my commits and proceed ahead on creating '/public-communities' page.

@majordwarf majordwarf changed the title WIP: Public Listing realm: Add option to list realms publicly. Aug 29, 2020
@majordwarf
Copy link
Collaborator Author

@timabbott The PR is done currently working on fronted.
@pragatiagrawal31 Thanks for the helping out with that detailed explanation 👍 Can you review the PR later?

The following commit adds a feature to allow sponsored realms to be
listed publicly. This is done by adding a field in Realm model and
toggle it via front-end. The feature will disabled by default for the
production, it can be enabled by editing value of `OSS_SHOWCASE` in
`defualt_settings.py`.

Fixes: #15844.
@@ -106,6 +109,13 @@ def update_realm(
video_chat_provider not in {p['id'] for p in Realm.VIDEO_CHAT_PROVIDERS.values()}):
return json_error(_("Invalid video_chat_provider {}").format(video_chat_provider))

realm_plan_type = realm.plan_type
Copy link
Member

Choose a reason for hiding this comment

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

This variable is unnecessary, we can go for realm.plan_type

@@ -106,6 +109,13 @@ def update_realm(
video_chat_provider not in {p['id'] for p in Realm.VIDEO_CHAT_PROVIDERS.values()}):
return json_error(_("Invalid video_chat_provider {}").format(video_chat_provider))

realm_plan_type = realm.plan_type
if oss_showcase_policy is not None and realm_plan_type != 4:
Copy link
Member

Choose a reason for hiding this comment

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

Instead of hardcoding 4 we can use the variable Realm.STANDARD_FREE

@@ -15,6 +15,10 @@ exports.reset = function () {
};

exports.maybe_disable_widgets = function () {
if (page_params.realm_plan_type !== 4) {
Copy link
Member

Choose a reason for hiding this comment

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

Here too, we can avoid hardcoding values, and put a constant in settings_config if there isn't one.

@pragatiagrawal31
Copy link
Member

And since we have two PRs for the same issue, and have similar progress, @akashaviator would you be willing to give this PR a detailed review?

@zulipbot
Copy link
Member

zulipbot commented Oct 9, 2020

Heads up @majordwarf, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@timabbott
Copy link
Sponsor Member

Closing in favor of the rebased version #17021. @majordwarf if you want to get involved in helping with review there, that'd be very welcome!

@timabbott timabbott closed this Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a directory of public communities on Zulip Cloud
5 participants