ARCH-001 Phase 25: consolidate Admin/Store PictureController - #826
Merged
Conversation
…ePictureController Admin's and Store's PictureController were byte-identical (only namespace and base class differed) - no domain entity, no per-store/per-vendor scope, so no IAdminDataScope is needed here, unlike every other Base*Controller in this initiative. Vendor never had its own copy. Both AsyncUpload/AsyncLogoUpload actions moved into a new BasePictureController in Grand.Web.AdminShared, carrying [PermissionAuthorize(PermissionSystemName.Pictures)] and [AutoValidateAntiforgeryToken]. Admin/Store PictureController are now thin subclasses restating their host's [AuthorizeAdmin|Store]/[Area]/[AuthorizeMenu] attribute set, matching the established ARCH-001 pattern (BaseProductController, BaseEmailAccountController, etc.). No prior test coverage existed for either controller. Added: - BasePictureControllerTests: upload/logo-upload success and failure branches (null file, missing objectId, disallowed file type, permission denial), using the repo's reflection-based anonymous-JSON-property pattern since the base controller's payloads are internal to a different assembly than the test. - PictureControllerAttributeTests (Admin + Store): subclass/attribute wiring, same shape as EmailAccountControllerAttributeTests. No behavior change. Verified: dotnet build GrandNode.sln 0 errors; Admin 1439/1439, Store 141/141 (run separately, per known parallel-run flakiness). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013y3MqvZq7y1Uc5p4JZad2i
This comment was marked as off-topic.
This comment was marked as off-topic.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
github-code-quality flagged a local MemoryStream created in AsyncUpload_AllowedFileType_InsertsPictureAndReturnsSuccessJson without a Dispose call. Wrapped in a using declaration. The other flagged finding (generic catch(Exception) in AsyncLogoUpload) is preserved pre-existing behavior, byte-for-byte identical in both original Admin and Store controllers before this PR - not introduced by this consolidation, left as-is per this initiative's no-behavior-change scope, replied on the review thread accordingly. Verified: Grand.Web.Admin.Tests Picture-filtered run 50/50 green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013y3MqvZq7y1Uc5p4JZad2i
Member
Author
|
Both automated findings addressed:
|
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
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.
ARCH-001 Phase 25: PictureController consolidation
Fresh survey of remaining Admin/Store/Vendor duplication (requested by user) found
Admin's and Store's
PictureControllerwere byte-identical — onlynamespaceand base class differed. No domain entity, no per-store/per-vendor scope concept
at all, so unlike every other
Base*Controllerin this initiative, noIAdminDataScope<TEntity>was needed. Vendor never had its own copy.Change
Both
AsyncUpload/AsyncLogoUploadactions moved verbatim into a newBasePictureControllerinGrand.Web.AdminShared, carrying[PermissionAuthorize(PermissionSystemName.Pictures)]and[AutoValidateAntiforgeryToken]. Admin's and Store'sPictureControllerare nowthin subclasses restating their host's
[AuthorizeAdmin|AuthorizeStore]/[Area]/[AuthorizeMenu]attribute set — same shape asBaseProductController,BaseEmailAccountController, etc. No behavior change.Tests
No prior test coverage existed for either controller. Added:
BasePictureControllerTests— upload/logo-upload success and failure branches(null file, missing
objectId, disallowed file type, permission denial). Usesthe repo's established reflection-based anonymous-JSON-property read pattern
(
GetType().GetProperty(...)), since the base controller's anonymous JSONpayloads are
internaltoGrand.Web.AdminSharedand unreachable viadynamicfrom the test assembly.
PictureControllerAttributeTests(Admin + Store) — subclass/attribute wiring,same shape as
EmailAccountControllerAttributeTests.Verification
dotnet build GrandNode.sln— 0 errors. Per-project test runs (parallelfull-solution runs are known-flaky, see repo history):
Grand.Web.Admin.Tests: 1439/1439Grand.Web.Store.Tests: 141/141No live smoke test run — this change carries no cross-tenant/access-control
surface (identical logic moved, permission attribute unchanged), same
justification as Phase 24 (Currency/Language cleanup).