-
Notifications
You must be signed in to change notification settings - Fork 172
E2565 : Integration of JoinTeamRequests #241
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
base: main
Are you sure you want to change the base?
Conversation
…orrect data to frontend
added create student team method inside student_teams controller modified logic of participant accepting invitation modified serializer
added from_participant reference inside invitations schema modified retract invitations functionality team serializer now takes participant serializer added more validations inside invitation validator
…student_teams controller and added corresponding routes added advertisement related fields inside signedup teams table
…e properly integrated with the frontend.
| join_team_request.team_id = params[:team_id] | ||
| participant = Participant.where(user_id: @current_user.id, assignment_id: params[:assignment_id]).first | ||
| team = Team.find(params[:team_id]) | ||
| # Find participant based on assignment_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find participant object for the user who is requesting to join the team
| ) | ||
|
|
||
| if existing_request | ||
| return render json: { error: 'You already have a pending request for this team' }, status: :unprocessable_entity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a pending request to join this team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds an "Advertisements" feature that enables teams to advertise for partners and allows students to request to join advertising teams. However, the PR contains several critical security issues that must be addressed before merging.
Key Changes:
- Added comprehensive test coverage for advertisements, join team requests, and invitation acceptance workflows
- Modified invitation status constants from
REJECT_STATUStoDECLINED_STATUS - Added test coverage for email notifications on join request and invitation acceptance
Reviewed changes
Copilot reviewed 99 out of 104 changed files in this pull request and generated 31 comments.
Show a summary per file
| File | Description |
|---|---|
| users.ibd | CRITICAL: Binary database file that should not be committed to version control |
| swagger/v1/swagger.yaml | CRITICAL: Hardcoded IP address in API configuration - should use localhost or environment variables |
| spec/swagger_helper.rb | CRITICAL: Hardcoded IP address in test configuration |
| spec/spec_helper.rb | Modified SimpleCov startup logic and commented out Coveralls configuration |
| spec/requests/api/v1/join_team_requests_controller_spec.rb | New comprehensive test suite for join team request authorization and functionality |
| spec/requests/api/v1/invitation_controller_spec.rb | Updated invitation tests with new participant setup and renamed constants |
| spec/requests/api/v1/advertisements_spec.rb | New comprehensive test suite covering advertisement display, creation, and join request workflows |
| spec/requests/api/v1/acceptance_email_integration_spec.rb | New integration tests for email notifications on acceptance |
| spec/rails_helper.rb | Commented out fixture path configuration |
| spec/models/invitation_spec.rb | Added email notification tests and updated constant names |
| get_token.sh | CRITICAL: New shell script with hardcoded IP address and credentials for JWT token retrieval |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| response(200, 'Update successful') do | ||
| let(:id) { invitation.id } | ||
| let(:invitation_status) { { reply_status: InvitationValidator::DECLINED_STATUS } } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constant has been changed from REJECT_STATUS to DECLINED_STATUS. Ensure this constant name change is consistent throughout the codebase and that InvitationValidator::DECLINED_STATUS is properly defined. This appears to be a breaking API change if the constant was used elsewhere.
| invitation = Invitation.create(to_id: user1.id, from_id: user2.id, assignment_id: assignment.id) | ||
| invitation.decline_invitation(nil) | ||
| expect(invitation.reply_status).to eq(InvitationValidator::REJECT_STATUS) | ||
| expect(invitation.reply_status).to eq(InvitationValidator::DECLINED_STATUS) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constant has been changed from REJECT_STATUS to DECLINED_STATUS. Ensure this constant name change is consistent throughout the codebase and that InvitationValidator::DECLINED_STATUS is properly defined. This appears to be a breaking API change if the constant was used elsewhere.
| if !ENV['COVERAGE_STARTED'] | ||
| SimpleCov.start 'rails' | ||
| ENV['COVERAGE_STARTED'] = 'true' | ||
| end |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an environment variable to control whether SimpleCov starts is unusual and may cause issues with coverage tracking. If multiple test files are loaded, only the first one will start SimpleCov, potentially leading to incomplete coverage data. Consider removing this check or documenting why this pattern is necessary. The standard approach is to let SimpleCov start once at the beginning of the test suite in spec_helper.rb.
| expect { | ||
| patch "/api/v1/join_team_requests/#{request_id}/accept", headers: team_member_headers | ||
| }.to have_enqueued_job(ActionMailer::MailDeliveryJob) | ||
|
|
||
| expect(response).to have_http_status(:ok) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test expects a job to be enqueued within the expect block but uses have_enqueued_job after the action has already occurred. This pattern can lead to flaky tests. Consider using enqueue_job matcher syntax or performing the action within the expect block:
expect {
patch "/api/v1/join_team_requests/#{request_id}/accept", headers: team_member_headers
}.to have_enqueued_job(ActionMailer::MailDeliveryJob)The response status check should be done after this assertion.
| variables: | ||
| defaultHost: | ||
| default: localhost:3002 | ||
| default: 152.7.176.23:3002 |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded IP address 152.7.176.23:3002 should not be committed to the repository. This appears to be a specific server IP address that:
- May expose internal infrastructure details
- Will not work for other developers or environments
- Should be configured through environment variables or configuration files
Please revert this to localhost:3002 or use an environment variable like ENV['API_HOST'] with appropriate defaults for different environments.
| # answer for scorable questions, and they will not be counted towards the total score) | ||
| total_weight = 0 | ||
| scores.each do |s| | ||
| item = Item.find(s.item_id) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call to a database query operation happens inside this loop, and could be hoisted to a single call outside the loop.
| maps.each do |map| | ||
| next if map.response.empty? | ||
|
|
||
| all_resp = Response.where(map_id: map.map_id).last |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call to a database query operation happens inside this loop, and could be hoisted to a single call outside the loop.
|
|
||
| ((num_students / 2)..num_students-1).each do |i| | ||
| user_id = student_user_ids[i] | ||
| handle = User.find(user_id).handle |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call to a database query operation happens inside this loop, and could be hoisted to a single call outside the loop.
|
|
||
| def courses_assisted_with | ||
| courses = TaMapping.where(ta_id: id) | ||
| courses.map { |c| Course.find(c.course_id) } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call to a database query operation happens inside this loop, and could be hoisted to a single call outside the loop.
| echo "" | ||
|
|
||
| # Make login request | ||
| response=$(curl -s -X POST http://152.7.176.23:3002/login \ |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script sends login credentials and JWT tokens to http://152.7.176.23:3002 over plain HTTP (see this line and similar uses on lines 25 and 52), which exposes passwords and bearer tokens to interception or tampering by any attacker on the network path. An attacker sniffing traffic on the same network could steal the credentials or token and impersonate the user or replay API calls. Switch these calls to an HTTPS endpoint (or gate HTTP strictly to local-only development), and ensure the base URL is configurable so that sensitive environments always use TLS.
No description provided.