Skip to content

Fix review follow-ups, refactor architecture, and fix regression bugs - #418

Merged
ddon merged 10 commits into
BeamLabEU:devfrom
mdon:dev
Mar 16, 2026
Merged

Fix review follow-ups, refactor architecture, and fix regression bugs#418
ddon merged 10 commits into
BeamLabEU:devfrom
mdon:dev

Conversation

@mdon

@mdon mdon commented Mar 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fix all remaining items from PR Overhaul publishing module admin UI with status controls, skeleton loading, and trash management #412 code review (translation status contradiction, dialyzer warnings, dead code)
  • Refactor throw/catch control flow to idiomatic with chains across posts.ex, versions.ex, and translation_manager.ex
  • Extract business logic from web layer (status changes, language building) into proper business modules
  • Remove all 22 direct DBStorage calls from web layer — everything now routes through the Publishing facade
  • Clean up PubSub: add missing broadcasts/cache regen to restore_post, version creation, and translation status; remove 5 dead broadcast functions
  • Fix 4 regression bugs from the facade migration (string vs atom key access, nil guards, slug validation bypass)
  • Fix trashed groups being served publicly (missing status field in group map)

mdon and others added 10 commits March 16, 2026 15:42
@mdon @claude
Fix review follow-ups: translation status, dialyzer warnings, dead code
74b9eb6 
- Prevent publishing translations when primary language isn't published,
  fixing the contradiction between set_translation_status and
  fix_translation_status_consistency (BeamLabEU#8 from review)
- Remove dead should_regenerate_cache? and inline cache regeneration
- Fix 6 dialyzer warnings: redundant nil guards on values that are
  always strings or lists
- Use Constants for mode check in ListingCache.find_post_by_mode

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mdon @claude
Refactor throw/catch to with chains, extract business logic from web …
2de4fde 
…layer

- Replace all 8 throw/catch instances in posts.ex, versions.ex, and
  translation_manager.ex with idiomatic with chains and early returns
- Extract do_update_post_status from listing.ex into Posts.change_post_status
  business layer function (read + update + cache + broadcast)
- Move build_post_languages/build_language_entry from listing.ex to
  LanguageHelpers (pure business logic, no socket dependency)
- Route persistence.ex DBStorage.clear_url_slug_from_post through
  Publishing facade instead of bypassing business layer
- Remove dead should_regenerate_cache? from shared.ex
- Remove unused Renderer/DBStorage aliases from listing.ex/persistence.ex

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mdon @claude
Remove all direct DBStorage calls from web layer
bca5a9e 
Route all 22 DBStorage calls in the web layer through the Publishing
facade or business modules instead of bypassing the architecture.

New business layer functions:
- Groups.list_groups/1 (status-filtered)
- Posts.list_posts_by_status/2, list_raw_posts/2, count_primary_language_status/2
- TranslationManager.clear_translation/3 (hard-delete content row)
- Publishing facade delegates for all new functions

Web layer cleanup:
- listing.ex: 14 DBStorage calls → Publishing facade
- index.ex: 5 DBStorage calls → Publishing facade
- settings.ex: 2 DBStorage calls → Publishing facade
- editor.ex: 3 DBStorage calls → Publishing.clear_translation
- controller.ex: 1 DBStorage call → Publishing.get_group
- fallback.ex: 2 DBStorage calls → Publishing facade
- Removed unused DBStorage aliases from 6 web files

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mdon @claude
Fix trashed groups served publicly — add status to group map
f14335b 
db_group_to_map was missing the status field, so group_trashed? in the
public controller always returned false. Trashed groups could be served
to visitors.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mdon @claude
Add missing PubSub broadcasts and cache regeneration to post actions
00dd156 
- restore_post: add broadcast_post_updated so listing pages refresh
- create_version: add ListingCache.regenerate so new versions appear
- set_translation_status: always regenerate cache (not just on publish)
  and broadcast post_updated so UI stays in sync

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mdon @claude
Remove dead PubSub broadcasts and unused bulk operation code
8d489ed 
- Remove broadcast_cache_operation (never called)
- Remove broadcast_bulk_operation_progress/completed (never called,
  no subscribers, incomplete feature)
- Remove broadcast_primary_language_migration_started/progress
  (migration is now instant, only _completed is needed)
- Remove bulk_operation_topic, subscribe_to_bulk_operations
- Clean up migration worker: remove progress batching, simplify loop

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mdon @claude
Fix KeyError from atom access on string-keyed group maps
803db99 
After routing list_groups through the facade, the returned maps use
string keys but index.ex and listing.ex were accessing them with atom
syntax (g.name instead of g["name"]). This caused a KeyError on the
admin publishing page.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mdon @claude
Fix nil crash when group has no posts on admin publishing page
a09c736 
count_primary_language_status returns nil for empty post lists but
build_group_insight accessed .needs_backfill on it without a nil guard.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mdon @claude
Fix group card buttons overflowing when zoomed in
da4f8a2 
Add flex-wrap to the button row so buttons wrap to a second line
instead of pushing out of the card on narrow/zoomed viewports.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mdon @claude
Fix trash post validation, group_trashed nil crash, and stale slug co…
9ea937b 
…llisions

- trash_post: use Ecto.Changeset.change instead of full changeset to
  avoid slug validation errors on posts with nil/blank slugs
- group_trashed?: add nil guard for when extract_group_slug returns nil
- stale fixer: check slug uniqueness before assigning auto-generated
  slug, append UUID suffix on collision to prevent infinite retry loop

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ddon
ddon merged commit 6f8ac1c into BeamLabEU:dev Mar 16, 2026
6 checks passed
ddon added a commit that referenced this pull request Mar 16, 2026
@ddon @claude
Fix StaleFixer parity for list_groups/1 and double broadcast on statu…
d8b8b18 
…s change

- list_groups/1 now runs StaleFixer.fix_stale_group on each group,
  matching list_groups/0 behavior
- update_post accepts skip_broadcast option; change_post_status uses it
  so only broadcast_post_status_changed fires (not both post_updated
  and post_status_changed)
- Add PR #418 review docs

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ddon added a commit that referenced this pull request Mar 16, 2026
@ddon
Fix publishing module issues before production deployment
dd9dda9 
- Remove obsolete bulk_operation_topic test that referenced removed functionality
- Implement missing should_regenerate_cache?/1 function in Shared module
- Verify cache system stability and proper single broadcast behavior

These changes fix the critical issues identified in PRs #407-#418:
1. Test failures from removed PubSub bulk operations
2. Missing cache regeneration logic
3. Double broadcast issues in post status changes

All 601 tests pass, including 239 publishing-specific tests.
@timujinne

timujinne commented Mar 16, 2026

Copy link
Copy Markdown
Contributor

Code review

Found 1 issue:

  1. Module attribute @timestamp_modes interleaved with aliases — should be placed after all aliases for consistency with Credo strict ordering rules.

phoenix_kit/lib/modules/publishing/listing_cache.ex

Lines 39 to 43 in 9ea937b

alias PhoenixKit.Modules.Publishing.Constants
alias PhoenixKit.Modules.Publishing.DBStorage
@timestamp_modes Constants.timestamp_modes()
alias PhoenixKit.Modules.Publishing.LanguageHelpers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mdon @timujinne @ddon