Skip to content

Commit f7f7b87

Browse files
FrooodleFrooodle
authored
fix update notification visibility and install flicker (Stirling-Tools#6776)
# Description of Changes - Closes Stirling-Tools#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.
1 parent 2602142 commit f7f7b87

13 files changed

Lines changed: 255 additions & 16 deletions

File tree

‎app/common/src/main/java/stirling/software/common/model/ApplicationProperties.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -869,8 +869,8 @@ public static class Timestamp {
869869
public static class System {
870870
private String defaultLocale;
871871
private boolean googlevisibility;
872-
private boolean showUpdate;
873-
private boolean showUpdateOnlyAdmin;
872+
private boolean showUpdate = true;
873+
private boolean showUpdateOnlyAdmin = true;
874874
private boolean showSettingsWhenNoLogin = true;
875875
private boolean customHTMLFiles;
876876
private String tessdataDir;

‎app/core/src/main/java/stirling/software/SPDF/controller/api/misc/ConfigController.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import stirling.software.SPDF.controller.api.security.TimestampController;
2323
import stirling.software.common.annotations.api.ConfigApi;
2424
import stirling.software.common.configuration.AppConfig;
25+
import stirling.software.common.configuration.interfaces.ShowAdminInterface;
2526
import stirling.software.common.model.ApplicationProperties;
2627
import stirling.software.common.service.ServerCertificateServiceInterface;
2728
import stirling.software.common.service.UserServiceInterface;
@@ -37,6 +38,7 @@ public class ConfigController {
3738
private final EndpointConfiguration endpointConfiguration;
3839
private final ServerCertificateServiceInterface serverCertificateService;
3940
private final UserServiceInterface userService;
41+
private final ShowAdminInterface showAdmin;
4042
private final stirling.software.common.service.LicenseServiceInterface licenseService;
4143
private final stirling.software.SPDF.config.ExternalAppDepConfig externalAppDepConfig;
4244

@@ -48,6 +50,8 @@ public ConfigController(
4850
ServerCertificateServiceInterface serverCertificateService,
4951
@org.springframework.beans.factory.annotation.Autowired(required = false)
5052
UserServiceInterface userService,
53+
@org.springframework.beans.factory.annotation.Autowired(required = false)
54+
ShowAdminInterface showAdmin,
5155
@org.springframework.beans.factory.annotation.Autowired(required = false)
5256
stirling.software.common.service.LicenseServiceInterface licenseService,
5357
stirling.software.SPDF.config.ExternalAppDepConfig externalAppDepConfig) {
@@ -56,6 +60,7 @@ public ConfigController(
5660
this.endpointConfiguration = endpointConfiguration;
5761
this.serverCertificateService = serverCertificateService;
5862
this.userService = userService;
63+
this.showAdmin = showAdmin;
5964
this.licenseService = licenseService;
6065
this.externalAppDepConfig = externalAppDepConfig;
6166
}
@@ -315,6 +320,10 @@ public ResponseEntity<Map<String, Object>> getAppConfig(HttpServletRequest reque
315320
configData.put(
316321
"enableAlphaFunctionality",
317322
applicationProperties.getSystem().isEnableAlphaFunctionality());
323+
boolean shouldShowUpdate =
324+
applicationProperties.getSystem().isShowUpdate()
325+
&& (showAdmin == null || showAdmin.getShowUpdateOnlyAdmins());
326+
configData.put("shouldShowUpdate", shouldShowUpdate);
318327
configData.put(
319328
"enableAnalytics", applicationProperties.getSystem().getEnableAnalytics());
320329
configData.put("enablePosthog", applicationProperties.getSystem().getEnablePosthog());

‎app/core/src/main/resources/settings.yml.template‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,8 +170,8 @@ system:
170170
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
171171
googlevisibility: false # 'true' to allow Google visibility (via robots.txt), 'false' to disallow
172172
enableAlphaFunctionality: false # set to enable functionality which might need more testing before it fully goes live (this feature might make no changes)
173-
showUpdate: false # see when a new update is available
174-
showUpdateOnlyAdmin: false # only admins can see when a new update is available, depending on showUpdate it must be set to 'true'
173+
showUpdate: true # see when a new update is available
174+
showUpdateOnlyAdmin: true # only admins can see when a new update is available, depending on showUpdate it must be set to 'true'
175175
showSettingsWhenNoLogin: true # set to 'false' to hide settings button when login is disabled (enableLogin: false). Only applies when login is disabled.
176176
customHTMLFiles: false # enable to have files placed in /customFiles/templates override the existing template HTML files
177177
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.

‎app/core/src/test/java/stirling/software/SPDF/controller/api/misc/ConfigControllerMoreTest.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import stirling.software.SPDF.config.EndpointConfiguration;
2727
import stirling.software.SPDF.config.ExternalAppDepConfig;
2828
import stirling.software.common.configuration.AppConfig;
29+
import stirling.software.common.configuration.interfaces.ShowAdminInterface;
2930
import stirling.software.common.model.ApplicationProperties;
3031
import stirling.software.common.service.LicenseServiceInterface;
3132
import stirling.software.common.service.ServerCertificateServiceInterface;
@@ -45,6 +46,7 @@ class ConfigControllerMoreTest {
4546
@Mock private EndpointConfiguration endpointConfiguration;
4647
@Mock private ServerCertificateServiceInterface serverCertificateService;
4748
@Mock private UserServiceInterface userService;
49+
@Mock private ShowAdminInterface showAdmin;
4850
@Mock private LicenseServiceInterface licenseService;
4951
@Mock private ExternalAppDepConfig externalAppDepConfig;
5052
@Mock private AppConfig appConfig;
@@ -70,6 +72,7 @@ private ConfigController newController() {
7072
endpointConfiguration,
7173
serverCertificateService,
7274
userService,
75+
showAdmin,
7376
licenseService,
7477
externalAppDepConfig);
7578
}

‎app/core/src/test/java/stirling/software/SPDF/controller/api/misc/ConfigControllerTest.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import stirling.software.SPDF.config.EndpointConfiguration.DisableReason;
2222
import stirling.software.SPDF.config.EndpointConfiguration.EndpointAvailability;
2323
import stirling.software.common.configuration.AppConfig;
24+
import stirling.software.common.configuration.interfaces.ShowAdminInterface;
2425
import stirling.software.common.model.ApplicationProperties;
2526
import stirling.software.common.model.ApplicationProperties.System;
2627
import stirling.software.common.service.LicenseServiceInterface;
@@ -35,6 +36,7 @@ class ConfigControllerTest {
3536
@Mock private EndpointConfiguration endpointConfiguration;
3637
@Mock private ServerCertificateServiceInterface serverCertificateService;
3738
@Mock private UserServiceInterface userService;
39+
@Mock private ShowAdminInterface showAdmin;
3840
@Mock private LicenseServiceInterface licenseService;
3941

4042
private ConfigController configController;
@@ -48,6 +50,7 @@ void setUp() {
4850
endpointConfiguration,
4951
serverCertificateService,
5052
userService,
53+
showAdmin,
5154
licenseService,
5255
mock(stirling.software.SPDF.config.ExternalAppDepConfig.class));
5356
}

‎app/proprietary/src/test/java/stirling/software/proprietary/security/service/AppUpdateAuthServiceTest.java‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package stirling.software.proprietary.security.service;
2+
3+
import static org.junit.jupiter.api.Assertions.assertFalse;
4+
import static org.junit.jupiter.api.Assertions.assertTrue;
5+
import static org.mockito.Mockito.mock;
6+
import static org.mockito.Mockito.mockStatic;
7+
import static org.mockito.Mockito.when;
8+
9+
import java.util.Optional;
10+
11+
import org.junit.jupiter.api.Test;
12+
import org.mockito.MockedStatic;
13+
import org.springframework.security.core.Authentication;
14+
import org.springframework.security.core.context.SecurityContext;
15+
import org.springframework.security.core.context.SecurityContextHolder;
16+
17+
import stirling.software.common.model.ApplicationProperties;
18+
import stirling.software.proprietary.security.database.repository.UserRepository;
19+
import stirling.software.proprietary.security.model.User;
20+
21+
class AppUpdateAuthServiceTest {
22+
23+
private final UserRepository userRepository = mock(UserRepository.class);
24+
25+
private AppUpdateAuthService service(boolean showUpdate, boolean onlyAdmin) {
26+
ApplicationProperties props = new ApplicationProperties();
27+
ApplicationProperties.System system = new ApplicationProperties.System();
28+
system.setShowUpdate(showUpdate);
29+
system.setShowUpdateOnlyAdmin(onlyAdmin);
30+
props.setSystem(system);
31+
return new AppUpdateAuthService(userRepository, props);
32+
}
33+
34+
private void withAuthentication(Authentication auth, Runnable assertions) {
35+
try (MockedStatic<SecurityContextHolder> holder = mockStatic(SecurityContextHolder.class)) {
36+
SecurityContext context = mock(SecurityContext.class);
37+
when(context.getAuthentication()).thenReturn(auth);
38+
holder.when(SecurityContextHolder::getContext).thenReturn(context);
39+
assertions.run();
40+
}
41+
}
42+
43+
private Authentication authenticatedAs(String username, String roles) {
44+
Authentication auth = mock(Authentication.class);
45+
when(auth.isAuthenticated()).thenReturn(true);
46+
when(auth.getName()).thenReturn(username);
47+
User user = mock(User.class);
48+
when(user.getRolesAsString()).thenReturn(roles);
49+
when(userRepository.findByUsername(username)).thenReturn(Optional.of(user));
50+
return auth;
51+
}
52+
53+
@Test
54+
void hidesForEveryoneWhenShowUpdateOff() {
55+
withAuthentication(
56+
authenticatedAs("admin", "ROLE_ADMIN"),
57+
() -> assertFalse(service(false, true).getShowUpdateOnlyAdmins()));
58+
}
59+
60+
@Test
61+
void showsToAllAuthenticatedWhenNotAdminOnly() {
62+
withAuthentication(
63+
authenticatedAs("bob", "ROLE_USER"),
64+
() -> assertTrue(service(true, false).getShowUpdateOnlyAdmins()));
65+
}
66+
67+
@Test
68+
void hidesFromAnonymousWhenAdminOnly() {
69+
withAuthentication(null, () -> assertFalse(service(true, true).getShowUpdateOnlyAdmins()));
70+
}
71+
72+
@Test
73+
void hidesFromAnonymousUserPrincipalWhenAdminOnly() {
74+
Authentication auth = mock(Authentication.class);
75+
when(auth.isAuthenticated()).thenReturn(true);
76+
when(auth.getName()).thenReturn("anonymousUser");
77+
withAuthentication(auth, () -> assertFalse(service(true, true).getShowUpdateOnlyAdmins()));
78+
}
79+
80+
@Test
81+
void showsToAdminWhenAdminOnly() {
82+
withAuthentication(
83+
authenticatedAs("admin", "ROLE_ADMIN"),
84+
() -> assertTrue(service(true, true).getShowUpdateOnlyAdmins()));
85+
}
86+
87+
@Test
88+
void hidesFromNonAdminWhenAdminOnly() {
89+
withAuthentication(
90+
authenticatedAs("bob", "ROLE_USER"),
91+
() -> assertFalse(service(true, true).getShowUpdateOnlyAdmins()));
92+
}
93+
}

‎frontend/editor/src/core/components/shared/UpdateModal.test.tsx‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
import { render, screen, waitFor } from "@testing-library/react";
2+
import { MantineProvider } from "@mantine/core";
3+
import { afterEach, describe, expect, it, vi } from "vitest";
4+
import type { MachineInfo, UpdateSummary } from "@app/services/updateService";
5+
6+
const getFullUpdateInfo = vi.fn();
7+
8+
vi.mock("@app/services/updateService", () => ({
9+
updateService: {
10+
getFullUpdateInfo: (...args: unknown[]) => getFullUpdateInfo(...args),
11+
getDownloadUrl: () => null,
12+
compareVersions: (a: string, b: string) => (a === b ? 0 : a > b ? 1 : -1),
13+
},
14+
}));
15+
16+
import UpdateModal from "@app/components/shared/UpdateModal";
17+
18+
const summary: UpdateSummary = {
19+
latest_version: "2.0.0",
20+
max_priority: "normal",
21+
any_breaking: false,
22+
};
23+
24+
const machine = (): MachineInfo => ({
25+
machineType: "Client-win",
26+
activeSecurity: false,
27+
licenseType: "NORMAL",
28+
});
29+
30+
function wrap(machineInfo: MachineInfo) {
31+
return (
32+
<MantineProvider>
33+
<UpdateModal
34+
opened
35+
onClose={() => {}}
36+
currentVersion="1.0.0"
37+
updateSummary={summary}
38+
machineInfo={machineInfo}
39+
/>
40+
</MantineProvider>
41+
);
42+
}
43+
44+
describe("UpdateModal", () => {
45+
afterEach(() => getFullUpdateInfo.mockReset());
46+
47+
it("does not refetch version info when machineInfo identity changes but values are equal", async () => {
48+
getFullUpdateInfo.mockResolvedValue({
49+
latest_version: "2.0.0",
50+
new_versions: [],
51+
});
52+
53+
const { rerender } = render(wrap(machine()));
54+
55+
await waitFor(() =>
56+
expect(
57+
screen.queryByText("update.loadingDetailedInfo"),
58+
).not.toBeInTheDocument(),
59+
);
60+
expect(getFullUpdateInfo).toHaveBeenCalledTimes(1);
61+
62+
rerender(wrap(machine()));
63+
64+
await waitFor(() => expect(getFullUpdateInfo).toHaveBeenCalledTimes(1));
65+
});
66+
});

‎frontend/editor/src/core/components/shared/UpdateModal.tsx‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -130,18 +130,27 @@ const UpdateModal: React.FC<UpdateModalProps> = ({
130130
new Set([0]),
131131
);
132132

133+
const { machineType, activeSecurity, licenseType } = machineInfo;
133134
useEffect(() => {
134-
if (opened) {
135-
setLoading(true);
136-
setExpandedVersions(new Set([0]));
137-
updateService
138-
.getFullUpdateInfo(currentVersion, machineInfo)
139-
.then((info) => {
140-
setFullUpdateInfo(info);
141-
setLoading(false);
142-
});
143-
}
144-
}, [opened, currentVersion, machineInfo]);
135+
if (!opened) return;
136+
let cancelled = false;
137+
setLoading(true);
138+
setExpandedVersions(new Set([0]));
139+
updateService
140+
.getFullUpdateInfo(currentVersion, {
141+
machineType,
142+
activeSecurity,
143+
licenseType,
144+
})
145+
.then((info) => {
146+
if (cancelled) return;
147+
setFullUpdateInfo(info);
148+
setLoading(false);
149+
});
150+
return () => {
151+
cancelled = true;
152+
};
153+
}, [opened, currentVersion, machineType, activeSecurity, licenseType]);
145154

146155
const toggleVersion = (index: number) => {
147156
setExpandedVersions((prev) => {

‎frontend/editor/src/core/components/shared/UpdateStartupPopup.tsx‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import { useEffect, useRef, useState } from "react";
22
import { useAppConfig } from "@app/contexts/AppConfigContext";
33
import { useFrontendVersionInfo } from "@app/hooks/useFrontendVersionInfo";
4+
import { useIsMobile } from "@app/hooks/useIsMobile";
45
import { updateService, type UpdateSummary } from "@app/services/updateService";
6+
import { isUpdatePopupAllowed } from "@app/components/shared/updatePopupGate";
57
import UpdateModal from "@app/components/shared/UpdateModal";
68

79
/**
@@ -26,6 +28,7 @@ const SNOOZE_DURATION_MS = 24 * 60 * 60 * 1000;
2628
*/
2729
export function UpdateStartupPopup() {
2830
const { config } = useAppConfig();
31+
const isMobile = useIsMobile();
2932
const { appVersion } = useFrontendVersionInfo(config?.appVersion);
3033

3134
// The version to compare against the latest. Prefer the frontend version
@@ -39,10 +42,12 @@ export function UpdateStartupPopup() {
3942
const [showModal, setShowModal] = useState(false);
4043
const hasChecked = useRef(false);
4144

45+
const allowed = isUpdatePopupAllowed(config, isMobile);
46+
4247
useEffect(() => {
4348
if (hasChecked.current) return;
4449
if (!currentVersion) return;
45-
// Don't even schedule the timer until we have a version to compare.
50+
if (!allowed) return;
4651
hasChecked.current = true;
4752

4853
const timer = setTimeout(async () => {
@@ -79,12 +84,14 @@ export function UpdateStartupPopup() {
7984

8085
return () => clearTimeout(timer);
8186
}, [
87+
allowed,
8288
currentVersion,
8389
config?.machineType,
8490
config?.activeSecurity,
8591
config?.license,
8692
]);
8793

94+
if (!allowed) return null;
8895
if (!updateSummary || !currentVersion) return null;
8996

9097
const machineInfo = {

‎frontend/editor/src/core/components/shared/updatePopupGate.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import { describe, expect, it } from "vitest";
2+
import type { AppConfig } from "@app/types/appConfig";
3+
import { isUpdatePopupAllowed } from "@app/components/shared/updatePopupGate";
4+
5+
describe("isUpdatePopupAllowed", () => {
6+
it("returns false until config has loaded", () => {
7+
expect(isUpdatePopupAllowed(null, false)).toBe(false);
8+
});
9+
10+
it("shows on desktop when the backend allows it", () => {
11+
expect(
12+
isUpdatePopupAllowed({ shouldShowUpdate: true } as AppConfig, false),
13+
).toBe(true);
14+
});
15+
16+
it("never shows on a mobile / narrow viewport", () => {
17+
expect(
18+
isUpdatePopupAllowed({ shouldShowUpdate: true } as AppConfig, true),
19+
).toBe(false);
20+
});
21+
22+
it("hides when the backend says not to (non-admin / showUpdate disabled)", () => {
23+
expect(
24+
isUpdatePopupAllowed({ shouldShowUpdate: false } as AppConfig, false),
25+
).toBe(false);
26+
});
27+
28+
it("fails closed when the backend omits the decision (e.g. 401 fallback)", () => {
29+
expect(
30+
isUpdatePopupAllowed({ enableLogin: true } as AppConfig, false),
31+
).toBe(false);
32+
});
33+
});

0 commit comments

Comments
 (0)