Copilot commented on code in PR #6527:
URL: https://github.com/apache/texera/pull/6527#discussion_r3609358053
##########
frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts:
##########
@@ -49,4 +81,179 @@ describe("AdminSettingsComponent", () => {
expect(el.textContent?.trim()).toBe("MiB");
});
});
+
+ describe("branding", () => {
+ it("saveLogos persists only the branding assets that are set and notifies
success", () => {
+ component.logoData = "data:image/png;base64,LOGO";
+ component.miniLogoData = "data:image/png;base64,MINI";
+ component.faviconData = null;
+
+ component.saveLogos();
+
+ expect(adminSettings.updateSetting).toHaveBeenCalledWith("logo",
"data:image/png;base64,LOGO");
+ expect(adminSettings.updateSetting).toHaveBeenCalledWith("mini_logo",
"data:image/png;base64,MINI");
+ expect(adminSettings.updateSetting).not.toHaveBeenCalledWith("favicon",
expect.anything());
+ expect(message.success).toHaveBeenCalledWith("Branding saved
successfully.");
+ });
+
+ it("saveLogos does nothing when no branding asset is set", () => {
+ component.logoData = null;
+ component.miniLogoData = null;
+ component.faviconData = null;
+
+ component.saveLogos();
+
+ expect(adminSettings.updateSetting).not.toHaveBeenCalled();
+ expect(message.success).not.toHaveBeenCalled();
+ });
+
+ it("saveLogos notifies an error when the request fails", () => {
+ component.logoData = "data:image/png;base64,LOGO";
+ adminSettings.updateSetting.mockReturnValue(throwError(() => new
Error("nope")));
+
+ component.saveLogos();
+
+ expect(message.error).toHaveBeenCalledWith("Failed to save branding.");
+ });
+
+ it("resetBranding resets all three branding settings", () => {
+ component.resetBranding();
+
+ expect(adminSettings.resetSetting).toHaveBeenCalledWith("logo");
+ expect(adminSettings.resetSetting).toHaveBeenCalledWith("mini_logo");
+ expect(adminSettings.resetSetting).toHaveBeenCalledWith("favicon");
+ expect(message.info).toHaveBeenCalledWith("Resetting branding...");
+ });
+ });
+
+ describe("tabs", () => {
+ it("saveTabs persists every sidebar tab and notifies success", () => {
+ const tabCount = Object.keys(component.sidebarTabs).length;
+
+ component.saveTabs();
+
+ expect(adminSettings.updateSetting).toHaveBeenCalledTimes(tabCount);
+ expect(adminSettings.updateSetting).toHaveBeenCalledWith("hub_enabled",
"false");
+ expect(message.success).toHaveBeenCalledWith("Tabs saved successfully.");
+ });
+
+ it("resetTabs resets every sidebar tab", () => {
+ const tabCount = Object.keys(component.sidebarTabs).length;
+
+ component.resetTabs();
+
+ expect(adminSettings.resetSetting).toHaveBeenCalledTimes(tabCount);
+ expect(message.info).toHaveBeenCalledWith("Resetting tabs...");
+ });
+ });
+
+ describe("dataset settings", () => {
+ beforeEach(() => {
+ component.maxConcurrentFiles = 3;
+ component.maxFileSizeMiB = 20;
+ component.maxConcurrentChunks = 10;
+ component.chunkSizeMiB = 50;
+ });
+
+ it("saveDatasetSettings persists the four upload settings and notifies
success", () => {
+ component.saveDatasetSettings();
+
+
expect(adminSettings.updateSetting).toHaveBeenCalledWith("max_number_of_concurrent_uploading_file",
"3");
+
expect(adminSettings.updateSetting).toHaveBeenCalledWith("single_file_upload_max_size_mib",
"20");
+
expect(adminSettings.updateSetting).toHaveBeenCalledWith("max_number_of_concurrent_uploading_file_chunks",
"10");
+
expect(adminSettings.updateSetting).toHaveBeenCalledWith("multipart_upload_chunk_size_mib",
"50");
+ expect(message.success).toHaveBeenCalledWith("Dataset upload settings
saved successfully.");
+ });
+
+ it("saveDatasetSettings rejects non-positive values without saving", () =>
{
+ component.maxFileSizeMiB = 0;
+
+ component.saveDatasetSettings();
+
+ expect(adminSettings.updateSetting).not.toHaveBeenCalled();
+ expect(message.error).toHaveBeenCalledWith("Please enter valid integer
values.");
+ });
+
Review Comment:
`saveDatasetSettings()` has an error handler (`message.error("Failed to save
dataset settings.")`), but the spec currently only covers success + validation
failures. Add a test that forces `updateSetting()` to error and asserts the
failure notification.
##########
frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts:
##########
@@ -49,4 +81,179 @@ describe("AdminSettingsComponent", () => {
expect(el.textContent?.trim()).toBe("MiB");
});
});
+
+ describe("branding", () => {
+ it("saveLogos persists only the branding assets that are set and notifies
success", () => {
+ component.logoData = "data:image/png;base64,LOGO";
+ component.miniLogoData = "data:image/png;base64,MINI";
+ component.faviconData = null;
+
+ component.saveLogos();
+
+ expect(adminSettings.updateSetting).toHaveBeenCalledWith("logo",
"data:image/png;base64,LOGO");
+ expect(adminSettings.updateSetting).toHaveBeenCalledWith("mini_logo",
"data:image/png;base64,MINI");
+ expect(adminSettings.updateSetting).not.toHaveBeenCalledWith("favicon",
expect.anything());
+ expect(message.success).toHaveBeenCalledWith("Branding saved
successfully.");
+ });
+
+ it("saveLogos does nothing when no branding asset is set", () => {
+ component.logoData = null;
+ component.miniLogoData = null;
+ component.faviconData = null;
+
+ component.saveLogos();
+
+ expect(adminSettings.updateSetting).not.toHaveBeenCalled();
+ expect(message.success).not.toHaveBeenCalled();
+ });
+
+ it("saveLogos notifies an error when the request fails", () => {
+ component.logoData = "data:image/png;base64,LOGO";
+ adminSettings.updateSetting.mockReturnValue(throwError(() => new
Error("nope")));
+
+ component.saveLogos();
+
+ expect(message.error).toHaveBeenCalledWith("Failed to save branding.");
+ });
+
+ it("resetBranding resets all three branding settings", () => {
+ component.resetBranding();
+
+ expect(adminSettings.resetSetting).toHaveBeenCalledWith("logo");
+ expect(adminSettings.resetSetting).toHaveBeenCalledWith("mini_logo");
+ expect(adminSettings.resetSetting).toHaveBeenCalledWith("favicon");
+ expect(message.info).toHaveBeenCalledWith("Resetting branding...");
+ });
+ });
+
+ describe("tabs", () => {
+ it("saveTabs persists every sidebar tab and notifies success", () => {
+ const tabCount = Object.keys(component.sidebarTabs).length;
+
+ component.saveTabs();
+
+ expect(adminSettings.updateSetting).toHaveBeenCalledTimes(tabCount);
+ expect(adminSettings.updateSetting).toHaveBeenCalledWith("hub_enabled",
"false");
+ expect(message.success).toHaveBeenCalledWith("Tabs saved successfully.");
+ });
+
+ it("resetTabs resets every sidebar tab", () => {
+ const tabCount = Object.keys(component.sidebarTabs).length;
+
+ component.resetTabs();
+
+ expect(adminSettings.resetSetting).toHaveBeenCalledTimes(tabCount);
+ expect(message.info).toHaveBeenCalledWith("Resetting tabs...");
+ });
+ });
Review Comment:
The tabs handler is only tested for the success path. Since `saveTabs()` has
an explicit error branch (`message.error("Failed to save tabs.")`), add a
regression test that forces `AdminSettingsService.updateSetting()` to fail and
asserts the error notification.
##########
frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts:
##########
@@ -49,4 +81,179 @@ describe("AdminSettingsComponent", () => {
expect(el.textContent?.trim()).toBe("MiB");
});
});
+
+ describe("branding", () => {
+ it("saveLogos persists only the branding assets that are set and notifies
success", () => {
+ component.logoData = "data:image/png;base64,LOGO";
+ component.miniLogoData = "data:image/png;base64,MINI";
+ component.faviconData = null;
+
+ component.saveLogos();
+
+ expect(adminSettings.updateSetting).toHaveBeenCalledWith("logo",
"data:image/png;base64,LOGO");
+ expect(adminSettings.updateSetting).toHaveBeenCalledWith("mini_logo",
"data:image/png;base64,MINI");
+ expect(adminSettings.updateSetting).not.toHaveBeenCalledWith("favicon",
expect.anything());
+ expect(message.success).toHaveBeenCalledWith("Branding saved
successfully.");
+ });
+
+ it("saveLogos does nothing when no branding asset is set", () => {
+ component.logoData = null;
+ component.miniLogoData = null;
+ component.faviconData = null;
+
+ component.saveLogos();
+
+ expect(adminSettings.updateSetting).not.toHaveBeenCalled();
+ expect(message.success).not.toHaveBeenCalled();
+ });
+
+ it("saveLogos notifies an error when the request fails", () => {
+ component.logoData = "data:image/png;base64,LOGO";
+ adminSettings.updateSetting.mockReturnValue(throwError(() => new
Error("nope")));
+
+ component.saveLogos();
+
+ expect(message.error).toHaveBeenCalledWith("Failed to save branding.");
+ });
+
+ it("resetBranding resets all three branding settings", () => {
+ component.resetBranding();
+
+ expect(adminSettings.resetSetting).toHaveBeenCalledWith("logo");
+ expect(adminSettings.resetSetting).toHaveBeenCalledWith("mini_logo");
+ expect(adminSettings.resetSetting).toHaveBeenCalledWith("favicon");
+ expect(message.info).toHaveBeenCalledWith("Resetting branding...");
+ });
+ });
+
+ describe("tabs", () => {
+ it("saveTabs persists every sidebar tab and notifies success", () => {
+ const tabCount = Object.keys(component.sidebarTabs).length;
+
+ component.saveTabs();
+
+ expect(adminSettings.updateSetting).toHaveBeenCalledTimes(tabCount);
+ expect(adminSettings.updateSetting).toHaveBeenCalledWith("hub_enabled",
"false");
+ expect(message.success).toHaveBeenCalledWith("Tabs saved successfully.");
+ });
+
+ it("resetTabs resets every sidebar tab", () => {
+ const tabCount = Object.keys(component.sidebarTabs).length;
+
+ component.resetTabs();
+
+ expect(adminSettings.resetSetting).toHaveBeenCalledTimes(tabCount);
+ expect(message.info).toHaveBeenCalledWith("Resetting tabs...");
+ });
+ });
+
+ describe("dataset settings", () => {
+ beforeEach(() => {
+ component.maxConcurrentFiles = 3;
+ component.maxFileSizeMiB = 20;
+ component.maxConcurrentChunks = 10;
+ component.chunkSizeMiB = 50;
+ });
+
+ it("saveDatasetSettings persists the four upload settings and notifies
success", () => {
+ component.saveDatasetSettings();
+
+
expect(adminSettings.updateSetting).toHaveBeenCalledWith("max_number_of_concurrent_uploading_file",
"3");
+
expect(adminSettings.updateSetting).toHaveBeenCalledWith("single_file_upload_max_size_mib",
"20");
+
expect(adminSettings.updateSetting).toHaveBeenCalledWith("max_number_of_concurrent_uploading_file_chunks",
"10");
+
expect(adminSettings.updateSetting).toHaveBeenCalledWith("multipart_upload_chunk_size_mib",
"50");
+ expect(message.success).toHaveBeenCalledWith("Dataset upload settings
saved successfully.");
+ });
+
+ it("saveDatasetSettings rejects non-positive values without saving", () =>
{
+ component.maxFileSizeMiB = 0;
+
+ component.saveDatasetSettings();
+
+ expect(adminSettings.updateSetting).not.toHaveBeenCalled();
+ expect(message.error).toHaveBeenCalledWith("Please enter valid integer
values.");
+ });
+
+ it("saveDatasetSettings rejects a configuration that would exceed the
10,000-part limit", () => {
+ component.maxFileSizeMiB = 100000;
+ component.chunkSizeMiB = 1;
+
+ component.saveDatasetSettings();
+
+ expect(adminSettings.updateSetting).not.toHaveBeenCalled();
+ expect(message.error).toHaveBeenCalled();
+ });
+
+ it("resetDatasetSettings resets all four upload settings", () => {
+ component.resetDatasetSettings();
+
+ expect(adminSettings.resetSetting).toHaveBeenCalledTimes(4);
+
expect(adminSettings.resetSetting).toHaveBeenCalledWith("multipart_upload_chunk_size_mib");
+ expect(message.info).toHaveBeenCalledWith("Resetting dataset
settings...");
+ });
+ });
+
+ describe("csv (result panel) settings", () => {
+ it("saveCsvSettings persists the max-columns value and notifies success",
() => {
+ component.csvMaxColumns = 256;
+
+ component.saveCsvSettings();
+
+
expect(adminSettings.updateSetting).toHaveBeenCalledWith("csv_parser_max_columns",
"256");
+ expect(notification.success).toHaveBeenCalledWith("Result panel settings
saved.");
+ });
+
+ it("saveCsvSettings notifies an error when the request fails", () => {
+ adminSettings.updateSetting.mockReturnValue(throwError(() => new
Error("nope")));
+
+ component.saveCsvSettings();
+
+ expect(notification.error).toHaveBeenCalledWith("Could not save result
panel settings.");
+ });
+
+ it("resetCsvSettings resets the max-columns setting", () => {
+ component.resetCsvSettings();
+
+
expect(adminSettings.resetSetting).toHaveBeenCalledWith("csv_parser_max_columns");
+ expect(notification.info).toHaveBeenCalledWith("Resetting result panel
settings...");
+ });
+ });
+
+ describe("onFileChange", () => {
+ it("rejects a non-image file with an error message", () => {
+ const event = {
+ target: { files: [new File(["x"], "notes.txt", { type: "text/plain"
})] },
+ } as unknown as Event;
+
+ component.onFileChange("logo", event);
+
+ expect(message.error).toHaveBeenCalledWith("Please upload a valid image
file.");
+ expect(component.logoData).toBeNull();
+ });
Review Comment:
The non-image `onFileChange` test asserts `logoData` is null, which is
already true in this test setup (initial value + `getSetting` mocked as empty
string). Setting `logoData` to a non-null sentinel first makes the test
actually verify that invalid files do not overwrite an existing logo.
--
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]
