[#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 |
src/it/java/teammates/it/ui/webapi/UpdateAccountRequestActionIT.java
Outdated
Show resolved
Hide resolved
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.
- 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 |
DhiraPT
left a comment
DhiraPT
left a comment
There was a problem hiding this comment.
Thanks! Please import the entities first before using them (I found this in several places).
| public boolean existsInstructorWithEmailInInstitute(String email, String institute) { | ||
| assert email != null; | ||
| assert institute != null; | ||
|
|
||
| List<InstructorAttributes> instructors = getInstructorsForEmail(email); | ||
| for (InstructorAttributes instructor : instructors) { | ||
| if (institute.equals(coursesLogic.getCourseInstitute(instructor.getCourseId()))) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
getCourseInstitute triggers a Datastore read. If an instructor belongs to 10 courses, this results in 11 queries (1 fetch for instructors + 10 fetches for courses). Would be better to fetch all courses in a single batch query using coursesLogic.getCourses(List<String> courseIds) to avoid the N+1 problem.
There was a problem hiding this comment.
Thanks for the suggestion! It does sound like a better solution. I have added a new getCourses method to do this batch fetching
| ______TS("email with existing account throws exception"); | ||
| ______TS("email with existing student account (only) under same institute should approve successfully"); | ||
| Account studentAccount = logic.createAccountWithTransaction(getTypicalStudentAccount()); | ||
| teammates.storage.sqlentity.Course studentCourse = getTypicalStudentCourse(); |
There was a problem hiding this comment.
have added the necessary imports :)
- ensure entities used are properly imported - use batch fetching for courses in InstructorsLogic to avoid cascading reads
3 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.