Skip to content

Conversation

@joyguptaa
Copy link
Contributor

@joyguptaa joyguptaa commented Mar 18, 2025

Date: 18 March 2025

Developer Name: Joy Gupta


Issue Ticket Number

PRs going in sync

Description

This PR contains

  1. Verify Command
  2. Creating enums for command names
  3. Added new env variables
  4. Improved function names

Documentation Updated?

  • Yes
  • No

Under Feature Flag

  • Yes
  • No

Database Changes

  • Yes
  • No

Breaking Changes

  • Yes
  • No

Development Tested?

  • Yes
  • No

Screenshots

Details Screenshot 2025-03-18 at 8 14 20 AM Screenshot 2025-03-18 at 8 16 16 AM

Description by Korbit AI

What change is being made?

Sync development branch with main by updating deployment workflow, documentation, core service structure, command constants, error handling utilities, and comprehensive tests across services, handlers, and routes.

Why are these changes being made?

Standardize command naming and routing, improve error handling and JSON responses, and align tests with the main service flow for reliability and maintainability. This includes refactoring base controllers, routing, and service entry points to use a unified pattern.

Is this description stale? Ask me to generate a new description by commenting /korbit-generate-pr-description

prakashchoudhary07 and others added 4 commits February 22, 2025 23:40
* Test: Add unit test for ListeningService command handling

* Feat : Listed verify command in register command

* Feat : Added listener for Verify command

* Test : Added unit test for VerifyService

* Refactor: Replace HomeHandler with DiscordBaseHandler and move logic to DiscordBaseService

* Fix: Ensure request body is closed after processing in DiscordBaseService

