diff --git a/app/controllers/slack_onboarding_controller.rb b/app/controllers/slack_onboarding_controller.rb index 4a85a84..e9f337d 100644 --- a/app/controllers/slack_onboarding_controller.rb +++ b/app/controllers/slack_onboarding_controller.rb @@ -25,6 +25,13 @@ def show @step = current_user ? current_user.slack_onboarding_step : :sign_up end + # The current step, as JSON. The page polls it while it waits on something + # that can happen elsewhere (accepting in Slack, the promotion), and reloads + # when the step changes. + def status + render json: { step: current_user.slack_onboarding_step } + end + # Sends the Slack invite, or sends it again. def create unless %i[request_invite accept_invite].include?(current_user.slack_onboarding_step) diff --git a/app/javascript/controllers/index.js b/app/javascript/controllers/index.js index 09a2054..fe9590f 100644 --- a/app/javascript/controllers/index.js +++ b/app/javascript/controllers/index.js @@ -13,6 +13,9 @@ application.register("fingerprint", FingerprintController) import LoginFormController from "./login_form_controller" application.register("login-form", LoginFormController) +import OnboardingStatusController from "./onboarding_status_controller" +application.register("onboarding-status", OnboardingStatusController) + import PasswordController from "./password_controller" application.register("password", PasswordController) diff --git a/app/javascript/controllers/onboarding_status_controller.js b/app/javascript/controllers/onboarding_status_controller.js new file mode 100644 index 0000000..bb920f3 --- /dev/null +++ b/app/javascript/controllers/onboarding_status_controller.js @@ -0,0 +1,34 @@ +import { Controller } from "@hotwired/stimulus" + +// Reloads the /slack onboarding page when the person's step changes somewhere +// else, for example when they accept the Code of Conduct in Slack. Polls the +// status endpoint while the tab is visible, and stops after a while. +export default class extends Controller { + static values = { url: String, step: String } + + connect() { + this.checks = 0 + this.timer = setInterval(() => this.check(), 5000) + } + + disconnect() { + clearInterval(this.timer) + } + + async check() { + if (document.hidden) return + if (++this.checks > 120) return this.disconnect() + + try { + const response = await fetch(this.urlValue, { headers: { Accept: "application/json" } }) + if (!response.ok) return + const { step } = await response.json() + if (step !== this.stepValue) { + this.disconnect() + window.Turbo ? window.Turbo.visit(window.location.href, { action: "replace" }) : window.location.reload() + } + } catch { + // A failed check is retried on the next tick. + } + } +} diff --git a/app/jobs/slack_code_of_conduct_accepted_job.rb b/app/jobs/slack_code_of_conduct_accepted_job.rb index 4286e1e..b58dfb2 100644 --- a/app/jobs/slack_code_of_conduct_accepted_job.rb +++ b/app/jobs/slack_code_of_conduct_accepted_job.rb @@ -8,9 +8,11 @@ # A full member who joined before the code-of-conduct flow is already a member, # so only the acceptance is recorded for them. # -# message is the Slack DM that asked for the acceptance, as { channel:, ts: }. -# When it is given, the DM is replaced with a thank-you so its button can't be -# used again. +# Then every code-of-conduct DM the user still has (User#slack_coc_messages) +# is replaced with a thank-you, wherever they accepted, so no live button is +# left behind. message is the DM the acceptance came from, as +# { channel:, ts: }. It is replaced too, which covers a Slack user with no +# Weave account. class SlackCodeOfConductAcceptedJob < ApplicationJob queue_as :default @@ -23,10 +25,16 @@ def perform(slack_user_id, message: nil) service = SlackService.new return unless service.configured? - already_member = user&.slack_member? - replace_message(service, message, already_member:) if message.present? - return if already_member + already_member = user&.slack_member? || false + promote(service, user, slack_user_id) unless already_member + replace_messages(service, user, message, already_member:) + rescue SlackService::ConfigurationError => e + Rails.logger.error "Slack not configured for promotion: #{e.message}" + end + + private + def promote(service, user, slack_user_id) result = service.promote_to_member(slack_user_id) if result[:ok] user&.update_columns(slack_membership: "member", updated_at: Time.current) @@ -34,16 +42,15 @@ def perform(slack_user_id, message: nil) else Rails.logger.error "Failed to promote #{slack_user_id} after CoC acceptance: #{result[:error]}" end - rescue SlackService::ConfigurationError => e - Rails.logger.error "Slack not configured for promotion: #{e.message}" end - private - - def replace_message(service, message, already_member:) - service.mark_code_of_conduct_accepted(channel: message[:channel], ts: message[:ts], already_member:) - rescue => e - Rails.logger.error "Failed to update CoC DM #{message.inspect}: #{e.message}" + def replace_messages(service, user, message, already_member:) + messages = [*user&.take_code_of_conduct_messages!, message].compact.uniq + messages.each do |ref| + service.mark_code_of_conduct_accepted(channel: ref[:channel], ts: ref[:ts], already_member:) + rescue => e + Rails.logger.error "Failed to update CoC DM #{ref.inspect}: #{e.message}" + end end end diff --git a/app/models/user.rb b/app/models/user.rb index 9c70302..ec08b86 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -29,6 +29,7 @@ # slack_birthday :date # slack_city :string # slack_coc_accepted_at :datetime +# slack_coc_messages :jsonb not null # slack_coc_requested_at :datetime # slack_cost_center :string # slack_country :string @@ -502,6 +503,30 @@ def slack_onboarding_step def slack_onboarding_complete? = slack_onboarding_step == :member + # Remembers a code-of-conduct DM that has a live button, so it can be + # replaced once the user accepts (see SlackCodeOfConductAcceptedJob). Appends + # in SQL, so two DMs sent at the same time are both kept. + def remember_code_of_conduct_message!(channel:, ts:) + return if channel.blank? || ts.blank? + + self.class.where(id: id).update_all( # rubocop:disable Rails/SkipsModelValidations + ["slack_coc_messages = slack_coc_messages || ?::jsonb", [{ channel: channel, ts: ts }].to_json] + ) + end + + # Returns the remembered DMs and forgets them. The row lock keeps a DM that + # is remembered at the same moment from being lost. (with_lock is not used: + # User#lock! means "lock the account".) + def take_code_of_conduct_messages! + messages = self.class.transaction do + rows = self.class.where(id: id) + taken = rows.lock.pick(:slack_coc_messages) || [] + rows.update_all(slack_coc_messages: []) # rubocop:disable Rails/SkipsModelValidations + taken + end + messages.map { |message| message.symbolize_keys.slice(:channel, :ts) } + end + # The membership a Slack user object (from users.info, users.lookupByEmail, # users.list or a team_join/user_change event) entitles its owner to. Guests # and deactivated accounts are not members. diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 7565eb8..8a70bf4 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -29,6 +29,7 @@ # slack_birthday :date # slack_city :string # slack_coc_accepted_at :datetime +# slack_coc_messages :jsonb not null # slack_coc_requested_at :datetime # slack_cost_center :string # slack_country :string diff --git a/app/services/slack_service.rb b/app/services/slack_service.rb index 6757176..77263c8 100644 --- a/app/services/slack_service.rb +++ b/app/services/slack_service.rb @@ -217,11 +217,14 @@ def post_code_of_conduct(slack_user_id, title: nil, paragraphs: nil, form: false web_url: Rails.application.routes.url_helpers.slack_onboarding_url(**Rails.application.config.action_mailer.default_url_options) ) - @client.chat_postMessage( + response = @client.chat_postMessage( channel: slack_user_id, text: "Please review and accept the Patchwork Labs Code of Conduct.", blocks: message[:blocks] ) + # Remembered so that accepting anywhere (Slack or the web) replaces it. + User.find_by(slack_id: slack_user_id)&.remember_code_of_conduct_message!(channel: response["channel"], ts: response["ts"]) + response end # Opens the code-of-conduct form from a button click. trigger_id comes from diff --git a/app/views/slack_onboarding/show.html.erb b/app/views/slack_onboarding/show.html.erb index ef10f24..96a060f 100644 --- a/app/views/slack_onboarding/show.html.erb +++ b/app/views/slack_onboarding/show.html.erb @@ -14,7 +14,12 @@ link_class = "text-grape hover:text-ink font-medium underline decoration-dashed underline-offset-2" %> -
+<%# While the next step can happen in Slack, check now and then and reload + when it does. %> +<%= tag.div class: "min-h-full flex items-center justify-center px-4 py-12", + data: (if current_user && %i[accept_invite accept_code_of_conduct awaiting_promotion].include?(@step) + { controller: "onboarding-status", onboarding_status_url_value: status_slack_onboarding_path, onboarding_status_step_value: @step } + end) do %>
@@ -142,4 +147,4 @@ <% end %>
- +<% end %> diff --git a/config/routes.rb b/config/routes.rb index bd21da3..8035ea4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -33,6 +33,7 @@ # GET /(*path) redirect(302) # root GET / home#index # accept_code_of_conduct_slack_onboarding POST /slack/code-of-conduct(.:format) slack_onboarding#accept_code_of_conduct +# status_slack_onboarding GET /slack/status(.:format) slack_onboarding#status # slack_onboarding GET /slack(.:format) slack_onboarding#show # POST /slack(.:format) slack_onboarding#create # GET /.well-known/*path well_known#show @@ -359,6 +360,7 @@ # request or resend the invite. resource :slack_onboarding, path: "slack", only: [:show, :create], controller: "slack_onboarding" do post :accept_code_of_conduct, path: "code-of-conduct" + get :status end # Well-known routes for standard compliance diff --git a/db/migrate/20261010120000_add_slack_coc_messages_to_users.rb b/db/migrate/20261010120000_add_slack_coc_messages_to_users.rb new file mode 100644 index 0000000..d5b0671 --- /dev/null +++ b/db/migrate/20261010120000_add_slack_coc_messages_to_users.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddSlackCocMessagesToUsers < ActiveRecord::Migration[8.1] + def change + # The code-of-conduct DMs sent to a user that still have a live button, as + # [{ "channel" => ..., "ts" => ... }]. They are all replaced with a + # thank-you once the user accepts, from Slack or the web. + add_column :users, :slack_coc_messages, :jsonb, default: [], null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 3f3e977..5a6ae88 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -12,7 +12,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_10_09_140000) do +ActiveRecord::Schema[8.1].define(version: 2026_10_10_120000) do # These are extensions that must be enabled in order to support this database enable_extension "fuzzystrmatch" enable_extension "pg_catalog.plpgsql" @@ -590,6 +590,7 @@ t.date "slack_birthday" t.string "slack_city" t.datetime "slack_coc_accepted_at" + t.jsonb "slack_coc_messages", default: [], null: false t.datetime "slack_coc_requested_at" t.string "slack_cost_center" t.string "slack_country" diff --git a/docs/CODE_OF_CONDUCT.md b/docs/CODE_OF_CONDUCT.md index 0079056..2854904 100644 --- a/docs/CODE_OF_CONDUCT.md +++ b/docs/CODE_OF_CONDUCT.md @@ -16,6 +16,8 @@ flowchart TD - The DM button accepts at once. When the Slack import saved a placeholder name (`NOTSET`, `Unknown`, `User`), the button opens a Slack form instead. The form has a required "I have read and accept" box, and name fields when the name is missing. - The email links to sign-in. After sign-in, `/` sends the member to `/slack`, which shows the same accept step and name fields. - The message text is in `config/locales/code_of_conduct_request.en.yml`. +- Weave remembers every CoC DM it sends (`users.slack_coc_messages`). When the person accepts, in Slack or on `/slack`, `SlackCodeOfConductAcceptedJob` replaces all of them with a thank-you, so no live button is left. +- While `/slack` waits on a step that can happen in Slack, it polls `GET /slack/status` every 5 seconds and reloads when the step changes. ## Runbook diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 297d1bb..c404b93 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -29,6 +29,7 @@ # slack_birthday :date # slack_city :string # slack_coc_accepted_at :datetime +# slack_coc_messages :jsonb not null # slack_coc_requested_at :datetime # slack_cost_center :string # slack_country :string diff --git a/spec/jobs/slack_code_of_conduct_accepted_job_spec.rb b/spec/jobs/slack_code_of_conduct_accepted_job_spec.rb index 8822896..8bf45a4 100644 --- a/spec/jobs/slack_code_of_conduct_accepted_job_spec.rb +++ b/spec/jobs/slack_code_of_conduct_accepted_job_spec.rb @@ -49,4 +49,27 @@ described_class.perform_now("U123", message: { channel: "D1", ts: "1.2" }) end + + it "replaces every DM the user still has, wherever they accepted, and forgets them" do + user.update!(slack_membership: "member") + user.remember_code_of_conduct_message!(channel: "D1", ts: "1.1") + user.remember_code_of_conduct_message!(channel: "D1", ts: "1.2") + replaced = [] + allow(service).to receive(:mark_code_of_conduct_accepted) { |**ref| replaced << ref } + + described_class.perform_now("U123", message: { channel: "D1", ts: "1.2" }) + + expect(replaced).to eq([{ channel: "D1", ts: "1.1", already_member: true }, { channel: "D1", ts: "1.2", already_member: true }]) + expect(user.reload.slack_coc_messages).to eq([]) + end + + it "keeps replacing the other DMs when one fails" do + user.update!(slack_membership: "member") + user.remember_code_of_conduct_message!(channel: "D1", ts: "1.1") + user.remember_code_of_conduct_message!(channel: "D1", ts: "1.2") + allow(service).to receive(:mark_code_of_conduct_accepted).with(hash_including(ts: "1.1")).and_raise(Slack::Web::Api::Errors::SlackError, "message_not_found") + expect(service).to receive(:mark_code_of_conduct_accepted).with(hash_including(ts: "1.2")) + + described_class.perform_now("U123") + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 298cec2..3bca8a4 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -29,6 +29,7 @@ # slack_birthday :date # slack_city :string # slack_coc_accepted_at :datetime +# slack_coc_messages :jsonb not null # slack_coc_requested_at :datetime # slack_cost_center :string # slack_country :string diff --git a/spec/requests/slack_onboarding_spec.rb b/spec/requests/slack_onboarding_spec.rb index d8e6e13..880a80f 100644 --- a/spec/requests/slack_onboarding_spec.rb +++ b/spec/requests/slack_onboarding_spec.rb @@ -89,6 +89,35 @@ end end + describe "GET /slack/status" do + it "reports the current step, so the page can reload when it changes elsewhere" do + user.update!(slack_id: "U1", slack_membership: "member") + sign_in_via_magic_link(user) + + get status_slack_onboarding_path + + expect(response.parsed_body).to eq("step" => "accept_code_of_conduct") + end + + it "makes the accept step check the status" do + user.update!(slack_id: "U1", slack_membership: "member") + sign_in_via_magic_link(user) + + get slack_onboarding_path + + expect(response.body).to include('data-controller="onboarding-status"', status_slack_onboarding_path) + end + + it "does not check the status once the person is a full member" do + user.update!(slack_id: "U1", slack_membership: "member", slack_coc_accepted_at: 1.day.ago) + sign_in_via_magic_link(user) + + get slack_onboarding_path + + expect(response.body).not_to include("onboarding-status") + end + end + describe "POST /slack" do before { sign_in_via_magic_link(user) } diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb index 4d13374..68e8389 100644 --- a/spec/serializers/user_serializer_spec.rb +++ b/spec/serializers/user_serializer_spec.rb @@ -29,6 +29,7 @@ # slack_birthday :date # slack_city :string # slack_coc_accepted_at :datetime +# slack_coc_messages :jsonb not null # slack_coc_requested_at :datetime # slack_cost_center :string # slack_country :string diff --git a/spec/services/slack_service_code_of_conduct_spec.rb b/spec/services/slack_service_code_of_conduct_spec.rb index 525fdb6..f5e6d27 100644 --- a/spec/services/slack_service_code_of_conduct_spec.rb +++ b/spec/services/slack_service_code_of_conduct_spec.rb @@ -30,6 +30,15 @@ def block_types(blocks) = blocks.map { |block| block[:type] } service.post_code_of_conduct("U1", title: "Hello", paragraphs: ["First paragraph"]) end + it "remembers the DM, so accepting anywhere can replace it" do + user = create(:user, slack_id: "U1") + allow(client).to receive(:chat_postMessage).and_return({ "ok" => true, "channel" => "D1", "ts" => "1.2" }) + + service.post_code_of_conduct("U1") + + expect(user.reload.slack_coc_messages).to eq([{ "channel" => "D1", "ts" => "1.2" }]) + end + it "uses the form button when asked" do expect(client).to receive(:chat_postMessage) do |args| button = args[:blocks].find { |block| block[:type] == "actions" }[:elements].first diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 61376e8..5cbfbc7 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -33,6 +33,7 @@ # slack_birthday :date # slack_city :string # slack_coc_accepted_at :datetime +# slack_coc_messages :jsonb not null # slack_coc_requested_at :datetime # slack_cost_center :string # slack_country :string diff --git a/test/models/user_test.rb b/test/models/user_test.rb index e974bc0..7b2be59 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -29,6 +29,7 @@ # slack_birthday :date # slack_city :string # slack_coc_accepted_at :datetime +# slack_coc_messages :jsonb not null # slack_coc_requested_at :datetime # slack_cost_center :string # slack_country :string