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

DEV: Ability to collect stats without exposing them via API #23933

Merged

Conversation

AndrewPrigorshnev
Copy link
Contributor

@AndrewPrigorshnev AndrewPrigorshnev commented Oct 13, 2023

This adds the ability to collect stats without exposing them among other stats via API.

The most important thing I wanted to achieve is to provide an API where stats are not exposed by default, and a developer has to explicitly specify that they should be exposed (expose_via_api: true). Implementing an opposite solution would be simpler, but that's less safe in terms of potential security issues.

When working on this, I had to refactor the current solution. I would go even further with the refactoring, but the next steps seem to be going too far in changing the solution we have, and that would also take more time. Two things that can be improved in the future:

  1. Data structures for holding stats can be further improved
  2. Core stats are hard-coded in the About template (it's hard to fix it without correcting data structures first, see point 1):
    <tr>
    <th>&nbsp;</th>
    <th><%=t 'js.about.stat.all_time' %></th>
    <th><%=t 'js.about.stat.last_day' %></th>
    <th><%=t 'js.about.stat.last_7_days' %></th>
    <th><%=t 'js.about.stat.last_30_days' %></th>
    </tr>
    <tr>
    <td class='title'><%=t 'js.about.topic_count' %></td>
    <td><%= stats["topic_count"] %></td>
    <td><%= stats["topics_last_day"] %></td>
    <td><%= stats["topics_7_days"] %></td>
    <td><%= stats["topics_30_days"] %></td>
    </tr>
    <tr>
    <td><%=t 'js.about.post_count' %></td>
    <td><%= stats["post_count"] %></td>
    <td><%= stats["posts_last_day"] %></td>
    <td><%= stats["posts_7_days"] %></td>
    <td><%= stats["posts_30_days"] %></td>
    </tr>
    <tr>
    <td><%=t 'js.about.user_count' %></td>
    <td><%= stats["user_count"] %></td>
    <td><%= stats["users_last_day"] %></td>
    <td><%= stats["users_7_days"] %></td>
    <td><%= stats["users_30_days"] %></td>
    </tr>
    <tr>
    <td><%=t 'js.about.active_user_count' %></td>
    <td>&mdash;</td>
    <td><%= stats["active_users_last_day"] %></td>
    <td><%= stats["active_users_7_days"] %></td>
    <td><%= stats["active_users_30_days"] %></td>
    </tr>
    <tr>
    <td><%=t 'js.about.like_count' %></td>
    <td><%= stats["like_count"] %></td>
    <td><%= stats["likes_last_day"] %></td>
    <td><%= stats["likes_7_days"] %></td>
    <td><%= stats["likes_30_days"] %></td>

The most significant refactorings are:

  1. Introducing the Stat model
  2. Aligning the way the core and the plugin stats' are registered

@github-actions github-actions bot added the chat PRs which include a change to Chat plugin label Oct 13, 2023
@AndrewPrigorshnev AndrewPrigorshnev force-pushed the dev/ability-to-collect-stats-without-public-exposing branch 7 times, most recently from 95d6f80 to 813039c Compare October 19, 2023 15:02
@AndrewPrigorshnev AndrewPrigorshnev force-pushed the dev/ability-to-collect-stats-without-public-exposing branch 3 times, most recently from 9dbac3b to bf3f119 Compare October 24, 2023 21:45
@AndrewPrigorshnev AndrewPrigorshnev changed the title DEV: Ability to collect stats without exposing them DEV: Ability to collect stats without exposing them via API Oct 24, 2023
@AndrewPrigorshnev AndrewPrigorshnev force-pushed the dev/ability-to-collect-stats-without-public-exposing branch from 7f0edf2 to 44d2ddf Compare October 24, 2023 22:22
@AndrewPrigorshnev AndrewPrigorshnev marked this pull request as ready for review October 24, 2023 23:22
@AndrewPrigorshnev AndrewPrigorshnev force-pushed the dev/ability-to-collect-stats-without-public-exposing branch 2 times, most recently from d2ee31f to 721b38c Compare October 26, 2023 13:51
app/models/stat.rb Outdated Show resolved Hide resolved
app/models/stat.rb Outdated Show resolved Hide resolved
app/models/stat.rb Outdated Show resolved Hide resolved
lib/statistics.rb Outdated Show resolved Hide resolved
@AndrewPrigorshnev AndrewPrigorshnev force-pushed the dev/ability-to-collect-stats-without-public-exposing branch 2 times, most recently from 3b58457 to 5757d51 Compare October 31, 2023 23:09
@AndrewPrigorshnev AndrewPrigorshnev force-pushed the dev/ability-to-collect-stats-without-public-exposing branch 2 times, most recently from c389b33 to 501753e Compare November 1, 2023 18:34
@AndrewPrigorshnev AndrewPrigorshnev force-pushed the dev/ability-to-collect-stats-without-public-exposing branch from 501753e to 864c3c2 Compare November 7, 2023 17:48
@AndrewPrigorshnev AndrewPrigorshnev force-pushed the dev/ability-to-collect-stats-without-public-exposing branch from 864c3c2 to 5fe393d Compare November 9, 2023 17:05
@AndrewPrigorshnev AndrewPrigorshnev merged commit d91456f into main Nov 9, 2023
15 checks passed
@AndrewPrigorshnev AndrewPrigorshnev deleted the dev/ability-to-collect-stats-without-public-exposing branch November 9, 2023 20:44
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/shields-io-unable-to-retrieve-discourse-statistics-api/128574/12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat PRs which include a change to Chat plugin
4 participants