Merge pull request #157 from basecamp/everyone

Implement all-access buckets

app/assets/stylesheets/toggles.css

@@ -0,0 +1,13 @@
+.toggler--toggled {
+  .toggler__visible-when-off {
+    display: none;
+  }
+
+  .toggler__visible-when-on {
+    display: unset;
+  }
+}
+
+.toggler__visible-when-on {
+  display: none;
+}

app/controllers/buckets_controller.rb

@@ -38,7 +38,7 @@ class BucketsController < ApplicationController
     end
 
     def bucket_params
-      params.expect(bucket: [ :name ])
+      params.expect(bucket: [ :name, :all_access ]).with_defaults(all_access: false)
     end
 
     def grantees

app/helpers/accesses_helper.rb

@@ -0,0 +1,15 @@
+module AccessesHelper
+  def access_menu_tag(bucket, &)
+    tag.menu class: [ "flex flex-column gap margin-none pad txt-medium", { "toggler--toggled": bucket.all_access? } ], data: {
+      controller: "filter toggle-class",
+      filter_active_class: "filter--active", filter_selected_class: "selected",
+      toggle_class_toggle_class: "toggler--toggled" }, &
+  end
+
+  def access_toggles_for(users, selected:)
+    render partial: "buckets/access_toggle",
+      collection: users, as: :user,
+      locals: { selected: selected },
+      cached: ->(user) { [ user, selected ] }
+  end
+end

app/models/bucket/accessible.rb

@@ -15,12 +15,20 @@ module Bucket::Accessible
       end
 
       def revoke_from(users)
-        destroy_by user: users
+        destroy_by user: users unless proxy_association.owner.all_access?
       end
     end
 
     has_many :users, through: :accesses
 
+    scope :all_access, -> { where(all_access: true) }
+
     after_create -> { accesses.grant_to creator }
+    after_save_commit :grant_access_to_everyone
   end
+
+  private
+    def grant_access_to_everyone
+      accesses.grant_to(account.users) if all_access_previously_changed?(to: true)
+    end
 end

app/models/filter/resources.rb

@@ -12,5 +12,11 @@ module Filter::Resources
     kind = resource.class.model_name.plural
     send "#{kind}=", send(kind).without(resource)
     empty? ? destroy! : save!
+  rescue ActiveRecord::RecordNotUnique
+    destroy!
+  end
+
+  def buckets
+    creator.buckets.where id: bucket_ids
   end
 end

app/models/user.rb

@@ -21,6 +21,8 @@ class User < ApplicationRecord
   validates_presence_of :email_address
   normalizes :email_address, with: ->(value) { value.strip.downcase }
 
+  after_create_commit :grant_access_to_buckets
+
   scope :active, -> { where(active: true) }
   scope :alphabetically, -> { order("LOWER(name)") }
 
@@ -48,4 +50,8 @@ class User < ApplicationRecord
     def deactived_email_address
       email_address.sub(/@/, "-deactivated-#{SecureRandom.uuid}@")
     end
+
+    def grant_access_to_buckets
+      Access.insert_all account.buckets.all_access.pluck(:id).collect { |bucket_id| { bucket_id: bucket_id, user_id: id } }
+    end
 end

app/views/bubbles/_messages.html.erb

@@ -5,6 +5,6 @@
     data-created-by-current-user-mine-class="comment--mine">
   <%# Template Dependency: comments/comment %>
   <%# Template Dependency: event_summaries/event_summary %>
-  <%= render bubble.messages, cache: true %>
+  <%= render bubble.messages, cached: true %>
   <%= render "comments/new", bubble: bubble %>
 </section>

app/views/buckets/_access_toggle.erb

