Copilot commented on code in PR #6402:
URL: https://github.com/apache/texera/pull/6402#discussion_r3635677402
##########
frontend/src/app/hub/component/about/local-login/local-login.component.ts:
##########
@@ -143,8 +144,14 @@ export class LocalLoginComponent implements OnInit {
this.registerErrorMessage = undefined;
const registerPassword = this.allForms.get("registerPassword")?.value;
const registerConfirmationPassword =
this.allForms.get("registerConfirmationPassword")?.value;
+ const registerEmail = this.allForms.get("registerEmail")?.value.trim();
const registerUsername =
this.allForms.get("registerUsername")?.value.trim();
const validation = UserService.validateUsername(registerUsername);
+ const emailValidation = UserService.validateEmail(registerEmail);
Review Comment:
`register()` calls `validateUsername()` before `validateEmail()`, so a bad
email cannot short-circuit username validation (and it contradicts the new unit
test intent). Also `...?.value.trim()` can throw if the control value is
null/undefined. Reorder the validations and use nullish coalescing before
trimming.
##########
frontend/src/app/hub/component/about/local-login/local-login.component.spec.ts:
##########
@@ -306,10 +323,72 @@ describe("LocalLoginComponent", () => {
expect(userServiceMock.register).not.toHaveBeenCalled();
});
+ it("sets registerErrorMessage when the email is empty", () => {
+ vi.spyOn(UserService, "validateEmail").mockReturnValue({
+ result: false,
+ message: "Email should not be empty.",
+ });
+ component.allForms.patchValue({
+ registerUsername: "alice",
+ registerEmail: "",
+ registerPassword: "abcdef",
+ registerConfirmationPassword: "abcdef",
+ });
+
+ component.register();
+
+ expect(component.registerErrorMessage).toBe("Email should not be
empty.");
+ expect(userServiceMock.register).not.toHaveBeenCalled();
+ });
+
+ it("sets registerErrorMessage when the email is malformed", () => {
+ vi.spyOn(UserService, "validateEmail").mockReturnValue({
+ result: false,
+ message: "Email format is invalid.",
+ });
+ component.allForms.patchValue({
+ registerUsername: "alice",
+ registerEmail: "not-an-email",
+ registerPassword: "abcdef",
+ registerConfirmationPassword: "abcdef",
+ });
+
+ component.register();
+
+ expect(component.registerErrorMessage).toBe("Email format is invalid.");
+ expect(userServiceMock.register).not.toHaveBeenCalled();
+ });
+
+ it("checks email validity before username validity", () => {
+ // Email validation runs before username validation in register(), so a
+ // bad email must short-circuit the flow even if username is also bad.
+ const validateUsernameSpy = vi
+ .spyOn(UserService, "validateUsername")
+ .mockReturnValue({ result: false, message: "Username should not be
empty." });
+ const validateEmailSpy = vi
+ .spyOn(UserService, "validateEmail")
+ .mockReturnValue({ result: false, message: "Email format is invalid."
});
+ component.allForms.patchValue({
+ registerUsername: "",
+ registerEmail: "not-an-email",
+ registerPassword: "abcdef",
+ registerConfirmationPassword: "abcdef",
+ });
+
+ component.register();
+
+ expect(component.registerErrorMessage).toBe("Email format is invalid.");
+ expect(validateUsernameSpy).not.toHaveBeenCalled();
+ expect(validateEmailSpy).toHaveBeenCalledWith("not-an-email");
+ expect(userServiceMock.register).not.toHaveBeenCalled();
+ });
+
it("calls UserService.register with the trimmed username and surfaces a
success notification", () => {
vi.spyOn(UserService, "validateUsername").mockReturnValue({ result:
true, message: "ok" });
+ vi.spyOn(UserService, "validateEmail").mockReturnValue({ result: true,
message: "ok" });
component.allForms.patchValue({
registerUsername: " alice ",
+ registerEmail: " [email protected] ",
registerPassword: "abcdef",
Review Comment:
This test updates the form to include `registerEmail`, but it still asserts
the old 2-arg call shape for `userService.register`. Since the component now
calls `register(username, email, password)`, the expectation should include the
trimmed email argument as well.
##########
amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala:
##########
@@ -103,13 +103,16 @@ class AuthResource {
@Path("/register")
def register(request: UserRegistrationRequest): TokenIssueResponse = {
val username = request.username
- if (username == null) throw new NotAcceptableException("Username cannot be
null.")
- if (username.trim.isEmpty) throw new NotAcceptableException("Username
cannot be empty.")
+ val useremail = request.email
+ if (username == null || username.trim.isEmpty)
+ throw new NotAcceptableException("Username cannot be empty")
+ if (useremail == null || useremail.trim.isEmpty)
+ throw new NotAcceptableException("Email cannot be empty")
Review Comment:
Backend registration currently stores raw `request.username`/`request.email`
(including leading/trailing whitespace) and uses a non-idiomatic `useremail`
name. Trimming early avoids duplicate-user edge cases and persisting
whitespace, and also makes the null/empty checks and error messages more
consistent.
##########
frontend/src/app/common/service/user/user.service.ts:
##########
@@ -81,9 +81,9 @@ export class UserService {
this.changeUser(undefined);
}
- public register(username: string, password: string): Observable<void> {
+ public register(username: string, email: string, password: string):
Observable<void> {
return this.authService
- .register(username, password)
+ .register(username, email, password)
.pipe(switchMap(({ accessToken }) =>
this.handleAccessToken(accessToken)));
}
Review Comment:
`UserService.register` now requires `(username, email, password)`, but there
are still callers/tests using the old `(username, password)` signature (e.g.
`frontend/src/app/common/service/user/user.service.spec.ts`). Those need to be
updated to pass an email so the frontend build/tests compile and reflect the
new registration contract.
##########
frontend/src/app/common/service/user/auth.service.ts:
##########
@@ -62,13 +62,15 @@ export class AuthService {
* This method will handle the request for user registration.
* It will automatically login, save the user account inside and trigger
userChangeEvent when success
* @param username
+ * @param email
* @param password
*/
- public register(username: string, password: string): Observable<Readonly<{
accessToken: string }>> {
+ public register(username: string, email: string, password: string):
Observable<Readonly<{ accessToken: string }>> {
return this.http.post<Readonly<{ accessToken: string }>>(
`${AppSettings.getApiEndpoint()}/${AuthService.REGISTER_ENDPOINT}`,
{
username,
+ email,
password,
}
Review Comment:
`AuthService.register` signature changed to require `email`, but the
existing unit tests and any other callers still invoke it as
`register(username, password)` (e.g.
`frontend/src/app/common/service/user/auth.service.spec.ts` still expects `{
username, password }`). Update those call sites/tests to pass an email and
assert the new request body `{ username, email, password }` so the frontend
test suite compiles and validates the new contract.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
