mengw15 commented on code in PR #5460:
URL: https://github.com/apache/texera/pull/5460#discussion_r3380305607
##########
frontend/src/app/common/service/gmail/gmail.service.spec.ts:
##########
@@ -63,4 +64,76 @@ describe("GmailService", () => {
expect(notificationSpy.error).toHaveBeenCalledWith("Failed to send email.
Please try again or contact admin.");
expect(notificationSpy.success).not.toHaveBeenCalled();
});
+
+ it("sends the correct PUT body for sendEmail with an explicit receiver", ()
=> {
+ service.sendEmail("subj", "body", "[email protected]");
+
+ const req = httpTestingController.expectOne(r =>
r.url.endsWith("/gmail/send") && r.method === "PUT");
+ expect(req.request.body).toEqual({ receiver: "[email protected]", subject:
"subj", content: "body" });
+ req.flush(null);
+ });
+
+ it("defaults the receiver to an empty string when it is omitted", () => {
+ service.sendEmail("subj", "body");
+
+ const req = httpTestingController.expectOne(r =>
r.url.endsWith("/gmail/send") && r.method === "PUT");
+ expect(req.request.body).toEqual({ receiver: "", subject: "subj", content:
"body" });
+ req.flush(null);
+ });
Review Comment:
Not a bug — likely a vestigial default. If `receiver` is empty the backend
sends to the requester's own address (`if (receiver.isEmpty) user.getEmail`),
so nothing breaks. Both callers pass an explicit receiver anyway (share-access
only after email validation; the admin form requires the email field), so the
empty default isn't exercised in normal use. Agreed on pinning the test as-is;
we can open a low-priority follow-up to clean up the unused default.
##########
frontend/src/app/common/service/gmail/gmail.service.spec.ts:
##########
@@ -63,4 +64,76 @@ describe("GmailService", () => {
expect(notificationSpy.error).toHaveBeenCalledWith("Failed to send email.
Please try again or contact admin.");
expect(notificationSpy.success).not.toHaveBeenCalled();
});
+
+ it("sends the correct PUT body for sendEmail with an explicit receiver", ()
=> {
+ service.sendEmail("subj", "body", "[email protected]");
+
+ const req = httpTestingController.expectOne(r =>
r.url.endsWith("/gmail/send") && r.method === "PUT");
+ expect(req.request.body).toEqual({ receiver: "[email protected]", subject:
"subj", content: "body" });
+ req.flush(null);
+ });
+
+ it("defaults the receiver to an empty string when it is omitted", () => {
+ service.sendEmail("subj", "body");
+
+ const req = httpTestingController.expectOne(r =>
r.url.endsWith("/gmail/send") && r.method === "PUT");
+ expect(req.request.body).toEqual({ receiver: "", subject: "subj", content:
"body" });
+ req.flush(null);
+ });
+
+ it("logs the underlying error to the console on a failed send", () => {
+ const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {});
+ service.sendEmail("subj", "body", "[email protected]");
+
+ const req = httpTestingController.expectOne(r =>
r.url.endsWith("/gmail/send") && r.method === "PUT");
+ req.flush("boom", { status: 502, statusText: "Bad Gateway" });
+
+ expect(consoleSpy).toHaveBeenCalledWith("Send email error:", "boom");
+ });
+
+ it("issues a GET to the sender-email endpoint and emits the response without
notifying", () => {
+ let emitted: string | undefined;
+ service.getSenderEmail().subscribe(value => (emitted = value as string));
+
+ const req = httpTestingController.expectOne(
+ r => r.url.endsWith("/gmail/sender/email") && r.method === "GET" &&
r.responseType === "text"
+ );
+ req.flush("[email protected]");
+
+ expect(emitted).toBe("[email protected]");
+ expect(notificationSpy.success).not.toHaveBeenCalled();
+ expect(notificationSpy.error).not.toHaveBeenCalled();
+ });
Review Comment:
`getSenderEmail` is only used on the admin Gmail-config page (`ngOnInit`) to
display the configured sender address — a read-only fetch on an admin-only
screen, and the component already logs failures to the console. A toast there
would mostly be page-load noise, so I don't think we need to notify the user.
Keeping the service a thin, non-notifying GET (unlike `sendEmail`, which
toasts) is intentional. I'd leave it as-is.
##########
frontend/src/app/common/service/gmail/gmail.service.spec.ts:
##########
@@ -63,4 +64,76 @@ describe("GmailService", () => {
expect(notificationSpy.error).toHaveBeenCalledWith("Failed to send email.
Please try again or contact admin.");
expect(notificationSpy.success).not.toHaveBeenCalled();
});
+
+ it("sends the correct PUT body for sendEmail with an explicit receiver", ()
=> {
+ service.sendEmail("subj", "body", "[email protected]");
+
+ const req = httpTestingController.expectOne(r =>
r.url.endsWith("/gmail/send") && r.method === "PUT");
+ expect(req.request.body).toEqual({ receiver: "[email protected]", subject:
"subj", content: "body" });
+ req.flush(null);
+ });
+
+ it("defaults the receiver to an empty string when it is omitted", () => {
+ service.sendEmail("subj", "body");
+
+ const req = httpTestingController.expectOne(r =>
r.url.endsWith("/gmail/send") && r.method === "PUT");
+ expect(req.request.body).toEqual({ receiver: "", subject: "subj", content:
"body" });
+ req.flush(null);
+ });
+
+ it("logs the underlying error to the console on a failed send", () => {
+ const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {});
+ service.sendEmail("subj", "body", "[email protected]");
+
+ const req = httpTestingController.expectOne(r =>
r.url.endsWith("/gmail/send") && r.method === "PUT");
+ req.flush("boom", { status: 502, statusText: "Bad Gateway" });
+
+ expect(consoleSpy).toHaveBeenCalledWith("Send email error:", "boom");
+ });
Review Comment:
Yes — the same `error` handler shows a user-facing toast
(`notificationService.error("Failed to send email. Please try again or contact
admin.")`) in addition to the console log. The toast was asserted in a separate
test; I've merged it into this one so the failure case asserts both the toast
and the console log together (matching the `notifyUnauthorizedLogin` failure
test).
--
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]
