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

Moved puzzle answer from config to database #271

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aletya
Copy link
Contributor

@aletya aletya commented Nov 3, 2024

Resolves #229

Copy link
Member

@Timothy-Gonzalez Timothy-Gonzalez left a comment

Choose a reason for hiding this comment

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

Looks good, glad we're moving answers to the database. A few things:

  • Should remove the config variables so the env variables aren't rquired anymore from both src/common/config.ts and .test.env
  • We should have tests for this service - it's not a huge service so this shouldn't be an issue, just want to make sure we are sure this won't break runes & riddles
  • Finally, we should have a Staff only endpoint to update these answers without having to connect to the db, but that can come later

src/services/puzzle/puzzle-router.ts Outdated Show resolved Hide resolved
Copy link
Member

@Timothy-Gonzalez Timothy-Gonzalez left a comment

Choose a reason for hiding this comment

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

Good overall, just need to clean up the tests a bit. A few overall things:

  • Don't hardcode strings, define them once and if they're already defined use it from before
  • Use beforeEach if you're going to do something for each test
  • Don't compare errors by message - compare by error (NotFound instead of Couldn't find user!. That way we can change messages without breaking tests. error will always be consistent.
  • Look at the coverage to see what is missed. According to coverage: 103-104,116-117,121-122,145-146 are all missed lines. 103-104 and 145-146 don't need to be tested, but 116-117 and 121-122 (puzzle not yet created and if puzzle is already completed) should be tested

This is very nit picky, yes, but I want the foundation of these tests to be good so that someone who extends them extends good code. Once the main tests are done, adding a few tests becomes much easier.

src/services/puzzle/puzzle-router.test.ts Outdated Show resolved Hide resolved
src/services/puzzle/puzzle-router.test.ts Outdated Show resolved Hide resolved
src/services/puzzle/puzzle-router.test.ts Outdated Show resolved Hide resolved
src/services/puzzle/puzzle-router.test.ts Show resolved Hide resolved
src/services/puzzle/puzzle-router.test.ts Outdated Show resolved Hide resolved
src/services/puzzle/puzzle-router.test.ts Outdated Show resolved Hide resolved
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.

Puzzle answers should be stored in database
2 participants