@@ -0,0 +1,21 @@
+<li class="flex align-center gap-half margin-none unpad" data-value="<%= user.name.downcase %>">
+  <figure class="avatar flex-item-no-shrink">
+    <%= avatar_tag user, loading: :lazy %>
+  </figure>
+
+  <div class="min-width">
+    <div class="overflow-ellipsis">
+      <strong><%= user.name %></strong>
+    </div>
+  </div>
+
+  <hr class="separator--horizontal flex-item-grow" style="--border-color: var(--color-subtle-dark); --border-style: dashed" aria-hidden="true">
+
+  <%= image_tag "check.svg", size: 20, class: "toggler__visible-when-on colorize--black flex-item-no-shrink", aria: { hidden: "true" } %>
+
+  <label class="switch toggler__visible-when-off flex-item-no-shrink">
+    <%= check_box_tag "user_ids[]", user.id, selected, class: "switch__input", id: nil %>
+    <span class="switch__btn round"></span>
+    <span class="for-screen-reader">Give <%= user.name %> access</span>
+  </label>
+</li>

app/views/buckets/_user_toggle.html.erb

@@ -1,24 +0,0 @@
-<li class="flex align-center gap-half margin-none unpad" data-value="<%= user.name.downcase %>">
-  <figure class="avatar flex-item-no-shrink">
-    <%= avatar_tag user, loading: :lazy %>
-  </figure>
-
-  <div class="min-width">
-    <div class="overflow-ellipsis">
-      <strong><%= user.name %></strong>
-    </div>
-  </div>
-
-  <hr class="separator--horizontal flex-item-grow" style="--border-color: var(--color-subtle-dark); --border-style: dashed" aria-hidden="true">
-
-  <% if user == Current.user %>
-    <%= hidden_field_tag "user_ids[]", user.id, id: nil %>
-    <%= image_tag "check.svg", size: 20, class: "colorize--black flex-item-no-shrink", aria: { hidden: "true" } %>
-  <% else %>
-    <label class="switch flex-item-no-shrink">
-      <%= check_box_tag "user_ids[]", user.id, selected, class: "switch__input", id: nil %>
-      <span class="switch__btn round"></span>
-      <span class="for-screen-reader">Give <%= user.name %> access</span>
-    </label>
-  <% end %>
-</li>

app/views/buckets/accesses/edit.html.erb

@@ -1,14 +0,0 @@
-<h1>Users on <%= @bucket.name %></h1>
-
-<%= form_with url: bucket_access_path, method: :put do |form| %>
-  <ul>
-    <% Current.account.users.active.each do |user| %>
-      <li>
-        <%= check_box_tag "user_ids[]", user.id, @bucket.users.include?(user), id: nil, disabled: user == Current.user %>
-        <%= user.name %>
-      </li>
-    <% end %>
-  </ul>
-
-  <%= form.submit %>
-<% end %>

app/views/buckets/edit.html.erb

@@ -17,7 +17,7 @@
 <% end %>
 
 <div class="panel shadow center" style="--panel-size: 55ch;">
-  <%= form_with model: @bucket, class: "flex flex-column gap txt-large", controller: "form" do |form| %>
+  <%= form_with model: @bucket, class: "flex flex-column gap txt-large", data: { controller: "form" } do |form| %>
     <div class="flex align-center gap">
       <%= translation_button(:bucket_name) %>
 
@@ -29,7 +29,7 @@
     </div>
 
     <section class="margin-block pad-inline fill-shade border-radius">
-      <menu class="flex flex-column gap margin-none pad txt-medium" data-controller="filter" data-filter-active-class="filter--active" data-filter-selected-class="selected">
+      <%= access_menu_tag @bucket do %>
         <li class="flex align-center gap margin-none">
           <figure class="avatar flex-item-no-shrink" style="--avatar-border-radius: 0; --avatar-size: 4ch;">
             <%= image_tag "everyone.svg", aria: { hidden: "true" }, class: "colorize--black" %>
@@ -42,13 +42,11 @@
 
           <hr class="separator--horizontal flex-item-grow" style="--border-color: var(--color-subtle-dark); --border-style: dashed" aria-hidden="true" />
 
