mirror of
https://github.com/Stirling-Tools/Stirling-PDF.git
synced 2026-09-03 05:10:16 +03:00
fix update notification visibility and install flicker (#6776)
# Description of Changes - Closes #6754 - Update popup now hidden on mobile, for non-admins, and never on SaaS - Respects admin "Show Update Notifications" setting (`showUpdate` / `showUpdateOnlyAdmin`, now default on) - Fixes update modal flickering during desktop install --- ## Checklist ### General - [ ] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [ ] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md) (if applicable) - [ ] I have performed a self-review of my own code - [ ] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) (if functionality has heavily changed) - [ ] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md#add-new-translation-tags) (for new translation tags only) ### Translations (if applicable) - [ ] I ran [`scripts/counter_translation.py`](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/docs/counter_translation.md) ### UI Changes (if applicable) - [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR) ### Testing (if applicable) - [ ] I have run `task check` to verify linters, typechecks, and tests pass - [ ] I have tested my changes locally. Refer to the [Testing Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md#7-testing) for more details.
This commit is contained in:
@@ -869,8 +869,8 @@ public class ApplicationProperties {
|
||||
public static class System {
|
||||
private String defaultLocale;
|
||||
private boolean googlevisibility;
|
||||
private boolean showUpdate;
|
||||
private boolean showUpdateOnlyAdmin;
|
||||
private boolean showUpdate = true;
|
||||
private boolean showUpdateOnlyAdmin = true;
|
||||
private boolean showSettingsWhenNoLogin = true;
|
||||
private boolean customHTMLFiles;
|
||||
private String tessdataDir;
|
||||
|
||||
+9
@@ -22,6 +22,7 @@ import stirling.software.SPDF.config.InitialSetup;
|
||||
import stirling.software.SPDF.controller.api.security.TimestampController;
|
||||
import stirling.software.common.annotations.api.ConfigApi;
|
||||
import stirling.software.common.configuration.AppConfig;
|
||||
import stirling.software.common.configuration.interfaces.ShowAdminInterface;
|
||||
import stirling.software.common.model.ApplicationProperties;
|
||||
import stirling.software.common.service.ServerCertificateServiceInterface;
|
||||
import stirling.software.common.service.UserServiceInterface;
|
||||
@@ -37,6 +38,7 @@ public class ConfigController {
|
||||
private final EndpointConfiguration endpointConfiguration;
|
||||
private final ServerCertificateServiceInterface serverCertificateService;
|
||||
private final UserServiceInterface userService;
|
||||
private final ShowAdminInterface showAdmin;
|
||||
private final stirling.software.common.service.LicenseServiceInterface licenseService;
|
||||
private final stirling.software.SPDF.config.ExternalAppDepConfig externalAppDepConfig;
|
||||
|
||||
@@ -48,6 +50,8 @@ public class ConfigController {
|
||||
ServerCertificateServiceInterface serverCertificateService,
|
||||
@org.springframework.beans.factory.annotation.Autowired(required = false)
|
||||
UserServiceInterface userService,
|
||||
@org.springframework.beans.factory.annotation.Autowired(required = false)
|
||||
ShowAdminInterface showAdmin,
|
||||
@org.springframework.beans.factory.annotation.Autowired(required = false)
|
||||
stirling.software.common.service.LicenseServiceInterface licenseService,
|
||||
stirling.software.SPDF.config.ExternalAppDepConfig externalAppDepConfig) {
|
||||
@@ -56,6 +60,7 @@ public class ConfigController {
|
||||
this.endpointConfiguration = endpointConfiguration;
|
||||
this.serverCertificateService = serverCertificateService;
|
||||
this.userService = userService;
|
||||
this.showAdmin = showAdmin;
|
||||
this.licenseService = licenseService;
|
||||
this.externalAppDepConfig = externalAppDepConfig;
|
||||
}
|
||||
@@ -315,6 +320,10 @@ public class ConfigController {
|
||||
configData.put(
|
||||
"enableAlphaFunctionality",
|
||||
applicationProperties.getSystem().isEnableAlphaFunctionality());
|
||||
boolean shouldShowUpdate =
|
||||
applicationProperties.getSystem().isShowUpdate()
|
||||
&& (showAdmin == null || showAdmin.getShowUpdateOnlyAdmins());
|
||||
configData.put("shouldShowUpdate", shouldShowUpdate);
|
||||
configData.put(
|
||||
"enableAnalytics", applicationProperties.getSystem().getEnableAnalytics());
|
||||
configData.put("enablePosthog", applicationProperties.getSystem().getEnablePosthog());
|
||||
|
||||
@@ -170,8 +170,8 @@ system:
|
||||
defaultLocale: "" # force a default language for new users (e.g. 'en-US', 'de-DE'). Empty string auto-detects from the browser, falling back to en-US
|
||||
googlevisibility: false # 'true' to allow Google visibility (via robots.txt), 'false' to disallow
|
||||
enableAlphaFunctionality: false # set to enable functionality which might need more testing before it fully goes live (this feature might make no changes)
|
||||
showUpdate: false # see when a new update is available
|
||||
showUpdateOnlyAdmin: false # only admins can see when a new update is available, depending on showUpdate it must be set to 'true'
|
||||
showUpdate: true # see when a new update is available
|
||||
showUpdateOnlyAdmin: true # only admins can see when a new update is available, depending on showUpdate it must be set to 'true'
|
||||
showSettingsWhenNoLogin: true # set to 'false' to hide settings button when login is disabled (enableLogin: false). Only applies when login is disabled.
|
||||
customHTMLFiles: false # enable to have files placed in /customFiles/templates override the existing template HTML files
|
||||
tessdataDir: "" # path to the directory containing the Tessdata files. This setting is relevant for Windows systems. For Windows users, this path should be adjusted to point to the appropriate directory where the Tessdata files are stored.
|
||||
|
||||
+3
@@ -26,6 +26,7 @@ import jakarta.servlet.http.HttpServletRequest;
|
||||
import stirling.software.SPDF.config.EndpointConfiguration;
|
||||
import stirling.software.SPDF.config.ExternalAppDepConfig;
|
||||
import stirling.software.common.configuration.AppConfig;
|
||||
import stirling.software.common.configuration.interfaces.ShowAdminInterface;
|
||||
import stirling.software.common.model.ApplicationProperties;
|
||||
import stirling.software.common.service.LicenseServiceInterface;
|
||||
import stirling.software.common.service.ServerCertificateServiceInterface;
|
||||
@@ -45,6 +46,7 @@ class ConfigControllerMoreTest {
|
||||
@Mock private EndpointConfiguration endpointConfiguration;
|
||||
@Mock private ServerCertificateServiceInterface serverCertificateService;
|
||||
@Mock private UserServiceInterface userService;
|
||||
@Mock private ShowAdminInterface showAdmin;
|
||||
@Mock private LicenseServiceInterface licenseService;
|
||||
@Mock private ExternalAppDepConfig externalAppDepConfig;
|
||||
@Mock private AppConfig appConfig;
|
||||
@@ -70,6 +72,7 @@ class ConfigControllerMoreTest {
|
||||
endpointConfiguration,
|
||||
serverCertificateService,
|
||||
userService,
|
||||
showAdmin,
|
||||
licenseService,
|
||||
externalAppDepConfig);
|
||||
}
|
||||
|
||||
+3
@@ -21,6 +21,7 @@ import stirling.software.SPDF.config.EndpointConfiguration;
|
||||
import stirling.software.SPDF.config.EndpointConfiguration.DisableReason;
|
||||
import stirling.software.SPDF.config.EndpointConfiguration.EndpointAvailability;
|
||||
import stirling.software.common.configuration.AppConfig;
|
||||
import stirling.software.common.configuration.interfaces.ShowAdminInterface;
|
||||
import stirling.software.common.model.ApplicationProperties;
|
||||
import stirling.software.common.model.ApplicationProperties.System;
|
||||
import stirling.software.common.service.LicenseServiceInterface;
|
||||
@@ -35,6 +36,7 @@ class ConfigControllerTest {
|
||||
@Mock private EndpointConfiguration endpointConfiguration;
|
||||
@Mock private ServerCertificateServiceInterface serverCertificateService;
|
||||
@Mock private UserServiceInterface userService;
|
||||
@Mock private ShowAdminInterface showAdmin;
|
||||
@Mock private LicenseServiceInterface licenseService;
|
||||
|
||||
private ConfigController configController;
|
||||
@@ -48,6 +50,7 @@ class ConfigControllerTest {
|
||||
endpointConfiguration,
|
||||
serverCertificateService,
|
||||
userService,
|
||||
showAdmin,
|
||||
licenseService,
|
||||
mock(stirling.software.SPDF.config.ExternalAppDepConfig.class));
|
||||
}
|
||||
|
||||
+93
@@ -0,0 +1,93 @@
|
||||
package stirling.software.proprietary.security.service;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertFalse;
|
||||
import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||
import static org.mockito.Mockito.mock;
|
||||
import static org.mockito.Mockito.mockStatic;
|
||||
import static org.mockito.Mockito.when;
|
||||
|
||||
import java.util.Optional;
|
||||
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.mockito.MockedStatic;
|
||||
import org.springframework.security.core.Authentication;
|
||||
import org.springframework.security.core.context.SecurityContext;
|
||||
import org.springframework.security.core.context.SecurityContextHolder;
|
||||
|
||||
import stirling.software.common.model.ApplicationProperties;
|
||||
import stirling.software.proprietary.security.database.repository.UserRepository;
|
||||
import stirling.software.proprietary.security.model.User;
|
||||
|
||||
class AppUpdateAuthServiceTest {
|
||||
|
||||
private final UserRepository userRepository = mock(UserRepository.class);
|
||||
|
||||
private AppUpdateAuthService service(boolean showUpdate, boolean onlyAdmin) {
|
||||
ApplicationProperties props = new ApplicationProperties();
|
||||
ApplicationProperties.System system = new ApplicationProperties.System();
|
||||
system.setShowUpdate(showUpdate);
|
||||
system.setShowUpdateOnlyAdmin(onlyAdmin);
|
||||
props.setSystem(system);
|
||||
return new AppUpdateAuthService(userRepository, props);
|
||||
}
|
||||
|
||||
private void withAuthentication(Authentication auth, Runnable assertions) {
|
||||
try (MockedStatic<SecurityContextHolder> holder = mockStatic(SecurityContextHolder.class)) {
|
||||
SecurityContext context = mock(SecurityContext.class);
|
||||
when(context.getAuthentication()).thenReturn(auth);
|
||||
holder.when(SecurityContextHolder::getContext).thenReturn(context);
|
||||
assertions.run();
|
||||
}
|
||||
}
|
||||
|
||||
private Authentication authenticatedAs(String username, String roles) {
|
||||
Authentication auth = mock(Authentication.class);
|
||||
when(auth.isAuthenticated()).thenReturn(true);
|
||||
when(auth.getName()).thenReturn(username);
|
||||
User user = mock(User.class);
|
||||
when(user.getRolesAsString()).thenReturn(roles);
|
||||
when(userRepository.findByUsername(username)).thenReturn(Optional.of(user));
|
||||
return auth;
|
||||
}
|
||||
|
||||
@Test
|
||||
void hidesForEveryoneWhenShowUpdateOff() {
|
||||
withAuthentication(
|
||||
authenticatedAs("admin", "ROLE_ADMIN"),
|
||||
() -> assertFalse(service(false, true).getShowUpdateOnlyAdmins()));
|
||||
}
|
||||
|
||||
@Test
|
||||
void showsToAllAuthenticatedWhenNotAdminOnly() {
|
||||
withAuthentication(
|
||||
authenticatedAs("bob", "ROLE_USER"),
|
||||
() -> assertTrue(service(true, false).getShowUpdateOnlyAdmins()));
|
||||
}
|
||||
|
||||
@Test
|
||||
void hidesFromAnonymousWhenAdminOnly() {
|
||||
withAuthentication(null, () -> assertFalse(service(true, true).getShowUpdateOnlyAdmins()));
|
||||
}
|
||||
|
||||
@Test
|
||||
void hidesFromAnonymousUserPrincipalWhenAdminOnly() {
|
||||
Authentication auth = mock(Authentication.class);
|
||||
when(auth.isAuthenticated()).thenReturn(true);
|
||||
when(auth.getName()).thenReturn("anonymousUser");
|
||||
withAuthentication(auth, () -> assertFalse(service(true, true).getShowUpdateOnlyAdmins()));
|
||||
}
|
||||
|
||||
@Test
|
||||
void showsToAdminWhenAdminOnly() {
|
||||
withAuthentication(
|
||||
authenticatedAs("admin", "ROLE_ADMIN"),
|
||||
() -> assertTrue(service(true, true).getShowUpdateOnlyAdmins()));
|
||||
}
|
||||
|
||||
@Test
|
||||
void hidesFromNonAdminWhenAdminOnly() {
|
||||
withAuthentication(
|
||||
authenticatedAs("bob", "ROLE_USER"),
|
||||
() -> assertFalse(service(true, true).getShowUpdateOnlyAdmins()));
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,66 @@
|
||||
import { render, screen, waitFor } from "@testing-library/react";
|
||||
import { MantineProvider } from "@mantine/core";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import type { MachineInfo, UpdateSummary } from "@app/services/updateService";
|
||||
|
||||
const getFullUpdateInfo = vi.fn();
|
||||
|
||||
vi.mock("@app/services/updateService", () => ({
|
||||
updateService: {
|
||||
getFullUpdateInfo: (...args: unknown[]) => getFullUpdateInfo(...args),
|
||||
getDownloadUrl: () => null,
|
||||
compareVersions: (a: string, b: string) => (a === b ? 0 : a > b ? 1 : -1),
|
||||
},
|
||||
}));
|
||||
|
||||
import UpdateModal from "@app/components/shared/UpdateModal";
|
||||
|
||||
const summary: UpdateSummary = {
|
||||
latest_version: "2.0.0",
|
||||
max_priority: "normal",
|
||||
any_breaking: false,
|
||||
};
|
||||
|
||||
const machine = (): MachineInfo => ({
|
||||
machineType: "Client-win",
|
||||
activeSecurity: false,
|
||||
licenseType: "NORMAL",
|
||||
});
|
||||
|
||||
function wrap(machineInfo: MachineInfo) {
|
||||
return (
|
||||
<MantineProvider>
|
||||
<UpdateModal
|
||||
opened
|
||||
onClose={() => {}}
|
||||
currentVersion="1.0.0"
|
||||
updateSummary={summary}
|
||||
machineInfo={machineInfo}
|
||||
/>
|
||||
</MantineProvider>
|
||||
);
|
||||
}
|
||||
|
||||
describe("UpdateModal", () => {
|
||||
afterEach(() => getFullUpdateInfo.mockReset());
|
||||
|
||||
it("does not refetch version info when machineInfo identity changes but values are equal", async () => {
|
||||
getFullUpdateInfo.mockResolvedValue({
|
||||
latest_version: "2.0.0",
|
||||
new_versions: [],
|
||||
});
|
||||
|
||||
const { rerender } = render(wrap(machine()));
|
||||
|
||||
await waitFor(() =>
|
||||
expect(
|
||||
screen.queryByText("update.loadingDetailedInfo"),
|
||||
).not.toBeInTheDocument(),
|
||||
);
|
||||
expect(getFullUpdateInfo).toHaveBeenCalledTimes(1);
|
||||
|
||||
rerender(wrap(machine()));
|
||||
|
||||
await waitFor(() => expect(getFullUpdateInfo).toHaveBeenCalledTimes(1));
|
||||
});
|
||||
});
|
||||
@@ -130,18 +130,27 @@ const UpdateModal: React.FC<UpdateModalProps> = ({
|
||||
new Set([0]),
|
||||
);
|
||||
|
||||
const { machineType, activeSecurity, licenseType } = machineInfo;
|
||||
useEffect(() => {
|
||||
if (opened) {
|
||||
setLoading(true);
|
||||
setExpandedVersions(new Set([0]));
|
||||
updateService
|
||||
.getFullUpdateInfo(currentVersion, machineInfo)
|
||||
.then((info) => {
|
||||
setFullUpdateInfo(info);
|
||||
setLoading(false);
|
||||
});
|
||||
}
|
||||
}, [opened, currentVersion, machineInfo]);
|
||||
if (!opened) return;
|
||||
let cancelled = false;
|
||||
setLoading(true);
|
||||
setExpandedVersions(new Set([0]));
|
||||
updateService
|
||||
.getFullUpdateInfo(currentVersion, {
|
||||
machineType,
|
||||
activeSecurity,
|
||||
licenseType,
|
||||
})
|
||||
.then((info) => {
|
||||
if (cancelled) return;
|
||||
setFullUpdateInfo(info);
|
||||
setLoading(false);
|
||||
});
|
||||
return () => {
|
||||
cancelled = true;
|
||||
};
|
||||
}, [opened, currentVersion, machineType, activeSecurity, licenseType]);
|
||||
|
||||
const toggleVersion = (index: number) => {
|
||||
setExpandedVersions((prev) => {
|
||||
|
||||
@@ -1,7 +1,9 @@
|
||||
import { useEffect, useRef, useState } from "react";
|
||||
import { useAppConfig } from "@app/contexts/AppConfigContext";
|
||||
import { useFrontendVersionInfo } from "@app/hooks/useFrontendVersionInfo";
|
||||
import { useIsMobile } from "@app/hooks/useIsMobile";
|
||||
import { updateService, type UpdateSummary } from "@app/services/updateService";
|
||||
import { isUpdatePopupAllowed } from "@app/components/shared/updatePopupGate";
|
||||
import UpdateModal from "@app/components/shared/UpdateModal";
|
||||
|
||||
/**
|
||||
@@ -26,6 +28,7 @@ const SNOOZE_DURATION_MS = 24 * 60 * 60 * 1000;
|
||||
*/
|
||||
export function UpdateStartupPopup() {
|
||||
const { config } = useAppConfig();
|
||||
const isMobile = useIsMobile();
|
||||
const { appVersion } = useFrontendVersionInfo(config?.appVersion);
|
||||
|
||||
// The version to compare against the latest. Prefer the frontend version
|
||||
@@ -39,10 +42,12 @@ export function UpdateStartupPopup() {
|
||||
const [showModal, setShowModal] = useState(false);
|
||||
const hasChecked = useRef(false);
|
||||
|
||||
const allowed = isUpdatePopupAllowed(config, isMobile);
|
||||
|
||||
useEffect(() => {
|
||||
if (hasChecked.current) return;
|
||||
if (!currentVersion) return;
|
||||
// Don't even schedule the timer until we have a version to compare.
|
||||
if (!allowed) return;
|
||||
hasChecked.current = true;
|
||||
|
||||
const timer = setTimeout(async () => {
|
||||
@@ -79,12 +84,14 @@ export function UpdateStartupPopup() {
|
||||
|
||||
return () => clearTimeout(timer);
|
||||
}, [
|
||||
allowed,
|
||||
currentVersion,
|
||||
config?.machineType,
|
||||
config?.activeSecurity,
|
||||
config?.license,
|
||||
]);
|
||||
|
||||
if (!allowed) return null;
|
||||
if (!updateSummary || !currentVersion) return null;
|
||||
|
||||
const machineInfo = {
|
||||
|
||||
@@ -0,0 +1,33 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import type { AppConfig } from "@app/types/appConfig";
|
||||
import { isUpdatePopupAllowed } from "@app/components/shared/updatePopupGate";
|
||||
|
||||
describe("isUpdatePopupAllowed", () => {
|
||||
it("returns false until config has loaded", () => {
|
||||
expect(isUpdatePopupAllowed(null, false)).toBe(false);
|
||||
});
|
||||
|
||||
it("shows on desktop when the backend allows it", () => {
|
||||
expect(
|
||||
isUpdatePopupAllowed({ shouldShowUpdate: true } as AppConfig, false),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("never shows on a mobile / narrow viewport", () => {
|
||||
expect(
|
||||
isUpdatePopupAllowed({ shouldShowUpdate: true } as AppConfig, true),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("hides when the backend says not to (non-admin / showUpdate disabled)", () => {
|
||||
expect(
|
||||
isUpdatePopupAllowed({ shouldShowUpdate: false } as AppConfig, false),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("fails closed when the backend omits the decision (e.g. 401 fallback)", () => {
|
||||
expect(
|
||||
isUpdatePopupAllowed({ enableLogin: true } as AppConfig, false),
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,10 @@
|
||||
import type { AppConfig } from "@app/types/appConfig";
|
||||
|
||||
export function isUpdatePopupAllowed(
|
||||
config: AppConfig | null,
|
||||
isMobile: boolean,
|
||||
): boolean {
|
||||
if (!config) return false;
|
||||
if (isMobile) return false;
|
||||
return config.shouldShowUpdate === true;
|
||||
}
|
||||
@@ -13,6 +13,7 @@ export interface AppConfig {
|
||||
enableOAuth?: boolean;
|
||||
enableSaml?: boolean;
|
||||
isAdmin?: boolean;
|
||||
shouldShowUpdate?: boolean;
|
||||
enableAlphaFunctionality?: boolean;
|
||||
enableAnalytics?: boolean | null;
|
||||
enablePosthog?: boolean | null;
|
||||
|
||||
@@ -0,0 +1,5 @@
|
||||
export function UpdateStartupPopup() {
|
||||
return null;
|
||||
}
|
||||
|
||||
export default UpdateStartupPopup;
|
||||
Reference in New Issue
Block a user