Save the autofilled site logo as the workspace logo on brand settings update#173
Merged
Conversation
… update Brand autofill on the workspace settings page already captured the site logo and rendered a preview beneath the URL, but the update flow never persisted it. The store flow attached it via LogoAttacher; the update flow was missing all three legs: the form field, the request rule, and the controller attach. - BrandTab: add logo_url to the useForm payload so autofill can set it and the form submits it. - UpdateWorkspaceRequest: validate logo_url (nullable url) — FormRequest strips any unvalidated key, so without a rule it was silently dropped. - WorkspaceController::updateSettings: pull logo_url out of the validated data (it is not a column) and attach it through LogoAttacher, mirroring store.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
…/catch LogoAttacher::attach promised in its docblock that any failure — including a persistence error — is logged and swallowed so the caller need not handle it. But the persistence block was try/finally with no catch, so a Throwable from clearMediaCollection/addMediaFromPath escaped. Both call sites (store and updateSettings) each wrapped the call in an identical try/catch + Log::warning to compensate — a band-aid duplicated across the controller. Fix it at the root: the persistence block now catches Throwable, logs it, and returns false, matching the documented contract. Both controller call sites collapse to a single attach() line, and the now-unused Log/Throwable imports are dropped. Adds LogoAttacherTest covering the success path, the swallowed persistence failure, a failed fetch, and a rejected mime type.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On the workspace Brand settings page, running brand autofill from the site URL already fetches the site's logo and shows a preview beneath the URL — but saving the form never persisted that logo as the workspace logo. The logo silently disappeared on submit.
The create flow (
store) handled this correctly viaLogoAttacher; the update flow (updateSettings) was missing all three legs of the same wiring.Fix
Mirror the store flow across the three places the logo needs to travel:
BrandTab.vue— addlogo_urlto theuseFormpayload soBrandForm's autofill sets it and the form actually submits it.UpdateWorkspaceRequest— validatelogo_url(nullable,url,max:1024).FormRequest::validated()strips any key without a rule, so without this the field was dropped before the controller ever saw it (the same silent-drop gotcha documented for platform meta).WorkspaceController::updateSettings— pulllogo_urlout of the validated data (it is not a column) and attach it throughLogoAttacher, wrapped in the same try/catch +Log::warningasstore.Tests
tests/Feature/WorkspaceControllerTest.php:logo_urlis providedlogo_urlis providedBoth mutation-tested (removing the request rule / the attach call makes them fail). Full suite green.