-          <%= link_to nil, class: "btn--faux flex-inline", tabindex: "-1" do %>
-            <label class="switch">
-              <input type="checkbox" lass="switch__input">
-              <span class="switch__btn round"></span>
-              <span class="for-screen-reader">Give everyone access to this project</span>
-            </label>
-          <% end %>
+          <label for="bucket_all_access" class="switch">
+            <%= form.check_box :all_access, class: "switch__input", checked: @bucket.all_access?, data: { action: "change->toggle-class#toggle" } %>
+            <span class="switch__btn round"></span>
+            <span class="for-screen-reader">Give everyone access to this project</span>
+          </label>
         </li>
 
         <hr class="separator--horizontal full-width" style="--border-color: var(--color-subtle-dark);" aria-hidden="true" />
@@ -58,15 +56,15 @@
         <% end %>
 
         <div data-filter-target="list">
-          <%= render partial: "buckets/user_toggle", collection: @selected_users, as: :user, locals: { selected: true } %>
+          <%= access_toggles_for @selected_users, selected: true %>
 
           <% if @selected_users.any? && @unselected_users.any? %>
             <hr class="separator full-width" style="--border-style: solid">
           <% end %>
 
-          <%= render partial: "buckets/user_toggle", collection: @unselected_users, as: :user, locals: { selected: false } %>
+          <%= access_toggles_for @unselected_users, selected: false %>
         </div>
-      </menu>
+      <% end %>
     </section>
 
     <button type="submit" id="log_in" class="btn btn--reversed center">

db/migrate/20241209223551_add_all_access_to_bucket.rb

@@ -0,0 +1,5 @@
+class AddAllAccessToBucket < ActiveRecord::Migration[8.1]
+  def change
+    add_column :buckets, :all_access, :boolean, default: false, null: false
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.0].define(version: 2024_11_20_222444) do
+ActiveRecord::Schema[8.1].define(version: 2024_12_09_223551) do
   create_table "accesses", force: :cascade do |t|
     t.integer "bucket_id", null: false
     t.integer "user_id", null: false
@@ -104,6 +104,7 @@ ActiveRecord::Schema[8.0].define(version: 2024_11_20_222444) do
     t.string "name", null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.boolean "all_access", default: false, null: false
     t.index ["account_id"], name: "index_buckets_on_account_id"
     t.index ["creator_id"], name: "index_buckets_on_creator_id"
   end

test/controllers/bubbles_controller_test.rb

@@ -49,4 +49,14 @@ class BubblesControllerTest < ActionDispatch::IntegrationTest
     assert_equal "moon.jpg", bubble.image.filename.to_s
     assert_equal [ tags(:mobile) ], bubble.tags
   end
+
+  test "users can only see bubbles in buckets they have access to" do
+    get bucket_bubble_url(buckets(:writebook), bubbles(:logo))
+    assert_response :success
+
+    buckets(:writebook).update! all_access: false
+    buckets(:writebook).accesses.revoke_from users(:kevin)
+    get bucket_bubble_url(buckets(:writebook), bubbles(:logo))
+    assert_response :not_found
+  end
 end

test/controllers/buckets_controller_test.rb

@@ -37,6 +37,20 @@ class BucketsControllerTest < ActionDispatch::IntegrationTest
     assert_redirected_to bubbles_path(bucket_ids: [ buckets(:writebook) ])
     assert_equal "Writebook bugs", buckets(:writebook).reload.name
     assert_equal users(:david, :jz), buckets(:writebook).users
+    assert_not buckets(:writebook).all_access?
+  end
+
+  test "update all access" do
+    bucket = Current.set(session: sessions(:kevin)) do
+      accounts("37s").buckets.create! name: "New bucket", all_access: false
+    end
+    assert_equal [ users(:kevin) ], bucket.users
+
+    patch bucket_url(bucket), params: { bucket: { name: "Bugs", all_access: true } }
+
+    assert_redirected_to bubbles_path(bucket_ids: [ bucket ])
+    assert bucket.reload.all_access?
+    assert_equal accounts("37s").users, bucket.users
   end
 
   test "destroy" do

test/fixtures/buckets.yml

@@ -2,3 +2,4 @@ writebook:
   name: Writebook
   account: 37s
   creator: david
+  all_access: true

test/fixtures/sessions.yml

@@ -1,2 +1,5 @@
 david:
   user: david
+
+kevin:
+  user: kevin

test/models/bucket_test.rb

@@ -2,6 +2,8 @@ require "test_helper"
 
 class BucketTest < ActiveSupport::TestCase
   test "revising access" do
+    buckets(:writebook).update! all_access: false
+
     buckets(:writebook).accesses.revise granted: users(:david, :jz), revoked: users(:kevin)
     assert_equal users(:david, :jz), buckets(:writebook).users
 
@@ -11,4 +13,21 @@ class BucketTest < ActiveSupport::TestCase
     buckets(:writebook).accesses.revoke_from users(:kevin)
     assert_not_includes buckets(:writebook).users.reload, users(:kevin)
   end
+
+  test "grants access to everyone after creation" do
+    bucket = Current.set(session: sessions(:david)) do
+      accounts("37s").buckets.create! name: "New bucket", all_access: true
+    end
+    assert_equal accounts("37s").users, bucket.users
+  end
+
+  test "grants access to everyone after update" do
+    bucket = Current.set(session: sessions(:david)) do
+      accounts("37s").buckets.create! name: "New bucket"
+    end
+    assert_equal [ users(:david) ], bucket.users
+
+    bucket.update! all_access: true
+    assert_equal accounts("37s").users, bucket.users.reload
+  end
 end

test/models/filter_test.rb

@@ -41,6 +41,13 @@ class FilterTest < ActiveSupport::TestCase
     assert_equal [ bubbles(:shipping) ], filter.bubbles
   end
 
+  test "can't see bubbles in buckets that aren't accessible" do
+    buckets(:writebook).update! all_access: false
+    buckets(:writebook).accesses.revoke_from users(:david)
+
+    assert_empty users(:david).filters.new(bucket_ids: [ buckets(:writebook).id ]).bubbles
+  end
+
   test "turning into params" do
     expected = { indexed_by: "most_discussed", tag_ids: [ tags(:mobile).id ], assignee_ids: [ users(:jz).id ], filter_id: filters(:jz_assignments).id }
     assert_equal expected.stringify_keys, filters(:jz_assignments).to_params.to_h
@@ -92,6 +99,15 @@ class FilterTest < ActiveSupport::TestCase
     end
   end
 
+  test "duplicate filters are removed after a resource is destroyed" do
+    users(:david).filters.create! tag_ids: [ tags(:mobile).id ], bucket_ids: [ buckets(:writebook).id ]
+    users(:david).filters.create! tag_ids: [ tags(:mobile).id, tags(:web).id ], bucket_ids: [ buckets(:writebook).id ]
+
+    assert_difference "Filter.count", -1 do
+      tags(:web).destroy!
+    end
+  end
+
   test "summary" do
     assert_equal "Most discussed, tagged #Mobile, and assigned to JZ in all projects", filters(:jz_assignments).summary
   end

test/models/user_test.rb

@@ -12,6 +12,16 @@ class UserTest < ActiveSupport::TestCase
     assert_equal user, User.authenticate_by(email_address: "victor@hey.com", password: "secret123456")
   end
 
+  test "creation gives access to all_access buckets" do
+    user = User.create! \
+      account: accounts("37s"),
+      name: "Victor Cooper",
+      email_address: "victor@hey.com",
+      password: "secret123456"
+
+    assert_equal [ buckets(:writebook) ], user.buckets
+  end
+
   test "deactivate" do
     assert_changes -> { users(:jz).active? }, from: true, to: false do
       users(:jz).deactivate