Conversation
Migration Safety CheckFound 1 potential issue: 20260325_173659_add_dev_mode.ts Warning (line 6): ALTER keyword detected - review for data loss sql`ALTER TABLE \`nac_widgets_config\` ADD \`dev_mode\` integer DEFAULT false NOT NULL;`,Review these patterns and add backup/restore logic if needed. See |
|
Preview deployment: https://nac-dev.preview.avy-fx.org |
|
@busbyk |
|
@busbyk Ready for review |
| jest.mock('../../src/services/nac/nac', () => ({ | ||
| getAvalancheCenterPlatforms: jest.fn(async (centerSlug: string) => { | ||
| const centerSlugToUse = centerSlug === 'dvac' ? 'nwac' : centerSlug | ||
|
|
||
| return mockCenters[centerSlugToUse] ?? allFalsePlatforms | ||
| }), | ||
| })) |
There was a problem hiding this comment.
Replacing the function we're testing here with a mocked function that's only used in this test doesn't actually test the getAvalancheCenterPlatforms function.
Ideally, we would intercept the API requests and return mocked data using MSW or something similar so that we would be testing the actual implementation of getAvalancheCenterPlatforms.
If you don't want to set up MSW, I suggest removing this test file completely.
There was a problem hiding this comment.
Good callout - set up MSW you linked and implemented this in 251372b
| it('forces devMode to false in production even when global config has it enabled', async () => { | ||
| Object.defineProperty(process.env, 'NODE_ENV', { value: 'production', configurable: true }) | ||
| mockConfig.devMode = true | ||
|
|
||
| const result = await getNACWidgetsConfig() | ||
| expect(result.devMode).toBe(false) | ||
| }) |
There was a problem hiding this comment.
The PR description says that nacWidgetsConfig.devMode should be respected when NAC_WIDGET_DEV_MODE is not set.
There was a problem hiding this comment.
Ah good shout - this was implemented incorrectly and overengineered. I somehow got confused. The priority is:
- if prod -> always
false nacWidgetsConfig.devModefrom admin panel (if env var not set)false(if neither is set)
src/utilities/getNACWidgetsConfig.ts
Outdated
| // In production, always force devMode off regardless of the admin setting | ||
| const devMode = | ||
| process.env.NODE_ENV === 'production' ? false : (nacWidgetsConfig?.devMode ?? false) |
There was a problem hiding this comment.
Yea I don't see why we wouldn't respect this in production.
There was a problem hiding this comment.
The intention was we would never want this to be accidentally true in production, but I guess we might want that? I can't think of a use case.
There was a problem hiding this comment.
from slack convo: we will check our environment friendly name via https://github.com/NWACus/web/blob/main/src/utilities/getEnvironmentFriendlyName.ts and disable in prod but allow in other environments like dev and preview.
2 participants
Description
Add a
devModesetting to the NAC Widgets Config admin panel and pass it through to all NAC widget components.Previously,
devModewas always hardcoded tofalse. This makes it impossible for staging/preview deployments to point widgets at the NAC staging base URL — needed for AvyApp development and testing custom data entered in staging. In production,devModeis always forced tofalseregardless of the admin panel setting.Related Issues
Fixes #717
Key Changes
devModecheckbox to NAC Widgets Config globaldevModesetting in non-production environments, forcefalsein productiondevModethrough to all NAC widget components (was hardcodedfalse)getNACWidgetsConfigandgetAvalancheCenterPlatformsusing MSW to intercept HTTP requestsdevModecolumnHow to test
2. with admin panel devMode off→ widgets use production URL (
devMode: false)3. with admin panel devMode on → widgets use staging URL (
devMode: true)devMode: false) regardless of admin setting/api/[center]/nac-configreturns the correctdevModevaluepnpm test— all tests passMigration Explanation
Adds a
devModeboolean column (defaultfalse) to the NAC widgets config table. Non-destructive — existing rows get the default value.