* Refactor/59 constant command names (#60)

* Refactor: Centralize command names in utils and update handlers to use constants

* Refactor: Replace hardcoded command names with constants from utils
* feat: Add environment configuration and constants for development, staging, and production

* test: Add unit tests for environment validation logic

* refactor: Use fmt.Sprintf for constructing URLs in environment configuration
@coderabbitai
Copy link

coderabbitai bot commented Mar 18, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch develop

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

vikhyat187
vikhyat187 previously approved these changes May 3, 2025

var EnvironmentURLs = map[Environment]URLs{
Development: {
RDS_BASE_API_URL: "http://localhost:3000",
Copy link
Member

Choose a reason for hiding this comment

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

how come line 34 and 35 has localhost value assigned here, did we forgot to remove @vikhyat187

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for the local environment.

Copy link
Member

Choose a reason for hiding this comment

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

It's for the local environment.

then this is not supposed to be here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But is there any specific reason? because we have been using environment variables for 3 types of environments i.e., development, staging & production

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR Ref : #67

* test: add tests to verify the behaviour

* fix: change expected and actual order in assert statement

* fix: add tests and assert statement

* refactor: improve error handling and code organization

- Add JSON struct tags to DataPacket for proper serialization
- Improve error handling in jsonHandler to return errors instead of empty strings
- Remove responseGenerator in favor of direct string formatting
- Update test descriptions to be more accurate
- Fix MainHandler test to use realistic JSON test data
- Update test assertions to match expected behavior
- Clean up redundant utility functions and their tests
* test: improve health check handler test coverage

- Add test case for JSON encoding failure scenario
- Extract WriteResponse function for better testability
- Refactor existing health check tests into subtests
- Add validation for error response format and status code

* fix: change test name for better readability

* refactor: move WriteResponse to utils package

- Move WriteResponse function from controllers to utils package
- Add dedicated tests for WriteResponse utility
- Update healthCheckHandler to use utils.WriteResponse
- Fix test assertions for error response formatting
* refactor: convert ToByte to package function and enhance tests for service

* chore: remove empty lines

* chore: remove unwanted Sprintln

* fix: address typo in test name

* refactor: improve error handling in Discord service

- Add proper JSON field tags to DataPacket struct
- Change error response for invalid JSON from 500 to 400
- Return 400 instead of 200 for unknown message types
- Update test descriptions to be more accurate

* fix: change assert statement for failing test

* chore: fix merge conflict

* refactor: Move Body.Close defer statement

* refactor: remove unused variable and function call

* refactor: remove literals and used predefined constants
* chore: fix readme for devlopment setup

* chore: fix typo

* chore: fix make installation setup instruction

* chore:  change order of instruction

* chore: fix spacing

* chore: fix spelling

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

---------

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
* fix: add options in verify command

* chore: add test and move commands into constants package
* chore: remove hardcoded apis

* chore: remove unused constants

* chore: add BOT_PRIVATE_KEY for integration with website-backend

* chore: add env in test config

* chore: add main site url in env

* chore: add main-site-url in test config

* chore: fix typo

* chore: add verification site url in env setup
* fix: add constant and fix discord dto for verify command

* chore: fix nit comment
)

* fix: refactor json and error handler

* fix: add test for response handler and errors

* fix: resolved bot comments

* chore: remove err from unauthorised response

* fix: close request body after reading

* chore: remove duplicate code

* chore: pass err object to error handler to handle internal server error

* chore: fix failing tests
* feat: add verify service to process verify command

* chore: fix spacing

* chore: assert response body

* chore: resolve bot comment
* feat: add helper function and test for token generation for verify command

* chore: fix error statement and spacing

* chore: fix nit comment of bot

* chore: remove unnecessary error handling

* refactor: fix types and remove un-necesssary logs
…cation (#121)

* fixed service code to handle discord interaction ping properly

* changed return type to uint8 and updated test variable
…nt workflow (#120)

* feat: add handler to process verify command

* chore: fix token handler access

test: add tests for verify handler

* fix: create single object for unique and auth token

fix: refactor test for token utils

feat: add interface and fix failing tests

* fix: add missing env in deployment workflow

* chore: fix env access

* chore: fix formatting

* chore: remove duplicate test

* flattened nested if statements

* changed_test_method_name

* updated and removed global variables and changed expiry time

* Apply suggestion from @graphite-app[bot]

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

---------

Co-authored-by: Suvidh <kaushiksuvidh6@gmail.com>
Co-authored-by: Amit Prakash <34869115+iamitprakash@users.noreply.github.com>
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
Comment on lines +18 to +26
type mockDiscordSession struct {
*discordgo.Session
capturedMessage *string
}

func (m *mockDiscordSession) WebhookMessageEdit(webhookID, token, messageID string, data *discordgo.WebhookEdit, options ...discordgo.RequestOption) (*discordgo.Message, error) {
if m.capturedMessage != nil {
*m.capturedMessage = *data.Content
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The mockDiscordSession struct is missing the GuildMemberNickname method which is required by the DiscordSessionWrapper interface. To ensure the mock fully implements the interface, please add this method to the struct implementation. This will prevent potential interface compliance issues during testing.

Suggested change
type mockDiscordSession struct {
*discordgo.Session
capturedMessage *string
}
func (m *mockDiscordSession) WebhookMessageEdit(webhookID, token, messageID string, data *discordgo.WebhookEdit, options ...discordgo.RequestOption) (*discordgo.Message, error) {
if m.capturedMessage != nil {
*m.capturedMessage = *data.Content
}
type mockDiscordSession struct {
*discordgo.Session
capturedMessage *string
}
func (m *mockDiscordSession) WebhookMessageEdit(webhookID, token, messageID string, data *discordgo.WebhookEdit, options ...discordgo.RequestOption) (*discordgo.Message, error) {
if m.capturedMessage != nil {
*m.capturedMessage = *data.Content
}
return nil, nil
}
func (m *mockDiscordSession) GuildMemberNickname(guildID, userID, nick string) error {
return nil
}

Spotted by Diamond

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

* fix: testing the trigger for deployment

* feat: update deployment workflow to tag Docker image with 'latest' and add private/public key files

* fix: correct formatting and spacing in deployment script for better readability
Comment on lines +13 to +27
type mockMainTestDiscordSession struct {
*discordgo.Session
}

func (m *mockMainTestDiscordSession) WebhookMessageEdit(webhookID, token, messageID string, data *discordgo.WebhookEdit, options ...discordgo.RequestOption) (*discordgo.Message, error) {
return nil, nil
}

func (m *mockMainTestDiscordSession) mockGuildMemberNickname(guildID, userID, nickname string, options ...discordgo.RequestOption) error {
panic("GuildMemberNickname called")
}

func (m *mockMainTestDiscordSession) mockClose() error {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The mockMainTestDiscordSession struct doesn't correctly implement the DiscordSessionWrapper interface. The interface requires a Close() method, but the mock implements mockClose() instead. To properly satisfy the interface contract, rename the method from mockClose() to Close().

Suggested change
type mockMainTestDiscordSession struct {
*discordgo.Session
}
func (m *mockMainTestDiscordSession) WebhookMessageEdit(webhookID, token, messageID string, data *discordgo.WebhookEdit, options ...discordgo.RequestOption) (*discordgo.Message, error) {
return nil, nil
}
func (m *mockMainTestDiscordSession) mockGuildMemberNickname(guildID, userID, nickname string, options ...discordgo.RequestOption) error {
panic("GuildMemberNickname called")
}
func (m *mockMainTestDiscordSession) mockClose() error {
return nil
}
type mockMainTestDiscordSession struct {
*discordgo.Session
}
func (m *mockMainTestDiscordSession) WebhookMessageEdit(webhookID, token, messageID string, data *discordgo.WebhookEdit, options ...discordgo.RequestOption) (*discordgo.Message, error) {
return nil, nil
}
func (m *mockMainTestDiscordSession) mockGuildMemberNickname(guildID, userID, nickname string, options ...discordgo.RequestOption) error {
panic("GuildMemberNickname called")
}
func (m *mockMainTestDiscordSession) Close() error {
return nil
}

Spotted by Diamond

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

iamitprakash and others added 5 commits September 17, 2025 02:11
…ore-fix-build branch (#129)

* fix: update deploy workflow to use lowercase repo name and include chore-fix-build branch

* fix: remove chore-fix-build branch from deploy workflow triggers
* fix: remove lowercase repo name handling from deploy workflow

* fix: add missing environment variables for Docker container in deploy workflow
* fix: remove lowercase repo name handling from deploy workflow

* fix: add missing environment variables for Docker container in deploy workflow

* fix: correct syntax for environment variable assignments in Docker run command
@iamitprakash iamitprakash merged commit dfdc753 into main Sep 16, 2025
5 checks passed
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.

7 participants