mengw15 commented on code in PR #6527:
URL: https://github.com/apache/texera/pull/6527#discussion_r3609389889


##########
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:
   Added — `saveTabs` now has a test that forces `updateSetting` to error and 
asserts `message.error("Failed to save tabs.")`.



##########
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:
   Added — with valid inputs, a test forces `updateSetting` to error and 
asserts `message.error("Failed to save dataset settings.")`.



##########
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:
   Good catch — the test now seeds `logoData` with a sentinel first and asserts 
the invalid file leaves it untouched (fails if the guard is removed), instead 
of the tautological null check.



-- 
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]

Reply via email to