Skip to content

feat: Disable sign up for xandylearning subdomain- #547 - #548

Open
HarishV14 wants to merge 3 commits into
masterfrom
xandylearning
Open

feat: Disable sign up for xandylearning subdomain- #547#548
HarishV14 wants to merge 3 commits into
masterfrom
xandylearning

Conversation

@HarishV14

Copy link
Copy Markdown
Contributor
  • Added "xandylearning" to the list of disallowed subdomains for sign up in Constants
  • Hide the sign up button in LoginActivity and UsernameAuthentication if
    the current subdomain matches the disallowed list

@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown

Claude finished @HarishV14's task in 2m 17s —— View job


Code Review

Todo List

  • Gather context (read changed files, repo CLAUDE.md — none found)
  • Understand the request (code review of PR feat: Disable sign up for xandylearning subdomain- #547 #548)
  • Review Constants.java changes
  • Review AppChecker.kt changes
  • Review LoginActivity.java changes
  • Review UsernameAuthentication.kt changes
  • Review config.json changes
  • Post final review

The core change (adding xandylearning to a disallowed-signup list and checking it in LoginActivity/UsernameAuthentication via a new AppChecker.isSignupDisabledForSubdomain()) is small and logically sound. A few things worth a look:

Findings (posted as inline comments):

  1. Duplicate import in LoginActivity.java (line 55) — in.testpress.testpress.core.Constants was already imported later in the file; this PR adds it a second time. Harmless to javac but should be cleaned up.
  2. Unused import in LoginActivity.java (line 49) — java.util.Arrays is imported but never referenced anywhere in the file.
  3. config.json change looks broader than the stated goal — this file is the shared default config that master currently builds as "sandbox". This PR changes testpress_site_subdomain to "xandylearning" and bumps version_code from 9123, while package_name/app_name still say "sandbox". Please confirm this is intentional for merging into master and not a local/testing artifact — if this repo builds one app per branch from config.json, this changes what everyone building from master gets by default.

Positive note: LoginActivity.java's new signup check (instituteSettings.getAllowSignup() != null && instituteSettings.getAllowSignup()) also fixes a latent NPE risk — getAllowSignup() returns a boxed Boolean, and the old code (!instituteSettings.getAllowSignup()) would have thrown on auto-unboxing a null value. Good catch even if incidental to this PR.

Minor/non-blocking:

  • AppChecker.isSignupDisabledForSubdomain follows the existing isCatkingApp/isEduportApp pattern reasonably; using an array + .contains() is fine at this size.
  • No new tests were added for isSignupDisabledForSubdomain, but there are no existing tests for the sibling AppChecker functions either, so this is consistent with current repo conventions rather than a regression.

No security, performance, or memory-leak concerns found in the diff.


import java.io.IOException;
import java.security.PublicKey;
import java.util.Arrays;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

java.util.Arrays is imported but never used anywhere in this file (no Arrays. reference). Unused import — please remove.

Fix this →

Comment on lines 2 to +4
"version" : "1.1.7",
"version_code" : 9,
"testpress_site_subdomain" : "sandbox",
"version_code" : 123,
"testpress_site_subdomain" : "xandylearning",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This changes the shared default config.json (currently what master builds as "sandbox") to point at xandylearning and bumps version_code from 9 to 123. A couple of concerns:

  1. testpress_site_subdomain is now "xandylearning" but package_name (in.testpress.sandbox) and app_name ("Testpress Sandbox") are unchanged — is that intentional, or should this branch's config only be used for a client-specific build rather than merged into master's default config?
  2. The version_code jump from 9 to 123 looks unrelated to the "disable signup" feature — please confirm this is intentional and not a leftover from local testing.

If this repo builds a single app per branch/tag from config.json, merging this to master would change what everyone building from master gets by default. Worth double-checking before merge.

import java.util.concurrent.Executors;

import javax.inject.Inject;
import in.testpress.testpress.core.Constants;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

in.testpress.testpress.core.Constants is already imported further down in this file (pre-existing import, now at line 67 after this diff). This adds a duplicate import. Javac tolerates duplicate imports so it won't break the build, but it should be removed since it's redundant.

Fix this →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant