fix(signage-manager): fix brand kit saves and image editor flows

apps/signage-manager/USER_STORIES.md

@@ -381,7 +381,7 @@ These stories cover the current app workflows:
   - Without `template-editing`, template create, edit, duplicate, delete and layout changes are hidden. Template mappings stay available.
   - Without `ai-generation`, the AI create actions are hidden.
   - Without `ai-editing`, "Edit with AI" and refinement in the AI editor are hidden.
-  - Without `branding-editing`, the branding tab is read-only.
+  - Without `branding-editing`, the branding tab is read-only, and the AI editor cannot change the logo.
 - The All Groups view uses only `app.features`.
 
 ---

apps/signage-manager/src/app/branding/brand-access.ts

@@ -0,0 +1,22 @@
+import { SignageContextService } from '../signage-context.service';
+
+type BrandAccessContext = Pick<
+    SignageContextService,
+    'is_sys_admin' | 'global_features'
+>;
+
+/**
+ * Whether brand kit edits are on. The kit is for the whole organisation, so
+ * only the global `app.features` setting turns them off, not a group.
+ */
+export function brandEditingOn(context: BrandAccessContext): boolean {
+    return (context.global_features() || []).includes('branding-editing');
+}
+
+/**
+ * Whether this user can change the brand kit, from the branding page or from
+ * the image editor. Call it inside a `computed` so it follows the signals.
+ */
+export function canEditBrandKit(context: BrandAccessContext): boolean {
+    return context.is_sys_admin() && brandEditingOn(context);
+}

apps/signage-manager/src/app/branding/branding.component.ts

@@ -26,12 +26,22 @@ import {
     ImageGenBrandKit,
     ImageGenLogoSlot,
 } from '../image-gen/image-gen.types';
-import { errorMessage } from '../image-gen/image-gen.util';
+import { actionError } from '../image-gen/image-gen.util';
 import { SignageContextService } from '../signage-context.service';
+import { brandEditingOn, canEditBrandKit } from './brand-access';
 import { BRAND_FONTS, ensureBrandFont } from './brand-fonts';
 
 const COLOUR_NAMES = ['primary', 'secondary', 'accent'];
 
+/** how many palette colours the page shows and edits */
+const MAX_COLOURS = 3;
+
+/** a palette colour and the key it is stored under */
+interface BrandColour {
+    key: string;
+    value: string;
+}
+
 @Component({
     selector: 'app-branding',
     template: `
@@ -100,7 +110,7 @@ const COLOUR_NAMES = ['primary', 'secondary', 'accent'];
                                 class="border-base-content/20 h-10 w-14 rounded border bg-transparent disabled:cursor-not-allowed disabled:opacity-60"
                                 [class.cursor-pointer]="can_edit()"
                                 [disabled]="!can_edit()"
-                                [value]="colour"
+                                [value]="colour.value"
                                 (input)="setColourFromInput($index, $event)"
                                 [attr.aria-label]="
                                     'SIGNAGE_MANAGER.BRAND_COLOURS' | translate
@@ -113,7 +123,7 @@ const COLOUR_NAMES = ['primary', 'secondary', 'accent'];
                             >
                                 <input
                                     matInput
-                                    [ngModel]="colour"
+                                    [ngModel]="colour.value"
                                     (ngModelChange)="setColour($index, $event)"
                                     [disabled]="!can_edit()"
                                     [class.text-error]="colour_errors()[$index]"
@@ -125,7 +135,7 @@ const COLOUR_NAMES = ['primary', 'secondary', 'accent'];
                             </mat-form-field>
                             <span
                                 class="text-base-content/60 text-xs uppercase"
-                                >{{ colourName($index) }}</span
+                                >{{ colour.key }}</span
                             >
                             @if (can_edit()) {
                                 <button
@@ -145,7 +155,7 @@ const COLOUR_NAMES = ['primary', 'secondary', 'accent'];
                             }
                         </div>
                     }
-                    @if (can_edit() && colours().length < 3) {
+                    @if (can_edit() && colours().length < max_colours) {
                         <button
                             mat-stroked-button
                             type="button"
@@ -330,28 +340,23 @@ export class BrandingComponent implements OnInit {
     public readonly fonts = BRAND_FONTS;
     public readonly enabled = this._image_gen.enabled;
 
-    /** The brand kit is for the whole organisation, so only the global
-     * `app.features` setting turns branding edits off */
     public readonly branding_disabled = computed(
-        () =>
-            !(this._context.global_features() || []).includes(
-                'branding-editing',
-            ),
+        () => !brandEditingOn(this._context),
     );
     /** Whether the stored brand kit is read. The form shows only after a
      * read works, so its defaults cannot replace the stored kit. */
     public readonly load_state = signal<'loading' | 'ready' | 'failed'>(
         'loading',
     );
     public readonly can_edit = computed(
-        () =>
-            this._context.is_sys_admin() &&
-            !this.branding_disabled() &&
-            this.load_state() === 'ready',
+        () => canEditBrandKit(this._context) && this.load_state() === 'ready',
     );
 
     public readonly organisation = signal('');
-    public readonly colours = signal<string[]>(['#0E6E52']);
+    public readonly colours = signal<BrandColour[]>([
+        { key: 'primary', value: '#0E6E52' },
+    ]);
+    public readonly max_colours = MAX_COLOURS;
     public readonly font = signal('');
     public readonly saving = signal(false);
 
@@ -362,7 +367,7 @@ export class BrandingComponent implements OnInit {
         on_dark: '',
     });
     public readonly derived = signal<ImageGenLogoSlot | ''>('');
-    /** Palette colours after the three that the page edits. A save replaces
+    /** Palette colours after the ones that the page edits. A save replaces
      * the whole kit, so they are saved back as they are. */
     private _extra_palette: Record<string, string> = {};
 
@@ -412,13 +417,31 @@ export class BrandingComponent implements OnInit {
         this.load_state.set('ready');
     }
 
-    public colourName(index: number) {
-        return COLOUR_NAMES[index] || `colour ${index + 1}`;
+    public addColour() {
+        if (this.colours().length >= MAX_COLOURS) return;
+        this.colours.update((list) => [
+            ...list,
+            { key: this._freeKey(), value: '#1B2420' },
+        ]);
     }
 
-    public addColour() {
-        if (this.colours().length >= 3) return;
-        this.colours.update((list) => [...list, '#1B2420']);
+    /** the first palette key not in use, so a new colour replaces nothing */
+    private _freeKey() {
+        const used = new Set([
+            ...this.colours().map((colour) => colour.key),
+            ...Object.keys(this._extra_palette),
+        ]);
+        // one more numbered name than keys in use, so one is always free
+        const names = [
+            ...COLOUR_NAMES,
+            ...Array.from(
+                { length: used.size + 1 },
+                (_, index) => `colour ${index + 1}`,
+            ),
+        ];
+        return (
+            names.find((name) => !used.has(name)) || `colour ${used.size + 1}`
+        );
     }
 
     public removeColour(index: number) {
@@ -430,12 +453,16 @@ export class BrandingComponent implements OnInit {
     public static readonly COLOUR = /^#([0-9a-f]{3}|[0-9a-f]{6})$/i;
 
     public readonly colour_errors = computed(() =>
-        this.colours().map((colour) => !BrandingComponent.COLOUR.test(colour)),
+        this.colours().map(
+            (colour) => !BrandingComponent.COLOUR.test(colour.value),
+        ),
     );
 
     public setColour(index: number, value: string) {
         this.colours.update((list) =>
-            list.map((colour, i) => (i === index ? value : colour)),
+            list.map((colour, i) =>
+                i === index ? { ...colour, value } : colour,
+            ),
         );
     }
 
@@ -487,7 +514,7 @@ export class BrandingComponent implements OnInit {
             notifySuccess(i18n('SIGNAGE_MANAGER.IMAGE_GEN_LOGO_SAVED'));
         } catch (error) {
             notifyError(
-                errorMessage(error, i18n('SIGNAGE_MANAGER.BRAND_SAVE_FAILED')),
+                actionError(error, i18n('SIGNAGE_MANAGER.BRAND_SAVE_FAILED')),
             );
         } finally {
             this.busy.set('');
@@ -504,7 +531,7 @@ export class BrandingComponent implements OnInit {
             notifySuccess(i18n('SIGNAGE_MANAGER.BRAND_LOGO_MADE'));
         } catch (error) {
             notifyError(
-                errorMessage(error, i18n('SIGNAGE_MANAGER.BRAND_SAVE_FAILED')),
+                actionError(error, i18n('SIGNAGE_MANAGER.BRAND_SAVE_FAILED')),
             );
         } finally {
             this.busy.set('');
@@ -519,10 +546,11 @@ export class BrandingComponent implements OnInit {
         }
         this.saving.set(true);
         try {
+            // each colour keeps the key it was read from
             const palette = { ...this._extra_palette };
-            this.colours().forEach((colour, index) => {
-                palette[this.colourName(index)] = colour;
-            });
+            for (const colour of this.colours()) {
+                palette[colour.key] = colour.value;
+            }
             await this._image_gen.saveBrandKit({
                 organisation: this.organisation().trim() || undefined,
                 palette,
@@ -531,7 +559,7 @@ export class BrandingComponent implements OnInit {
             notifySuccess(i18n('SIGNAGE_MANAGER.BRAND_SAVED'));
         } catch (error) {
             notifyError(
-                errorMessage(error, i18n('SIGNAGE_MANAGER.BRAND_SAVE_FAILED')),
+                actionError(error, i18n('SIGNAGE_MANAGER.BRAND_SAVE_FAILED')),
             );
         } finally {
             this.saving.set(false);
@@ -548,10 +576,14 @@ export class BrandingComponent implements OnInit {
             ),
         ];
         if (ordered.length) {
-            this.colours.set(ordered.slice(0, 3).map((key) => palette[key]));
+            this.colours.set(
+                ordered
+                    .slice(0, MAX_COLOURS)
+                    .map((key) => ({ key, value: palette[key] })),
+            );
         }
         this._extra_palette = Object.fromEntries(
-            ordered.slice(3).map((key) => [key, palette[key]]),
+            ordered.slice(MAX_COLOURS).map((key) => [key, palette[key]]),
         );
         const font = brand.font;
         this.font.set(typeof font === 'string' ? font : font?.family || '');

apps/signage-manager/src/app/image-gen/image-gen-layer-controls.component.ts

@@ -233,10 +233,7 @@ export function newTextBlock(
             >
                 @if (!has_logo()) {
                     <span class="text-sm">{{
-                        (can_set_logo()
-                            ? 'SIGNAGE_MANAGER.IMAGE_GEN_NO_LOGO_YET'
-                            : 'SIGNAGE_MANAGER.IMAGE_GEN_NO_LOGO_ADMIN'
-                        ) | translate
+                        no_logo_note() | translate
                     }}</span>
                     @if (can_set_logo()) {
                         <button
@@ -361,6 +358,8 @@ export class ImageGenLayerControlsComponent {
     public readonly brand = input<ImageGenBrandKit | null>(null);
     public readonly uploading = input(false);
     public readonly can_set_logo = input(true);
+    /** whether the brand kit can be changed by anyone */
+    public readonly branding_editing = input(true);
 
     public readonly changed = output<ImageGenLayerState>();
     public readonly logo_picked = output<File>({ alias: 'logoPicked' });
@@ -372,6 +371,15 @@ export class ImageGenLayerControlsComponent {
         () => !!this.logo_on_light() && !!this.logo_on_dark(),
     );
 
+    /** points at the branding page only when someone can use it */
+    public readonly no_logo_note = computed(() =>
+        this.can_set_logo()
+            ? 'SIGNAGE_MANAGER.IMAGE_GEN_NO_LOGO_YET'
+            : this.branding_editing()
+              ? 'SIGNAGE_MANAGER.IMAGE_GEN_NO_LOGO_ADMIN'
+              : 'SIGNAGE_MANAGER.IMAGE_GEN_NO_LOGO_LOCKED',
+    );
+
     public readonly fonts = BRAND_FONTS;
 
     /** names the face a block falls back to */

apps/signage-manager/src/app/image-gen/image-gen-layer.component.ts

@@ -131,6 +131,23 @@ export class ImageGenLayerComponent {
         return typeof font === 'string' ? font : font?.family || '';
     });
 
+    /**
+     * Every face in use. Equal by content, so a drag, which makes new blocks
+     * with the same fonts, does not ask for the faces again.
+     */
+    private readonly _families = computed(
+        () =>
+            [
+                ...new Set(
+                    [
+                        this._brand_family(),
+                        ...this.state().blocks.map((block) => block.font),
+                    ].filter(Boolean),
+                ),
+            ].sort(),
+        { equal: (a, b) => a.join('\n') === b.join('\n') },
+    );
+
     constructor() {
         effect(() => {
             const url = this.image_url();
@@ -146,13 +163,7 @@ export class ImageGenLayerComponent {
         });
         effect(() => {
             // a face has to be in the document before a canvas can draw with it
-            const families = new Set(
-                [
-                    this._brand_family(),
-                    ...this.state().blocks.map((block) => block.font),
-                ].filter(Boolean),
-            );
-            for (const family of families) {
+            for (const family of this._families()) {
                 ensureBrandFont(family).then(() => this._draw());
             }
         });
@@ -546,7 +557,11 @@ export class ImageGenLayerComponent {
     }
 
     private _isLight(hex: string) {
-        const value = hex.replace('#', '');
+        let value = hex.replace('#', '');
+        // #rgb is short for #rrggbb
+        if (value.length === 3) {
+            value = [...value].map((digit) => digit + digit).join('');
+        }
         if (value.length < 6) return true;
         const r = parseInt(value.slice(0, 2), 16);
         const g = parseInt(value.slice(2, 4), 16);