Skip to content

refactor: env-var overrides, error handling, and cleanup for testability#130

Closed
amitb0ra wants to merge 1 commit intoRocketChat:masterfrom
amitb0ra:refactor/server-testability
Closed

refactor: env-var overrides, error handling, and cleanup for testability#130
amitb0ra wants to merge 1 commit intoRocketChat:masterfrom
amitb0ra:refactor/server-testability

Conversation

@amitb0ra
Copy link
Copy Markdown
Contributor

Summary

Extracts server-side bug fixes, cleanup, and env-var overrides into a standalone PR

Changes

app.js

  • Env-var path overridesLEADERBOARD_CONFIG_PATH, LEADERBOARD_DATA_PATH, LEADERBOARD_LOG_PATH, LEADERBOARD_ADMINDATA_PATH, LEADERBOARD_CONFIG_BACKUP_PATH, LEADERBOARD_PORT
  • LEADERBOARD_SKIP_REFRESH=1 — skip spawning refresh.js child process (useful for testing and dry-run setups)
  • Pass res to Util.post() calls — enables proper error responses on malformed JSON (see Util.js below)
  • Fix hardcoded ./admindata.json in login handler → use the admindataPath variable (was already defined but not used in one spot)
  • Move findContributor from inline function to Util.findContributor (deduplication)
  • module.exports = { server } — export the HTTP server for programmatic use

Util.js

API.js

  • Remove process.exit() on bad GitHub credentials — log the error and return instead (prevents the entire server from crashing on a single bad token check)
  • Support LEADERBOARD_CONFIG_PATH env var for config resolution

Cleanup

  • Remove stale .dockerignore (Docker support was removed in a previous PR)
  • Remove dead tests/access.sh and tests/api-call-tests.sh (brute-force stress scripts hitting a defunct external URL)

Merge order

  1. This PR first (source changes)
  2. PR test: add e2e test suite #127 second (test suite, depends on this)

- app.js: env-var path overrides (LEADERBOARD_CONFIG_PATH, LEADERBOARD_DATA_PATH,
  LEADERBOARD_LOG_PATH, LEADERBOARD_ADMINDATA_PATH, LEADERBOARD_CONFIG_BACKUP_PATH,
  LEADERBOARD_PORT), LEADERBOARD_SKIP_REFRESH to disable child process, pass res
  to Util.post calls, fix hardcoded ./admindata.json in login handler, export server
- Util.js: post(req, res, callback) — respond 400 on malformed JSON instead of
  silently hanging, move findContributor from app.js
- API.js: remove process.exit() on bad credentials — log error and return instead,
  support LEADERBOARD_CONFIG_PATH env var
- Remove dead .dockerignore (Docker support removed previously)
- Remove stale tests/access.sh and tests/api-call-tests.sh (brute-force scripts
  hitting dead external URLs)
Copy link
Copy Markdown
Member

@Sing-Li Sing-Li left a comment

Choose a reason for hiding this comment

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

It is a pity to add ENV var JUST TO SUPPORT TESTING.

Please look into the npm .dotenv library and make this PR a support for .env files during deployment. (in addition to just support testing)

I am totally against combining code generator suggested hallucinations into one PR. For example, the fix in the callback() JSON parse is simply nonsense.

Please remove all other fixes or mods that has nothing to do with supporting dotenv, then test to make sure .env files are actually supported. Thanks.

@Sing-Li
Copy link
Copy Markdown
Member

Sing-Li commented Mar 26, 2026

closing this since you didn't reuse it.

@Sing-Li Sing-Li closed this Mar 26, 2026
@amitb0ra amitb0ra deleted the refactor/server-testability branch April 1, 2026 17:56
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.

2 participants