analytics: Make stats of all realms accessible to server admins. #9099

shubhamdhama · 2018-04-15T17:55:05Z

Currently, I've made things working.
@timabbott @rishig FYI.
~~Tests aren't updated/added yet.~~ Added
Sorry for the delay actually in this comment activity: First steps to make the data clearer #7568 (comment) I've misunderstood that we want the data of /stat to be served in the tables of /activity page, ~~indeed I'm not sure whether we want to do this also or not?~~
Currently we can access chart stats of any realm at URL analytics/chart_data/realm/(?P<realm_str>[\S]+). Definitely, we need a page to direct to these links or we can have these links at /activity page(I think this clears my above confusion) Added link in /activity table

shubhamdhama · 2018-04-15T17:56:39Z

analytics/views.py

+                               realm=realm.string_id,
+                               is_staff=request.user.is_staff))
+
+@require_server_admin_access


Any better name suggestion for this decorator :)

see above :)

rishig · 2018-04-16T04:29:54Z

cool! I would put the links on /activity next to the email icon. Maybe a chart icon?

shubhamdhama · 2018-04-16T10:41:29Z

Sounds good, added the link:

timabbott · 2018-04-16T17:13:04Z

zerver/decorator.py

+        if not user_profile.is_staff:
+            raise JsonableError(_("Must be an server administrator"))
+        return view_func(request, user_profile, *args, **kwargs)
+    return _wrapped_view_func  # type: ignore # https://github.com/python/mypy/issues/1927


This should come with some tests in test_decorator.py.

Also, I'd call it require_server_admin_api -- since the point is it's for API/JSON type views.

timabbott · 2018-04-16T17:15:24Z

analytics/views.py

+                  'analytics/stats.html',
+                  context=dict(given_realm_name=realm.name,
+                               realm=realm.string_id,
+                               is_staff=request.user.is_staff))


I think you want to deduplication this render block in a function. E.g. return render_stats(request, realm).

timabbott · 2018-04-16T17:17:01Z

analytics/views.py

+                             realm_str: str, **kwargs: Any) -> HttpResponse:
+    realm = get_realm(realm_str)
+    if realm is None:
+        raise JsonableError(_(("Realm %s does not exist") % (realm_str)))


This is the wrong order-of-operations of i8n and string interpolation and doesn't work.

Also, we generally don't use realm in user-facing strings. I'd probably make this error just be "Invalid organization" -- no need to include the organization ID in the error (it'll be right there in the URL).

Ohh yeah, I missed this, it should be _("Realm %s does not exist") % (realm_str), BTW removing realm_str.

timabbott · 2018-04-16T17:18:09Z

static/js/stats/stats.js

 function get_chart_data(data, callback) {
+    var url;
+    if (page_params.is_staff === "True") {


This is kinda hacky; ideally, we'd do the same JSON encoding thing that we do in home() to end up with a proper JS "true".

timabbott · 2018-04-16T17:18:45Z

templates/analytics/stats.html

@@ -14,7 +24,7 @@
    <div class="page-content">
        <div id="id_stats_errors" class="alert alert-error"></div>
        <div class="center-charts">
-            <h1 class="analytics-page-header">{% trans %}Zulip analytics for {{ realm_name }}{% endtrans %}</h1>
+            <h1 class="analytics-page-header">{% trans %}Zulip analytics for {{ given_realm_name }}{% endtrans %}</h1>


I think a better name would be target_realm_name, perhaps?

timabbott · 2018-04-16T17:19:08Z

I posted a batch of comments. Thanks for doing this @shubhamdhama, I'm excited about this :)

rishig · 2018-04-16T18:12:46Z

small comment about the UI: we don't need another column for this. We can just have an empty column label, and then put both the mail icon and the pie chart icon together in each box.

shubhamdhama · 2018-04-17T12:45:52Z

@rishig @timabbott Updated the PR.

rishig · 2018-04-17T15:09:23Z

Tim: do we care for this page to be translated at all?

timabbott · 2018-04-18T01:17:35Z

No, we don't want translations on this page; it'd waste translator time more than anything else.

timabbott · 2018-04-18T01:18:00Z

zerver/decorator.py

+        if not user_profile.is_staff:
+            raise JsonableError(_("Must be an server administrator"))
+        return view_func(request, user_profile, *args, **kwargs)
+    return _wrapped_view_func  # type: ignore # https://github.com/python/mypy/issues/1927


Can you add a test in the commit that adds this?

rishig · 2018-04-18T01:32:06Z

ah, I was confused by something in the diff. Agreed about not translating.

This is just variabnt of `require_server_admin` for JSON/api views.

In this commit: Two new URLs are added, to make all realms accessible for server admins. One is for the stats page itself and another for getting chart data i.e. chart data API requests. For the above two new URLs corresponding two view functions are added.

shubhamdhama · 2018-04-18T16:17:27Z

analytics/tests/test_views.py

+
+        result = self.client_get('/json/analytics/chart_data/realm/zulip/',
+                                 {'chart_name': 'number_of_humans'})
+        self.assert_json_error(result, "Must be an server administrator", 400)


@timabbott This already tests the new decorator function( just like require_realm_admin, which is also tested by similar requests in other tests).
Actually I give this a shot, and it is really a pain to add a test for this, the problem here is that if we want to test the decorator (in its own commit) we have to mock zulip_login_required decorator itself and from my search mocking a decorator is a complex task which become more worse here as we are further testing another decorator (because decorators act at the load time rather than run time which makes it difficult). Another alternative here is that to mock whole request object, but this approach doesn't look right to me because of redefining many functions and other object members.
If we still want to add test, can you please help me here?

OK, I guess the current tests are fine.

timabbott · 2018-04-18T18:13:00Z

I fixed a couple things:

The target_realm_name variable fix was in the wrong commit
There was a typo in the last commit's spelling of "activity".

Merged, thanks @shubhamdhama!

I think one follow-up that we should do after this is take advantage of it to improve the analytics development process. @rishig may have some thoughts, but ideally, we'd have a default user have is_staff enabled.
(1) populate_analytics_db we should probably run directly from the main populate_db project, and maybe hide the shylock user.
(2) We should make update the docs on analytics development to recommend using this feature

(The overall goal is to make "testing analytics" not require a bunch of manual setup)

zulipbot added the size: XL label Apr 15, 2018

shubhamdhama commented Apr 15, 2018

View reviewed changes

shubhamdhama force-pushed the activtiy-get-realm-data branch from c5fba96 to cf90d96 Compare April 16, 2018 10:39

shubhamdhama force-pushed the activtiy-get-realm-data branch from cf90d96 to 26b48cc Compare April 16, 2018 12:05

shubhamdhama changed the title ~~[WIP]analytics: Make stats of all realms accessible to server admins.~~ analytics: Make stats of all realms accessible to server admins. Apr 16, 2018

timabbott reviewed Apr 16, 2018

View reviewed changes

shubhamdhama force-pushed the activtiy-get-realm-data branch from 26b48cc to b93ae3a Compare April 17, 2018 12:23

timabbott reviewed Apr 18, 2018

View reviewed changes

shubhamdhama added 3 commits April 18, 2018 19:25

decorator: Add decorator for checking whether user is server admin.

693544e

This is just variabnt of `require_server_admin` for JSON/api views.

activtiy: Add realm stats link to "realms activtiy table".

4c5963e

shubhamdhama commented Apr 18, 2018

View reviewed changes

shubhamdhama force-pushed the activtiy-get-realm-data branch from b93ae3a to 4c5963e Compare April 18, 2018 16:18

timabbott mentioned this pull request Apr 18, 2018

activity: First steps to make the data clearer #7568

Closed

timabbott closed this Apr 18, 2018

shubhamdhama mentioned this pull request Jun 5, 2018

streams: Send peer events to subscribed guest users also and followups. #9673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

analytics: Make stats of all realms accessible to server admins. #9099

analytics: Make stats of all realms accessible to server admins. #9099

shubhamdhama commented Apr 15, 2018 •

edited

Loading

shubhamdhama Apr 15, 2018

timabbott Apr 16, 2018

rishig commented Apr 16, 2018

shubhamdhama commented Apr 16, 2018

timabbott Apr 16, 2018

timabbott Apr 16, 2018

timabbott Apr 16, 2018

shubhamdhama Apr 17, 2018

timabbott Apr 16, 2018

timabbott Apr 16, 2018

timabbott commented Apr 16, 2018

rishig commented Apr 16, 2018

shubhamdhama commented Apr 17, 2018

rishig commented Apr 17, 2018

timabbott commented Apr 18, 2018

timabbott Apr 18, 2018

rishig commented Apr 18, 2018

shubhamdhama Apr 18, 2018

timabbott Apr 18, 2018

timabbott commented Apr 18, 2018

analytics: Make stats of all realms accessible to server admins. #9099

analytics: Make stats of all realms accessible to server admins. #9099

Conversation

shubhamdhama commented Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishig commented Apr 16, 2018

shubhamdhama commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Apr 16, 2018

rishig commented Apr 16, 2018

shubhamdhama commented Apr 17, 2018

rishig commented Apr 17, 2018

timabbott commented Apr 18, 2018

Choose a reason for hiding this comment

rishig commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Apr 18, 2018

shubhamdhama commented Apr 15, 2018 •

edited

Loading