Repository navigation
chore(notifications): B-series follow-up cleanups - #130
Merged
Merged
Conversation
deliver_async/3 used to silently return {:error, reason} from
Oban.insert/1 and rely on the caller to log it. Callers in change
modules and LiveViews don't have an obvious place to surface that —
the email just never gets sent. Add a Logger.warning at the enqueue
site so a misconfigured Oban or DB outage at least shows up in logs.
The {:error, _} tuple still propagates to the caller; this is purely
additive instrumentation.
Three branches of the role-swap logic each repeated the same
{:ok, _} -> :ok / {:error, r} -> {:error, r} case. Pull the pattern
out into a private ok_or_error/1 so the action body reads as a flat
pipeline.
No behavior change.
The helper was called humanize_role/1 but it doesn't humanize anything — it just substitutes "member" when the role string is empty and otherwise returns it verbatim. role_label/1 describes what it does. Pure rename, no behavior change.
Two cosmetic cleanups in group_membership_notifications_test: - flush_mailbox/0 was defined mid-module between two describe blocks. Move it to the bottom of the module where private helpers conventionally live in this codebase. - Remove a Process.sleep(0) that was decorating an Oban.drain_queue/1 call. drain_queue is synchronous, so the zero-millisecond sleep was a no-op.
mwoods79
force-pushed
the
chore/notifications-b-series-followups
branch
from
April 30, 2026 20:00
3be9e3d to
cce926c
Compare
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.
Summary
Four small follow-ups on the B-series notifications work merged in #129. None of them are functionally novel; they're cleanups that should have shipped with #129. Splitting into a separate PR rather than carrying them onto the upcoming C/D-series branch keeps that diff focused.
feat(notifications): log Oban enqueue failures in deliver_async— only behavior change. Adds aLogger.warningwhenOban.insert/1rejects the enqueue. Caller still gets{:error, reason}back; this is purely additive instrumentation.refactor(group): extract ok_or_error helper in transfer_ownership— three branches of the role-swap repeated the samecaseclauses; pulled into a private helper.refactor(notifications): rename humanize_role to role_label— the helper didn't actually humanize anything, just labelled. Pure rename inSenders.GroupRoleChanged.refactor(test): tidy flush_mailbox placement and drop dead sleep— moves the private helper to the bottom of the module and drops aProcess.sleep(0)that was decorating an already-synchronousOban.drain_queue/1.Test plan
mix precommitclean (921 tests, credo clean)