Skip to content

Send owner transfer email when requested - #1011

Merged
cocomarine merged 9 commits into
mainfrom
1766-send-transfer-request-email
Sep 14, 2026
Merged

cocomarine merged 9 commits into
mainfrom
1766-send-transfer-request-email

Conversation

@cocomarine

@cocomarine cocomarine commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Status

What for?

  • This PR adds backend functionality to send an email to a prospective owner (when the current school owner clicks transfer button).
  • Out of scope:
    • Actual ownership transfer
    • Hooking it up to the frontend

What's changed?

  • Added SchoolOwnershipMailer and template
    • Add instance variables (such as those for current owner name and prospective owner name) to the email template when controller actions are set up and plumbed to the frontend.
    • Followed what the invitation emails do and added text.erb template (deviating slightly from copy doc due to lack of formatting inline link and bullet points)
  • Added ownership_transfers table and its model
  • The model calls and queues the mailer and the mailer reads @school off the ownership_transfer param to build the email.

Emails

Screenshot 2026-09-14 at 12 18 43
  • When triggered in rails console:
    • Queues GoodJob
    Screenshot 2026-09-14 at 11 09 38 - Email gets sent via Postmark dev sandbox
Screenshot 2026-09-14 at 13 15 43

@cla-bot cla-bot Bot added the cla-signed label Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown

Test coverage

93.57% line coverage reported by SimpleCov.
Run: https://github.com/RaspberryPiFoundation/editor-api/actions/runs/34854447714

@raspberrypiherokubot
raspberrypiherokubot temporarily deployed to editor-api-p-1766-send--uw2tba September 11, 2026 16:15 Inactive
@cocomarine
cocomarine temporarily deployed to editor-api-p-1766-send--uw2tba September 14, 2026 11:08 Inactive
@cocomarine
cocomarine marked this pull request as ready for review September 14, 2026 11:27
Copilot AI lite review requested due to automatic review settings September 14, 2026 11:27
@cocomarine
cocomarine temporarily deployed to editor-api-p-1766-send--uw2tba September 14, 2026 11:31 Inactive

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The email token URL and current-owner name rendering remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds backend persistence and email notifications for school ownership-transfer requests.

Changes:

  • Adds encrypted ownership-transfer records and tokens.
  • Adds HTML/text email templates, previews, factories, and specs.
  • Updates local public URL configuration.
File summaries
File Summary
spec/models/ownership_transfer_spec.rb Tests transfer behavior.
spec/mailers/school_ownership_mailer_spec.rb Tests email rendering.
spec/mailers/previews/school_ownership_mailer_preview.rb Adds mailer preview.
spec/mailers/previews/invitation_preview.rb Updates invitation preview URL.
spec/factories/ownership_transfer.rb Adds transfer factory.
db/schema.rb Records schema changes.
db/migrate/20260911104254_ownership_transfers.rb Creates transfer table.
app/views/school_ownership_mailer/request_ownership_transfer.text.erb Plain-text email content.
app/views/school_ownership_mailer/request_ownership_transfer.html.erb HTML email content.
app/models/ownership_transfer.rb Defines transfer persistence and email callback.
app/mailers/school_ownership_mailer.rb Builds ownership-transfer emails.
.env.example Updates local public URL.
Review details

Suppressed comments (3)

app/mailers/school_ownership_mailer.rb:9

  • @current_owner_name and @nominee_name are never assigned by this action, so every delivered email falls back to Hi there and <school name> has nominated you...; the school name is not the current owner's name. Populate these values from the transfer/request context or use neutral copy that does not substitute the school name for a person.
    @school = ownership_transfer.school
    @token = ownership_transfer.generate_token_for(:ownership_transfer)

app/views/school_ownership_mailer/request_ownership_transfer.html.erb:3

  • The mailer never assigns @current_owner_name, so this fallback renders the school name as the person: “Elmwood Secondary School has nominated you…”. That is factually wrong in every sent HTML email; pass the owner's name into the mailer/transfer record or use a neutral fallback until that data is available.
<p><%= @current_owner_name || @school.name %> has nominated you to become the new owner of the Code Classroom account for <%= @school.name %>.</p>

app/views/school_ownership_mailer/request_ownership_transfer.text.erb:3

  • The mailer never assigns @current_owner_name, so this fallback renders the school name as the person: “Elmwood Secondary School has nominated you…”. That is factually wrong in every sent text email; pass the owner's name into the mailer/transfer record or use a neutral fallback until that data is available.
<%= @current_owner_name || @school.name %> has nominated you to become the new owner of the Code Classroom account for <%= @school.name %>.
  • Files reviewed: 12/12 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread app/mailers/school_ownership_mailer.rb Outdated
Comment thread app/views/school_ownership_mailer/request_ownership_transfer.html.erb Outdated
Comment thread app/views/school_ownership_mailer/request_ownership_transfer.html.erb Outdated
@zetter-rpf

Copy link
Copy Markdown
Contributor

❓ Do we need both text.erb and html.erb templates?

I would follow what the invitation emails do unless there's a reason to change

Comment thread app/views/school_ownership_mailer/request_ownership_transfer.html.erb Outdated

@zetter-rpf zetter-rpf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some suggestions of things that might be able to be removed as they are not being used/set up as part of this so keeping them might be confusing.

Let me know if I can re-review

@cocomarine

Copy link
Copy Markdown
Contributor Author

❓ Do we need both text.erb and html.erb templates?

I would follow what the invitation emails do unless there's a reason to change

OK, removed html.erb.

@cocomarine
cocomarine temporarily deployed to editor-api-p-1766-send--uw2tba September 14, 2026 12:17 Inactive
@cocomarine
cocomarine temporarily deployed to editor-api-p-1766-send--uw2tba September 14, 2026 12:22 Inactive
Comment thread app/models/ownership_transfer.rb Outdated

@zetter-rpf zetter-rpf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved given that we'll remove the references to tokens

@cocomarine
cocomarine temporarily deployed to editor-api-p-1766-send--uw2tba September 14, 2026 14:15 Inactive
@cocomarine
cocomarine merged commit 78e10ab into main Sep 14, 2026
8 checks passed
@cocomarine
cocomarine deleted the 1766-send-transfer-request-email branch September 14, 2026 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants