[#13119] Admin: Unable to create an instructor account if the same email was used for a student account #13360
Conversation
Previously, admin could not approve an account request if any account with the same email existed, including student accounts. This was too restrictive, as only instructor accounts in the same institute should block approval. This commit updates the approval logic to only prevent approval if there is an existing instructor account with the same email in the same institute as the account request. The check now uses `logic.isInstructorWithEmailInInstitute(email, institute)`, which searches for instructors with the given email and verifies if any belong to a course under the specified institute. This allows admins to approve instructor account requests even if a student account with the same email exists, as long as there is no conflicting instructor account in the same institute.
- Add getInstructorsForEmail method in InstructorsDb to retrieve all instructors with a specific email address. - Add corresponding forwarding method in Logic. - Refactor InstructorsLogic.isExistingInstructorWithEmailInInstitute to use getInstructorsForEmail instead of searchInstructorsInWholeSystem, reducing dependency on search service. - Update error message in UpdateAccountRequestAction to clarify duplicate instructor account condition.
- Update UpdateAccountRequestActionIT test - Fix checkstyle errors
- Update the error message in UpdateAccountRequestAction and related tests to be more descriptive, mentioning that an instructor account with the same email under the same institute already exists.
Adding a trivial comment to trigger a new CI workflow run. This is to re-verify the failing integration test (Job ID: 46799151757).
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where admins couldn't create instructor accounts when a student account already existed with the same email address. The solution refines the validation logic to only prevent instructor account creation when there's an existing instructor account with the same email in the same institute, rather than blocking for any account type.
- Replaced broad email conflict check with institute-specific instructor validation
- Added new methods to check for existing instructors by email within a specific institute
- Updated error messaging to be more specific about the conflict type
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| UpdateAccountRequestAction.java | Updated approval logic to check for instructor conflicts only within the same institute |
| InstructorsDb.java | Added database layer method to retrieve instructors by email |
| InstructorsLogic.java | Added business logic to check for existing instructors by email in a specific institute |
| Logic.java | Added public API method for instructor existence validation |
| UpdateAccountRequestActionIT.java | Updated integration tests to verify the new behavior and error messages |
| if (instructor.getEmail().equals(email) | ||
| && institute.equals(coursesLogic.getCourseInstitute(instructor.getCourseId()))) { |
There was a problem hiding this comment.
The email comparison instructor.getEmail().equals(email) is redundant since getInstructorsForEmail(email) already filters by email. This condition will always be true and should be removed.
| if (instructor.getEmail().equals(email) | |
| && institute.equals(coursesLogic.getCourseInstitute(instructor.getCourseId()))) { | |
| if (institute.equals(coursesLogic.getCourseInstitute(instructor.getCourseId()))) { |
There was a problem hiding this comment.
Please take a look at this copilot suggestion.
|
|
||
| /** | ||
| * Checks if there is an existing instructor with the given email | ||
| * in any course under that belongs to the specified institute. |
There was a problem hiding this comment.
Grammar error in the comment: 'under that belongs to' should be 'that belongs to'.
| * in any course under that belongs to the specified institute. | |
| * in any course that belongs to the specified institute. |
|
|
||
| /** | ||
| * Returns the typical course attributes. | ||
| * This is a placeholder method because getTypicalCourse is not compatible, using the newer sql |
There was a problem hiding this comment.
Grammar error in the comment: 'using the newer sql' should be 'as it uses the newer SQL'.
| * This is a placeholder method because getTypicalCourse is not compatible, using the newer sql | |
| * This is a placeholder method because getTypicalCourse is not compatible, as it uses the newer SQL |
There was a problem hiding this comment.
Thank you for the PR. The code changes look correct, minimal, and aligned with architecture and business rules. However, the PR currently lacks an integration test that specifically reproduces the original bug (student → instructor email reuse) to prove the fix. Adding that test is critical to ensure the fix addresses issue #13119 and to prevent regressions. Please add this test to UpdateAccountRequestActionIT.
| * Checks if there is an existing instructor with the given email | ||
| * in any course under that belongs to the specified institute. | ||
| */ | ||
| public boolean isExistingInstructorWithEmailInInstitute(String email, String institute) { |
There was a problem hiding this comment.
Perhaps you could standardize isExistingInstructorWithEmailInInstitute and isInstructorWithEmailInInstitute to existsInstructorWithEmailInInstitute.
| if (instructor.getEmail().equals(email) | ||
| && institute.equals(coursesLogic.getCourseInstitute(instructor.getCourseId()))) { |
There was a problem hiding this comment.
Please take a look at this copilot suggestion.
- standardized isExistingInstructorWithEmailInInstitute and isInstructorWithEmailInInstitute to existsInstructorWithEmailInInstitute for better clarity - fixed a few grammatical issues in comments
|
Hi @DhiraPT, thanks for getting back! I have made the changes and added the relevant integration test to |
2 participants
Fixes #13119
Cause
Previously, admins could not approve an account request if any account with the same email existed, including student accounts. This was due to the
UpdateAccountRequestActionclass calling thegetAccountsForEmailWithTransactionmethod fromsqlLogic, which gets all accounts (regardless of if those were student-only or instructor accounts) linked to a particular email. This was too restrictive, as only instructor accounts in the same institute should block approval.Outline of Solution
isInstructorWithEmailInInstitute(email, institute)into theLogicclass, which searches for instructors with the given email and verifies if it belongs to any course under the specified institute.