[#11878] Add status and comments to AccountRequest #12898
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| package teammates.common.datatransfer; | ||
|
|
||
| /** | ||
| * The status of an account request. | ||
| */ | ||
| public enum AccountRequestStatus { | ||
|
|
||
| /** | ||
| * The account request has not yet been processed by the admin. | ||
| */ | ||
| PENDING, | ||
|
|
||
| /** | ||
| * The account request has been rejected by the admin. | ||
| */ | ||
| REJECTED, | ||
|
|
||
| /** | ||
| * The account request has been approved by the admin but the instructor has not created an account yet. | ||
| */ | ||
| APPROVED, | ||
|
|
||
| /** | ||
| * The account request has been approved by the admin and the instructor has created an account. | ||
| */ | ||
| REGISTERED | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,13 +9,17 @@ | |
|
|
||
| import org.hibernate.annotations.UpdateTimestamp; | ||
|
|
||
| import teammates.common.datatransfer.AccountRequestStatus; | ||
| import teammates.common.util.Config; | ||
| import teammates.common.util.Const; | ||
| import teammates.common.util.FieldValidator; | ||
| import teammates.common.util.SanitizationHelper; | ||
| import teammates.common.util.StringHelper; | ||
|
|
||
| import jakarta.persistence.Column; | ||
| import jakarta.persistence.Entity; | ||
| import jakarta.persistence.EnumType; | ||
| import jakarta.persistence.Enumerated; | ||
| import jakarta.persistence.Id; | ||
| import jakarta.persistence.Table; | ||
| import jakarta.persistence.UniqueConstraint; | ||
|
|
@@ -40,6 +44,12 @@ public class AccountRequest extends BaseEntity { | |
|
|
||
| private String institute; | ||
|
|
||
| @Enumerated(EnumType.STRING) | ||
| private AccountRequestStatus status; | ||
|
|
||
| @Column(columnDefinition = "TEXT") | ||
| private String comments; | ||
|
There was a problem hiding this comment. Could the plural name There was a problem hiding this comment. I was also thinking this. However, I felt the singular, teammates/src/main/java/teammates/storage/sqlentity/Student.java Lines 15 to 23 in 2342189 Nevertheless, I can change this, but I think we will also have to change this in the tech design doc (I see it in some places there too), if you still insist. There was a problem hiding this comment. I agree that it is hard to find an alternative name that sounds right. (I was thinking something along the lines of There may be a bit of confusion as the words "comment"/"comments" is also used in the context of feedback response comments. In that context, the plural "comments" would almost always be referring to a collection. However, seeing as that context is quite separate from this one, leaving the string's name as There was a problem hiding this comment. @xenosf I think for this one, we can consider merging for now to get the ball rolling first. We can rename it later, if necessary, before the ARF feature branch is merged. |
||
|
|
||
| private Instant registeredAt; | ||
|
|
||
| @UpdateTimestamp | ||
|
|
@@ -49,11 +59,13 @@ protected AccountRequest() { | |
| // required by Hibernate | ||
| } | ||
|
|
||
| public AccountRequest(String email, String name, String institute, AccountRequestStatus status, String comments) { | ||
| this.setId(UUID.randomUUID()); | ||
| this.setEmail(email); | ||
| this.setName(name); | ||
| this.setInstitute(institute); | ||
| this.setStatus(status); | ||
| this.setComments(comments); | ||
| this.generateNewRegistrationKey(); | ||
| this.setCreatedAt(Instant.now()); | ||
| this.setRegisteredAt(null); | ||
|
|
@@ -128,6 +140,22 @@ public void setInstitute(String institute) { | |
| this.institute = SanitizationHelper.sanitizeTitle(institute); | ||
| } | ||
|
|
||
| public AccountRequestStatus getStatus() { | ||
| return this.status; | ||
| } | ||
|
|
||
| public void setStatus(AccountRequestStatus status) { | ||
| this.status = status; | ||
| } | ||
|
|
||
| public String getComments() { | ||
| return this.comments; | ||
| } | ||
|
|
||
| public void setComments(String comments) { | ||
| this.comments = comments; | ||
| } | ||
|
|
||
| public Instant getRegisteredAt() { | ||
| return this.registeredAt; | ||
| } | ||
|
|
@@ -166,8 +194,8 @@ public int hashCode() { | |
| @Override | ||
| public String toString() { | ||
| return "AccountRequest [id=" + id + ", registrationKey=" + registrationKey + ", name=" + name + ", email=" | ||
| + email + ", institute=" + institute + ", status=" + status + ", comments=" + comments | ||
| + ", registeredAt=" + registeredAt + ", createdAt=" + getCreatedAt() + ", updatedAt=" + updatedAt + "]"; | ||
| } | ||
|
|
||
| public String getRegistrationUrl() { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If we set all the old account requests to approved, is it possible to result in inconsistencies with Accounts db?
Eg. if some account request in datastore has registeredAt == null, so not approved yet. If we migrate, then that account request will have status = approve, but regKey and regAt are null and no account in accounts db.
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.
From my understanding, the feature branch will only be merged after the migration is completed, right? Is this no longer the case? Assuming this is still the case, the script should not be used anymore when this is merged so I was not too careful on this. This was mainly placed there to prevent compilation errors. 😅
Regardless, I did specifically choose
APPROVEDbecause it does in fact correspond toregisteredAtbeing null. What you are describing, whereregisteredAtis non-null and an account has been created, actually corresponds toREGISTERED.There was also a discussion on this in the issue. It was long ago though, so maybe you missed it. Just in case, see:
Of course, there is the case where it is set to
APPROVEDwhen it should actually in fact beREGISTERED, but I felt it was not a big problem because of the assumption above, and because the account was already created so the account request could then already be ignored.Still, I can try to change this so that it is more accurate by making it such that it is
APPROVEDifregisteredAtis null orREGISTEREDotherwise, instead of being defaultAPPROVEDalways. This way, it should be correct even if migration is done after the feature branch is merged. In order for the other PR that depends on this to be submitted, let's merge this first; I will make this change to the migration script in a separate PR. I hope this is okay, @EuniceSim142