Feat/company invitations#32
Merged
Merged
Conversation
Create invitations table with hashed token, status enum, 7-day expiry, and unique partial index on (company_id, email) for pending invitations. Add Invitation schema with changeset validations (invitable roles only, email normalization) and :invitation factory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ccept Implements create_invitation/3 (owner-only, hashed token), accept_invitation/2 (token verification, membership creation via Multi), cancel_invitation/2, list_pending_invitations/1, and accept_pending_invitations_for_email/1 for auto-accepting on sign-up. All functions enforce authorization and expiry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Delivers invitation emails via Swoosh with accept URL, company name, and role. Follows the same pattern as UserNotifier for confirmation and password reset emails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
InvitationAcceptLive validates token and accepts if logged in, or redirects to login with return_to param if not. Add return_to support to UserLoginLive (hidden field), UserSessionController (param pass-through), and UserAuth.log_in_user (sanitized redirect). Open-redirect protection validates path-only URLs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Call Invitations.accept_pending_invitations_for_email/1 after user registration (UserRegistrationLive) and Google sign-in (AuthController) to automatically grant company access for pending invitations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner-only TeamLive page for viewing members, sending invitations, cancelling pending invitations, and removing members. Add list_members/1, delete_membership/1, and update_membership_role/2 to Companies context. Add Team nav link (owner-only) to sidebar layout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use Company.t() instead of map() in send_invitation_email spec - Log email delivery failures instead of silently discarding - Fix misplaced @SPEC render above defp sanitize_return_to - Add {:error, Ecto.Changeset.t()} to do_accept_invitation spec - Replace html =~ with has_element? using CSS selectors in tests - Add mass-assignment test for Invitation changeset - Add open-redirect tests for sanitize_return_to - Add cross-tenant test for cancel_invitation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a token-based company invitation subsystem (create/accept/cancel/auto-accept), Invitation schema and notifier, owner-only Team LiveView and invitation-accept LiveView, membership management helpers, return_to sanitization, DB migrations, UI/router/auth changes, extensive tests, and docs checklist updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Owner as Owner
participant TeamLV as Team LiveView
participant Invitations as Invitations
participant Notifier as InvitationNotifier
participant Mailer as Mailer
participant DB as Database
Owner->>TeamLV: submit invite form
TeamLV->>Invitations: create_invitation(company_id, owner_id, attrs)
Invitations->>Invitations: authorize owner, check membership
Invitations->>Invitations: generate raw token, hash token
Invitations->>DB: insert invitation (token_hash, expires_at, pending)
DB-->>Invitations: {:ok, invitation}
Invitations-->>TeamLV: {:ok, invitation, raw_token}
TeamLV->>Notifier: deliver_invitation(email, accept_url, ctx)
Notifier->>Mailer: deliver(email)
Mailer-->>Notifier: {:ok, email}
Notifier-->>TeamLV: {:ok, email}
TeamLV-->>Owner: show success
sequenceDiagram
participant User as User
participant AcceptLV as InvitationAcceptLive
participant Invitations as Invitations
participant Companies as Companies
participant DB as Database
User->>AcceptLV: GET /invitations/accept/:token
alt authenticated
AcceptLV->>Invitations: accept_invitation(token, user)
Invitations->>Invitations: lookup by token_hash, check expiry, check membership
Invitations->>DB: Ecto.Multi -> insert membership, update invitation status
DB-->>Invitations: {:ok, {invitation, membership}}
Invitations-->>AcceptLV: {:ok, ...}
AcceptLV-->>User: redirect /dashboard (success)
else not authenticated
AcceptLV-->>User: redirect /users/log-in?return_to=/invitations/accept/:token
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@lib/ksef_hub_web/live/invitation_accept_live.ex`:
- Around line 33-61: handle_accept/3 currently matches :ok, :not_found,
:expired, and :already_member but misses the {:error, %Ecto.Changeset{}} case
returned by Invitations.accept_invitation(token, user); add a new clause
matching {:error, %Ecto.Changeset{} = changeset} that returns {:ok, socket |>
assign(page_title: "Invitation Error") |> assign(error: :changeset, accepted:
false) |> assign(changeset: changeset)} (or include a put_flash with a generic
error message if preferred) so transaction-level validation failures are handled
without raising a FunctionClauseError.
In `@lib/ksef_hub_web/live/team_live.ex`:
- Around line 92-112: The remove_member handler lacks server-side role
validation and can allow deleting an owner; inside handle_event("remove_member")
after fetching membership with Companies.get_membership(member_user_id,
company.id) check membership.role (or membership.user_role) and if it equals
"owner" return {:noreply, put_flash(socket, :error, "Cannot remove company
owner.")} without calling Companies.delete_membership; otherwise proceed with
the existing delete flow that calls Companies.delete_membership and
load_team_data().
In `@lib/ksef_hub/invitations.ex`:
- Around line 54-60: The Invitation.changeset currently allows :status and
:expires_at to be mass-assigned; update the cast list in Invitation.changeset to
only [:email, :role] (remove :status and :expires_at) and modify the call site
that builds the %Invitation{} (the block that pipes into Invitation.changeset)
to set expires_at and status directly on the struct (alongside company_id,
invited_by_id, token_hash) before calling Invitation.changeset so those fields
are not cast from user input.
- Around line 140-155: The cancel_invitation/2 function can race with concurrent
accept_invitation because it reads the invitation with Repo.get and later
updates it; change it to perform an atomic conditional update using
Repo.update_all on Invitation filtered by id and status == "pending" after
authorization: call Companies.authorize(user_id, invitation.company_id,
["owner"]) as before (you may still Repo.get the invitation to check
existence/ownership), then run Repo.update_all(from(i in Invitation, where: i.id
== ^invitation_id and i.status == "pending"), set: [status: "cancelled"]) and
inspect the returned {count, _} to return {:ok, updated_invitation} when count
== 1 or {:error, :not_found} when count == 0 so an already-accepted invitation
is not overwritten.
- Around line 42-66: The create_invitation flow currently calls
check_not_already_member/2 and then Repo.insert which can race; refactor
create_invitation to build an Ecto.Multi: first use Multi.run (or Multi.run/3)
to perform Companies.authorize(user_id, company_id, ...) and then
check_not_already_member(company_id, email) inside the same transaction, next
use Multi.insert to insert the %Invitation{} via Invitation.changeset (using the
same token_hash/expires_at/status), and finally call Repo.transaction(multi) and
return {:ok, %{invitation: invitation, token: raw_token}} or {:error, reason}
based on the transaction result so the membership check and invitation insert
are atomic.
🧹 Nitpick comments (7)
lib/ksef_hub_web/controllers/auth_controller.ex (1)
38-43: Auto-accept integration looks correct, but consider adding observability.The placement of
accept_pending_invitations_for_email/1after successful Google sign-in is correct. The function always returns{:ok, memberships}per the implementation inlib/ksef_hub/invitations.ex, so discarding the return value won't hide errors.However, for observability, consider logging successful auto-accepts to help with debugging and auditing:
📊 Optional: Add logging for observability
{:ok, user} -> - Invitations.accept_pending_invitations_for_email(user) + {:ok, memberships} = Invitations.accept_pending_invitations_for_email(user) + + if memberships != [] do + require Logger + Logger.info("Auto-accepted #{length(memberships)} invitation(s) for user #{user.id}") + end conn |> put_flash(:info, "Welcome, #{user.name || user.email}!")priv/repo/migrations/20260210300001_create_invitations.exs (1)
20-22: Consider adding an index onThe
accept_pending_invitations_for_email/1function queries invitations by email without a company_id filter, making a dedicated email index beneficial as the table grows. While a composite index on[:company_id, :email]exists, it cannot efficiently support email-only filtering.🔧 Proposed index addition
create index(:invitations, [:company_id]) create index(:invitations, [:invited_by_id]) create index(:invitations, [:token_hash], unique: true) + create index(:invitations, [:email])lib/ksef_hub_web/live/user_registration_live.ex (1)
32-33: Consider logging failures from invitation acceptance.The call to
accept_pending_invitations_for_email/1ignores its return value. While this is correct behavior (registration should succeed regardless), any failures during invitation acceptance will be silent. Consider capturing the result for logging, similar to how confirmation email failures are logged below (lines 41-43).💡 Optional enhancement for observability
- Invitations.accept_pending_invitations_for_email(user) + case Invitations.accept_pending_invitations_for_email(user) do + {:ok, memberships} when memberships != [] -> + require Logger + Logger.info("Auto-accepted #{length(memberships)} invitation(s) for user #{user.id}") + + _ -> + :ok + endtest/ksef_hub_web/live/invitation_accept_live_test.exs (1)
43-71: Consider adding a test for already-accepted invitation.The tests cover expired and invalid tokens well. Per the AI summary, there should also be an "already-member" case that redirects with a notice. Consider adding a test for attempting to accept an invitation when the user is already a member of the company.
📝 Suggested test addition
test "shows notice when user is already a member", %{conn: conn, company: company, owner: owner, user: user} do # User is already a member from accepting the first invitation in setup # Create another invitation for the same user {:ok, %{token: token}} = Invitations.create_invitation(owner.id, company.id, %{ email: "another-email@example.com", # Different email but same user role: "invoice_reviewer" }) # This test would need adjustment based on actual already-member handling # Verify appropriate redirect or message endlib/ksef_hub_web/live/team_live.ex (2)
116-124: Consider using LiveView streams for the collections.The coding guidelines specify to always use LiveView streams for collections to avoid memory ballooning. While team lists are typically small, using streams would align with the project conventions and prevent issues if teams grow larger.
♻️ Proposed refactor using streams
`@spec` load_team_data(Phoenix.LiveView.Socket.t()) :: Phoenix.LiveView.Socket.t() defp load_team_data(socket) do company = socket.assigns.current_company members = Companies.list_members(company.id) pending_invitations = Invitations.list_pending_invitations(company.id) socket - |> assign(members: members, pending_invitations: pending_invitations) + |> stream(:members, members, reset: true) + |> stream(:pending_invitations, pending_invitations, reset: true) endThen update mount to initialize streams and update the template to use
@streams.membersand@streams.pending_invitations.As per coding guidelines: "Always use LiveView streams for collections to avoid memory ballooning and runtime termination."
126-145: Silently swallowing email delivery errors may hide issues.The
send_invitation_email/3function logs a warning on failure but returns:okregardless. While this prevents user-facing errors for email delivery issues, consider whether the calling code should be aware of the failure (e.g., to show a different flash message like "Invitation created but email delivery failed").lib/ksef_hub_web/live/user_login_live.ex (1)
37-49: Consider extracting the duplicatedsanitize_return_to/1helper to a shared module.This function appears identically in both
lib/ksef_hub_web/user_auth.ex(lines 65–76) andlib/ksef_hub_web/live/user_login_live.ex(lines 38–49). Extract it to a shared module (e.g.,KsefHubWeb.SecurityorKsefHubWeb.UrlSanitizer) to maintain a single source of truth for URL sanitization logic and reduce maintenance burden.
…ements
- Tighten Invitation changeset: remove :status/:expires_at from cast list,
set them on struct at call sites to prevent mass-assignment
- Wrap create_invitation in Ecto.Multi for atomic authorize + member check + insert
- Make cancel_invitation atomic with Repo.update_all to prevent race with accept
- Add {:error, %Ecto.Changeset{}} handler to InvitationAcceptLive
- Add server-side owner removal guard in TeamLive remove_member handler
- Show different flash when invitation email delivery fails
- Use LiveView streams for members and pending_invitations collections
- Extract duplicated sanitize_return_to/1 to shared KsefHubWeb.UrlHelpers
- Add observability logging for auto-accept on sign-up and Google login
- Add email index on invitations table for accept_pending_invitations_for_email
- Add test: already-member invitation accept, owner removal guard, status/expires_at
not castable
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/invitations.ex`:
- Around line 124-143: The current pre-check in do_accept_invitation calling
Companies.get_membership(user.id, invitation.company_id) causes a race; move
that membership existence check into the Ecto.Multi using Multi.run/3 (e.g., add
a :check_membership run that queries Companies.get_membership/2 and returns
{:ok, :exists} or {:error, :already_member}) and then perform the
Multi.insert(:membership, ...) after the run so the check and insert are atomic;
remove the outer if-else pre-check and ensure the Multi.run returns {:error,
:already_member} when a membership exists so concurrent requests yield a
consistent {:error, :already_member} response instead of a changeset DB error.
🧹 Nitpick comments (3)
lib/ksef_hub_web/live/user_registration_live.ex (1)
32-37: Moverequire Loggerto module level.
require Loggerappears inline at both line 35 and line 47. While this works (it's a compile-time directive), the idiomatic approach is to placerequire Loggeronce at the module level alongside the aliases.♻️ Suggested refactor
Add at module level (after line 10):
alias KsefHub.Invitations require LoggerThen simplify the inline code:
{:ok, memberships} = Invitations.accept_pending_invitations_for_email(user) if memberships != [] do - require Logger Logger.info("Auto-accepted #{length(memberships)} invitation(s) for user #{user.id}") endAnd similarly at line 47:
{:error, _reason} -> - require Logger Logger.error("Failed to deliver confirmation email")lib/ksef_hub_web/live/team_live.ex (1)
19-35: Consider using a plug or hook for owner authorization.The owner check in
mount/3works correctly, but duplicating authorization logic in LiveViews can become error-prone as the codebase grows. Consider extracting this to a reusableon_mounthook for owner-only pages.lib/ksef_hub/invitations.ex (1)
216-236: Silent failure in auto-accept may hide issues.The
accept_pending_invitations_for_email/1function silently ignores errors fromdo_accept_invitation/2(line 231). While this prevents one failed invitation from blocking others, consider logging errors for observability so issues like database failures or unexpected states are visible.♻️ Proposed logging for failed auto-accepts
+ require Logger + `@spec` accept_pending_invitations_for_email(User.t()) :: {:ok, [Membership.t()]} def accept_pending_invitations_for_email(%User{} = user) do email = normalize_email(user.email) now = DateTime.utc_now() invitations = Invitation |> where([i], i.email == ^email) |> where([i], i.status == "pending") |> where([i], i.expires_at > ^now) |> Repo.all() memberships = Enum.reduce(invitations, [], fn invitation, acc -> case do_accept_invitation(invitation, user) do {:ok, %{membership: membership}} -> [membership | acc] - {:error, _} -> acc + {:error, reason} -> + Logger.warning("Failed to auto-accept invitation #{invitation.id}: #{inspect(reason)}") + acc end end) {:ok, Enum.reverse(memberships)} end
Move membership existence check into Ecto.Multi in do_accept_invitation to prevent race condition. Extract owner authorization to reusable :require_owner on_mount hook. Move require Logger to module level in UserRegistrationLive. Add warning logs for auto-accept failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/invitations.ex`:
- Around line 126-147: do_accept_invitation/2 currently uses Multi.update with a
stale invitation struct which can re-accept a cancelled invite; replace the
Multi.update(:invitation, ...) step with a conditional update using
Repo.update_all (or a Multi.run that calls Repo.update_all) to set status:
"accepted" only when status == "pending" (mirror do_atomic_cancel/1 approach),
return {:error, :not_found} when update_all returns {0, _} (no rows updated),
and update the `@spec` for do_accept_invitation/2 to include | {:error,
:not_found}; keep the rest of the Multi (check_membership, insert membership)
and ensure you map the update_all result into the {:ok, invitation} shape for
the success clause.
Replace Multi.update (stale struct) with atomic Repo.update_all that only sets status to "accepted" when status is still "pending", preventing re-acceptance of cancelled invitations. Mirrors do_atomic_cancel pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/invitations.ex`:
- Around line 122-166: In do_accept_invitation update the transactional guard
and error handling: extend the Repo.update_all WHERE to include expires_at >
^now so the row is only claimed if still valid, and if the update count is 0
then fetch the invitation (Repo.get/1) and return {:error, :expired} when
invitation.expires_at <= now versus {:error, :not_found} otherwise; also update
the `@spec` for do_accept_invitation to include {:error, :expired} as a possible
return. Ensure you reference the function do_accept_invitation, the Multi.run
block labeled :invitation that calls Repo.update_all, and the final
Repo.transaction result clause to add a branch for {:error, :invitation,
:expired, _changes} mapping to {:error, :expired}.
…_found
Extend Repo.update_all WHERE clause in do_accept_invitation to include
expires_at > now so expired invitations are not claimed. When update
count is 0, fetch the invitation to distinguish {:error, :expired} from
{:error, :not_found}.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract atomic update and failure classification into do_atomic_accept/1 and classify_accept_failure/2 to resolve nesting depth and cyclomatic complexity warnings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Feb 11, 2026
Merged
This was referenced Mar 24, 2026
Merged
9 tasks
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.
Summary by CodeRabbit