[#11878] Add status and comments to AccountRequest

[jayasting98]

Part of #11878

Outline of Solution

Added status (and its type, AccountRequestStatus) and comments attributes to AccountRequest.

[xenosf]

LGTM! Just one comment

[xenosf]

Could the plural name comments be somewhat ambiguous? A singular form name might be clearer in indicating that it is a single String and not a collection. (same with the other String comments elsewhere in the code)

[jayasting98]

I was also thinking this. However, I felt the singular, comment, didn't feel exactly right either. Also, I saw teammates.storage.sqlentity.Student also had a similar plural comments attribute, despite not being a collection, but a mere string.

teammates/src/main/java/teammates/storage/sqlentity/Student.java

Lines 15 to 23 in 2342189

	/**
	* Represents a Student.
	*/
	@Entity
	@Table(name = "Students")
	public class Student extends User {
	@Column(nullable = false)
	private String comments;

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.

[xenosf]

I agree that it is hard to find an alternative name that sounds right. (I was thinking something along the lines of commentsText, but that seems a bit clunky.)

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 comments is probably OK too.

[jayasting98]

@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.

[xenosf]

LGTM

[EuniceSim142]

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.

[jayasting98]

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 APPROVED because it does in fact correspond to registeredAt being null. What you are describing, where registeredAt is non-null and an account has been created, actually corresponds to REGISTERED.

There was also a discussion on this in the issue. It was long ago though, so maybe you missed it. Just in case, see:

• Account Request Form Upgrade #11878 (comment)
• Account Request Form Upgrade #11878 (comment)
• Account Request Form Upgrade #11878 (comment)

Of course, there is the case where it is set to APPROVED when it should actually in fact be REGISTERED, 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 APPROVED if registeredAt is null or REGISTERED otherwise, instead of being default APPROVED always. 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

[EuniceSim142]

Other than my comment above, everything else lgtm!