openwisp / openwisp/openwisp-radius

[bug] Monitoring stats: count only verified and active users

Open
#663 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Python
Stars
445
Forks
234
Avg merge
20h 21m
Merged PRs (30d)
19

Description

⚠️ Warning: not suited for beginner contributors. PRs from beginner contributors will be closed without discussion.

The current monitoring stats which log new signups and total amount of users are counting also unverified users and inactive users.

I spent some time to change the queries and tests to fix it as it seemed an easy issue to solve, but I later realized that the solution would still be imperfect, due to the fact that the celery tasks run periodically and at the moment of running, users may be completing the verification process and may not be counted ever in the sign up stats.

I think we should change the implementation as follows.

Total user stats
  • Count only active users (filter by is_active=True, I was trying to tag along with the exclude logic in the current code but didn't seem to work well).
  • For users having a RegisteredUser relation, ensure they have is_verified=True, if identity verification is mandatory for that org
New registrations

This is a tricky one. I think we can avoid worrying about is_active=True because it's highly unlikely that a user signs up, completes the verification process and gets banned right away.

However, we shouldn't count unverified users here, but doing so in the periodic task could mean we would lose some users.
I propose that for users which have a RegisteredUser relation we stop using the periodic celery task and perform the timeseries DB write once Is_verified is set to True, which will require the addition of a new signal for this module (the docs only mentions the radius accounting signal).

We also need to take into account the case in which identity verification is not mandatory, which makes this even trickier, because it means we should count the user regardless of the is_verified flag: the old code could work, but keeping both implementations may make maintenance harder, a possible alternative solution would be to bind both signals: post_save and verification_success, using one or the other depending on whether identity verification is mandatory.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by locating the monitoring statistics queries, their tests, and the periodic Celery task described in the issue. Trace how RegisteredUser.is_verified, organization verification requirements, and user activation are handled, then review the existing radius accounting signal. Done means total and new-registration statistics consistently count the intended users without losing users during verification.

Written by the indexing model from the issue text.

Assessment

Tech stack
django, python
Domain
analytics, authentication, backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.