From f5cc30773557a8828b48b7f2143c46f0385942c8 Mon Sep 17 00:00:00 2001 From: Benjamin Armintor Date: Wed, 10 Jul 2024 10:55:04 -0400 Subject: [PATCH 1/7] update .ruby-version to match RVM wrappers (HOURS-129) --- .ruby-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ruby-version b/.ruby-version index 73ad478..c939cb5 100644 --- a/.ruby-version +++ b/.ruby-version @@ -1 +1 @@ -ruby-2.7.5 \ No newline at end of file +ruby-3.0.3 \ No newline at end of file From 0a57dcdb231332477c7edfcb6c0ea4fe8ac78575 Mon Sep 17 00:00:00 2001 From: Benjamin Armintor Date: Wed, 10 Jul 2024 14:12:16 -0400 Subject: [PATCH 2/7] administrators can suppress an inactive calendar (HOURS-118) - add a missing error template for internal_server_error (HOURS-130) - correct datatype of fk indexes to match id column types (bigint) --- app/controllers/locations_controller.rb | 13 ++++-- .../errors/internal_server_error.html.erb | 5 +++ app/views/errors/not_found.html.erb | 5 +++ app/views/locations/_location_form.html.erb | 9 ++++ app/views/locations/show.html.erb | 3 ++ config/locales/en.yml | 3 +- ...06_add_location_suppress_display_switch.rb | 9 ++++ db/schema.rb | 44 +++++++++---------- spec/features/locations_spec.rb | 31 +++++++++++++ 9 files changed, 96 insertions(+), 26 deletions(-) create mode 100644 app/views/errors/internal_server_error.html.erb create mode 100644 app/views/errors/not_found.html.erb create mode 100644 db/migrate/20240710150406_add_location_suppress_display_switch.rb diff --git a/app/controllers/locations_controller.rb b/app/controllers/locations_controller.rb index ebd8d90..cb761f5 100644 --- a/app/controllers/locations_controller.rb +++ b/app/controllers/locations_controller.rb @@ -9,9 +9,9 @@ def index path.blank? or path == '/' end if home_page - @locations = Location.where(front_page: true).where.not(code: 'all') + @locations = Location.where(front_page: true).where.not(code: 'all', suppress_display: true) else - @locations = Location.where.not(code: 'all') + @locations = Location.where.not(code: 'all', suppress_display: true) end @locations = @locations.order(:name) @now = Time.current @@ -59,6 +59,13 @@ def destroy end def show + unless @location && (current_user || !@location.suppress_display) + respond_to do |format| + format.html { render "errors/not_found", status: 404, layout: "public" } + format.json { render json: {}, status: 404 } + end + return + end @secondary_locations = Location.where(primary_location: @location).load @date = params[:date] ? Date.parse(params[:date]) : Date.current respond_to do |format| @@ -129,7 +136,7 @@ def create_params def update_params if current_user.administrator? - params.require(:location).permit(:name, :code, :comment, :comment_two, :url, :summary, :primary, :primary_location_id, :front_page, :short_note, :short_note_url) + params.require(:location).permit(:name, :code, :comment, :comment_two, :url, :summary, :primary, :primary_location_id, :front_page, :short_note, :short_note_url, :suppress_display) else params.require(:location).permit(:comment, :comment_two, :url, :summary, :short_note, :short_note_url) end diff --git a/app/views/errors/internal_server_error.html.erb b/app/views/errors/internal_server_error.html.erb new file mode 100644 index 0000000..881855f --- /dev/null +++ b/app/views/errors/internal_server_error.html.erb @@ -0,0 +1,5 @@ +
+

Internal Server Error

+ +

Please contact a site administrator to report an error at this location.

+
diff --git a/app/views/errors/not_found.html.erb b/app/views/errors/not_found.html.erb new file mode 100644 index 0000000..749522c --- /dev/null +++ b/app/views/errors/not_found.html.erb @@ -0,0 +1,5 @@ +
+

Location not found

+ +

Contact a site administrator if you think this is an error.

+
diff --git a/app/views/locations/_location_form.html.erb b/app/views/locations/_location_form.html.erb index c0dde19..2c31684 100644 --- a/app/views/locations/_location_form.html.erb +++ b/app/views/locations/_location_form.html.erb @@ -18,6 +18,15 @@ <% end %> +
+ <%= f.label :suppress_display %> + <% if current_user.administrator? %> + <%= f.check_box :suppress_display %> + <% elsif @location.suppress_display %> +

<%= t("location.suppressed") %>

+ <% end %> +
+ <% if current_user.administrator? %>
<%= f.label :front_page %> diff --git a/app/views/locations/show.html.erb b/app/views/locations/show.html.erb index ac90ec7..b6da047 100644 --- a/app/views/locations/show.html.erb +++ b/app/views/locations/show.html.erb @@ -4,6 +4,9 @@

<%= link_to @location.name, @location.url %>

+ <% if @location.suppress_display %> +

Public display of this location is suppressed.

+ <% end %> <% if @location.primary_location %>

Check hours for: <%= link_to @location.primary_location.name, location_path(@location.primary_location.code) %>

<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index cf9b342..098952c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -30,4 +30,5 @@ # available at https://guides.rubyonrails.org/i18n.html. en: - hello: "Hello world" + location: + suppressed: "Public display of this location is suppressed." diff --git a/db/migrate/20240710150406_add_location_suppress_display_switch.rb b/db/migrate/20240710150406_add_location_suppress_display_switch.rb new file mode 100644 index 0000000..7e1767f --- /dev/null +++ b/db/migrate/20240710150406_add_location_suppress_display_switch.rb @@ -0,0 +1,9 @@ +class AddLocationSuppressDisplaySwitch < ActiveRecord::Migration[6.0] + def up + add_column :locations, :suppress_display, :boolean, default: false + end + + def down + remove_column :locations, :suppress_display + end +end diff --git a/db/schema.rb b/db/schema.rb index 77dbe38..fce2d8b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2,53 +2,53 @@ # of editing this file, please use the migrations feature of Active Record to # incrementally modify your database, and then regenerate this schema definition. # -# This file is the source Rails uses to define your schema when running `rails -# db:schema:load`. When creating a new database, `rails db:schema:load` tends to +# This file is the source Rails uses to define your schema when running `bin/rails +# db:schema:load`. When creating a new database, `bin/rails db:schema:load` tends to # be faster and is potentially less error prone than running all of your # migrations from scratch. Old migrations may fail to apply correctly if those # migrations use external dependencies or application code. # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_02_28_151512) do - - create_table "locations", charset: "utf8", force: :cascade do |t| +ActiveRecord::Schema[7.0].define(version: 2024_07_10_150406) do + create_table "locations", force: :cascade do |t| t.string "name", null: false t.string "code", null: false t.string "url" t.text "comment" t.text "comment_two" t.string "summary" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime "created_at", precision: nil, null: false + t.datetime "updated_at", precision: nil, null: false t.bigint "primary_location_id" t.boolean "primary", default: false t.boolean "front_page", default: false, null: false t.string "short_note" t.text "short_note_url" + t.boolean "suppress_display", default: false t.index ["code"], name: "index_locations_on_code" t.index ["front_page"], name: "index_locations_on_front_page" t.index ["primary"], name: "index_locations_on_primary" t.index ["primary_location_id"], name: "index_locations_on_primary_location_id" end - create_table "permissions", charset: "utf8", force: :cascade do |t| + create_table "permissions", force: :cascade do |t| t.bigint "user_id" t.string "role", null: false t.string "subject_class" t.integer "subject_id" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime "created_at", precision: nil, null: false + t.datetime "updated_at", precision: nil, null: false t.index ["user_id"], name: "index_permissions_on_user_id" end - create_table "timetables", charset: "utf8", force: :cascade do |t| + create_table "timetables", force: :cascade do |t| t.date "date" - t.datetime "open" - t.datetime "close" + t.datetime "open", precision: nil + t.datetime "close", precision: nil t.text "notes" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime "created_at", precision: nil, null: false + t.datetime "updated_at", precision: nil, null: false t.boolean "tbd", default: false, null: false t.boolean "closed", default: false, null: false t.string "note" @@ -60,17 +60,17 @@ t.index ["open"], name: "index_timetables_on_open" end - create_table "users", charset: "utf8", force: :cascade do |t| + create_table "users", force: :cascade do |t| t.string "reset_password_token" - t.datetime "reset_password_sent_at" - t.datetime "remember_created_at" + t.datetime "reset_password_sent_at", precision: nil + t.datetime "remember_created_at", precision: nil t.integer "sign_in_count", default: 0, null: false - t.datetime "current_sign_in_at" - t.datetime "last_sign_in_at" + t.datetime "current_sign_in_at", precision: nil + t.datetime "last_sign_in_at", precision: nil t.string "current_sign_in_ip" t.string "last_sign_in_ip" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime "created_at", precision: nil, null: false + t.datetime "updated_at", precision: nil, null: false t.string "uid" t.string "provider" t.string "email", default: "", null: false diff --git a/spec/features/locations_spec.rb b/spec/features/locations_spec.rb index b583161..dde090e 100644 --- a/spec/features/locations_spec.rb +++ b/spec/features/locations_spec.rb @@ -4,6 +4,8 @@ let(:lehman) { FactoryBot.create(:lehman) } let(:underbutler) { FactoryBot.create(:underbutler) } let(:butler) { underbutler.primary_location } + let(:duanereade) { FactoryBot.create(:duanereade, suppress_display: true) } + context 'when user without role logged in' do # initialize at least one library before { lehman } @@ -28,6 +30,11 @@ expect(page).to have_css("h2", text: underbutler.name) expect(page).to have_css("p", text: butler.name) end + + it "does not display suppressed locations", js: false do + visit("/locations/#{duanereade.code}") + expect(page.status_code).to eq(404) + end end context 'when administrator logged in' do @@ -84,6 +91,20 @@ end it "can update primary location" + + it "displays suppressed locations", js: false do + visit("/locations/#{duanereade.code}") + expect(page).to have_content("Public display of this location is suppressed.") + end + + it "can update suppress_display" do + expect(duanereade.suppress_display).to be true + visit edit_location_path(duanereade.code) + uncheck "Suppress" + click_on "Update Location" + expect(page).to have_content("Location successfully updated") + expect(duanereade.reload.suppress_display).to be false + end end context 'when manager logged in' do @@ -103,6 +124,11 @@ visit new_location_path expect(page).to have_content("Unauthorized") end + + it "displays suppressed locations", js: false do + visit("/locations/#{duanereade.code}") + expect(page).to have_content("Public display of this location is suppressed.") + end end context 'when editor logged in' do @@ -131,5 +157,10 @@ end it "cannot delete lehman" + + it "displays suppressed locations", js: false do + visit("/locations/#{duanereade.code}") + expect(page).to have_content("Public display of this location is suppressed.") + end end end From c0ae1c3e7a00bc6907b5d1de7f3b2e0ab29c8f92 Mon Sep 17 00:00:00 2001 From: Benjamin Armintor Date: Wed, 10 Jul 2024 23:27:45 -0400 Subject: [PATCH 3/7] Implement administrative interface for wifi configs and use them for density display - ensure all fk references are bigint - locations_controller permits admins to update location.access_points - location editing form generates a tree of checkboxes for access points in CUIT data and any previously configured APs - HOURS-128 - HOURS-131 --- app/controllers/locations_controller.rb | 13 ++- app/helpers/edit_location_helper.rb | 23 +++++ app/models/access_point.rb | 4 + app/models/access_points_location.rb | 4 + app/models/location.rb | 4 + app/views/locations/_location_form.html.erb | 53 +++++++---- ...0170615203023_add_fields_to_time_tables.rb | 2 +- ...0170627151458_change_library_table_name.rb | 2 +- .../20170814194528_add_primary_location.rb | 2 +- ...240710210750_add_location_access_points.rb | 17 ++++ db/schema.rb | 19 +++- lib/hours/wifi_density.rb | 95 +++++++++++++++++-- spec/hours/wifi_density_spec.rb | 11 ++- 13 files changed, 212 insertions(+), 37 deletions(-) create mode 100644 app/helpers/edit_location_helper.rb create mode 100644 app/models/access_point.rb create mode 100644 app/models/access_points_location.rb create mode 100644 db/migrate/20240710210750_add_location_access_points.rb diff --git a/app/controllers/locations_controller.rb b/app/controllers/locations_controller.rb index cb761f5..a49603b 100644 --- a/app/controllers/locations_controller.rb +++ b/app/controllers/locations_controller.rb @@ -9,9 +9,9 @@ def index path.blank? or path == '/' end if home_page - @locations = Location.where(front_page: true).where.not(code: 'all', suppress_display: true) + @locations = Location.where(front_page: true).where.not(code: 'all', suppress_display: true).includes(:access_points) else - @locations = Location.where.not(code: 'all', suppress_display: true) + @locations = Location.where.not(code: 'all', suppress_display: true).includes(:access_points) end @locations = @locations.order(:name) @now = Time.current @@ -44,7 +44,12 @@ def create def update authorize! :update, @location - if @location.update(update_params) + clean_params = update_params + clean_params.dig(:access_points)&.map! do |ap| + ap.present? ? AccessPoint.find_or_create_by(id: ap.to_i) : nil + end + clean_params.dig(:access_points)&.compact! + if @location.update(clean_params) flash[:success] = "Location successfully updated" redirect_to admin_url else @@ -136,7 +141,7 @@ def create_params def update_params if current_user.administrator? - params.require(:location).permit(:name, :code, :comment, :comment_two, :url, :summary, :primary, :primary_location_id, :front_page, :short_note, :short_note_url, :suppress_display) + params.require(:location).permit(:name, :code, :comment, :comment_two, :url, :summary, :primary, :primary_location_id, :front_page, :short_note, :short_note_url, :suppress_display, :wifi_connections_baseline, access_points: []) else params.require(:location).permit(:comment, :comment_two, :url, :summary, :short_note, :short_note_url) end diff --git a/app/helpers/edit_location_helper.rb b/app/helpers/edit_location_helper.rb new file mode 100644 index 0000000..3cb5cec --- /dev/null +++ b/app/helpers/edit_location_helper.rb @@ -0,0 +1,23 @@ +module EditLocationHelper + # + def access_point_checkboxes(access_point_branches, count_iterator, current_values = []) + content_tag(:ul) do + access_point_branches.map do |branch| + branch_tag = content_tag(:li) do + cb_index = count_iterator.next + content_tag(:label, for: "location_access_points_#{cb_index}") do + checked = current_values.include?(branch.id) + tag(:input, type: 'checkbox', value: branch.id, name: "location[access_points][]", id: "location_access_points_#{cb_index}", checked: checked).safe_concat("#{branch.name} #{branch.updated_at}") + end + end + if branch.children.size > 0 + child_tag = content_tag(:li) do + access_point_checkboxes(branch.children.values, count_iterator) + end + branch_tag.safe_concat child_tag + end + branch_tag + end.join.html_safe + end + end +end \ No newline at end of file diff --git a/app/models/access_point.rb b/app/models/access_point.rb new file mode 100644 index 0000000..ab1c3e0 --- /dev/null +++ b/app/models/access_point.rb @@ -0,0 +1,4 @@ +class AccessPoint < ApplicationRecord + has_many :access_points_locations + has_many :locations, through: :access_points_locations +end diff --git a/app/models/access_points_location.rb b/app/models/access_points_location.rb new file mode 100644 index 0000000..14e8a5a --- /dev/null +++ b/app/models/access_points_location.rb @@ -0,0 +1,4 @@ +class AccessPointsLocation < ApplicationRecord + belongs_to :location + belongs_to :access_point +end diff --git a/app/models/location.rb b/app/models/location.rb index 37f109d..83511ef 100644 --- a/app/models/location.rb +++ b/app/models/location.rb @@ -3,7 +3,11 @@ class Location < ApplicationRecord validate :primary_location_must_be_primary has_many :timetables + has_many :access_points_locations + has_many :access_points, through: :access_points_locations + belongs_to :primary_location, class_name: 'Location', foreign_key: 'primary_location_id', optional: true + accepts_nested_attributes_for :access_points # TODO: when a location is deleted any permissions related to that location should also be deleted diff --git a/app/views/locations/_location_form.html.erb b/app/views/locations/_location_form.html.erb index 2c31684..95e5835 100644 --- a/app/views/locations/_location_form.html.erb +++ b/app/views/locations/_location_form.html.erb @@ -19,28 +19,31 @@
- <%= f.label :suppress_display %> - <% if current_user.administrator? %> - <%= f.check_box :suppress_display %> - <% elsif @location.suppress_display %> -

<%= t("location.suppressed") %>

- <% end %> +

Location Display Options

+ <% if current_user.administrator? %> +
    +
  • <%= f.label(:suppress_display) { f.check_box(:suppress_display).safe_concat(" Suppress Display") } %>
  • +
  • <%= f.label(:front_page) { f.check_box(:front_page).safe_concat(" Front Page") } %>
  • +
  • <%= f.label(:primary) { f.check_box(:primary).safe_concat(" Primary") } %>
  • +
+
+ <%= f.label :primary_location %> + <%= collection_select(:location, :primary_location_id, Location.where(primary: true), :id, :name, {include_blank: 'None'}) %> +
+ <% else %> +
    + <% if @location.suppress_display %> +
  • <%= t("location.suppressed") %>
  • + <% end %> +
  • Front Page: <%= (!!@location.front_page).to_s %>
  • +
  • Primary: <%= (!!@location.primary).to_s %>
  • + <% if @location.primary_location %> +
  • Primary Location: <%= @location.primary_location.name %>
  • + <% end %> +
+ <% end %>
- <% if current_user.administrator? %> -
- <%= f.label :front_page %> - <%= f.check_box :front_page %> -
-
- <%= f.label :primary %> - <%= f.check_box :primary %> -
-
- <%= f.label :primary_location %> - <%= collection_select(:location, :primary_location_id, Location.where(primary: true), :id, :name, {include_blank: 'None'}) %> -
- <% end %>
<%= f.label :url %> <%= f.text_field :url, :class => "form-control" %> @@ -66,6 +69,16 @@ <%= f.text_field :short_note_url, :class => "form-control" %>
+ +
+

WiFi Configuration

+
+ <%= f.label :wifi_connections_baseline, 'WiFi Connections Baseline Count' %> + <%= f.text_field :wifi_connections_baseline, :class => "form-control" %> +
+ <% current_aps = @location.access_point_ids.map(&:to_s) %> + <%= access_point_checkboxes(Hours::WifiDensity.access_point_trees(@location), (1...).each, current_aps) %> +
<%= f.submit :class => 'btn btn-primary btn-sm'%> <% end %>
diff --git a/db/migrate/20170615203023_add_fields_to_time_tables.rb b/db/migrate/20170615203023_add_fields_to_time_tables.rb index 894187c..343230d 100644 --- a/db/migrate/20170615203023_add_fields_to_time_tables.rb +++ b/db/migrate/20170615203023_add_fields_to_time_tables.rb @@ -5,7 +5,7 @@ def change add_column :time_tables, :note, :string remove_index :time_tables, column: [:code, :date] remove_column :time_tables, :code - add_reference :time_tables, :library, index: true, foreign_key: true + add_reference :time_tables, :library, index: true, foreign_key: true, type: :bigint add_index :time_tables, [:library_id, :date], :unique => true rename_table :time_tables, :timetables end diff --git a/db/migrate/20170627151458_change_library_table_name.rb b/db/migrate/20170627151458_change_library_table_name.rb index fb35ac9..c12d14e 100644 --- a/db/migrate/20170627151458_change_library_table_name.rb +++ b/db/migrate/20170627151458_change_library_table_name.rb @@ -3,7 +3,7 @@ def change remove_index :timetables, name: :index_timetables_on_library_id_and_date remove_reference :timetables, :library, index: true, foreign_key: true rename_table :libraries, :locations - add_reference :timetables, :location, index: true, foreign_key: true + add_reference :timetables, :location, index: true, foreign_key: true, type: :bigint add_index :timetables, [:location_id, :date], :unique => true end end diff --git a/db/migrate/20170814194528_add_primary_location.rb b/db/migrate/20170814194528_add_primary_location.rb index 2cf9685..58c320d 100644 --- a/db/migrate/20170814194528_add_primary_location.rb +++ b/db/migrate/20170814194528_add_primary_location.rb @@ -1,6 +1,6 @@ class AddPrimaryLocation < ActiveRecord::Migration[5.1] def up - add_reference :locations, :primary_location, foreign_key: {to_table: :locations} + add_reference :locations, :primary_location, type: :bigint, foreign_key: {to_table: :locations} add_column :locations, :primary, :boolean, default: false add_index :locations, :primary end diff --git a/db/migrate/20240710210750_add_location_access_points.rb b/db/migrate/20240710210750_add_location_access_points.rb new file mode 100644 index 0000000..4831140 --- /dev/null +++ b/db/migrate/20240710210750_add_location_access_points.rb @@ -0,0 +1,17 @@ +class AddLocationAccessPoints < ActiveRecord::Migration[6.0] + def change + create_table :access_points do |t| + t.integer :client_count + t.timestamps null: false + end + create_table :access_points_locations do |t| + t.timestamps null: false + t.bigint :location_id, null: false + t.bigint :access_point_id, null: false + t.index :location_id + t.index :access_point_id + t.index [:location_id, :access_point_id], unique: true + end + add_column :locations, :wifi_connections_baseline, :int, null: true + end +end diff --git a/db/schema.rb b/db/schema.rb index fce2d8b..b060934 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,23 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_07_10_150406) do +ActiveRecord::Schema[7.0].define(version: 2024_07_10_210750) do + create_table "access_points", force: :cascade do |t| + t.integer "client_count" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + create_table "access_points_locations", force: :cascade do |t| + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.bigint "location_id", null: false + t.bigint "access_point_id", null: false + t.index ["access_point_id"], name: "index_access_points_locations_on_access_point_id" + t.index ["location_id", "access_point_id"], name: "index_access_points_locations_on_location_id_and_access_point_id", unique: true + t.index ["location_id"], name: "index_access_points_locations_on_location_id" + end + create_table "locations", force: :cascade do |t| t.string "name", null: false t.string "code", null: false @@ -26,6 +42,7 @@ t.string "short_note" t.text "short_note_url" t.boolean "suppress_display", default: false + t.integer "wifi_connections_baseline" t.index ["code"], name: "index_locations_on_code" t.index ["front_page"], name: "index_locations_on_front_page" t.index ["primary"], name: "index_locations_on_primary" diff --git a/lib/hours/wifi_density.rb b/lib/hours/wifi_density.rb index f969de1..b361e11 100644 --- a/lib/hours/wifi_density.rb +++ b/lib/hours/wifi_density.rb @@ -11,15 +11,13 @@ def self.data_url def self.percentage_for(location) # If this location's code is not mapped in the wifi density config file, # that means wifi density info is not supported for the location. - config_for_location = config_for_location_code(location.code) + config_for_location = location_wifi_configuration(location) return nil if config_for_location.nil? || !config_for_location.key?(:high) || !config_for_location.key?(:cuit_location_ids) Rails.cache.fetch('wifi_density_data-' + location.code, expires_in: wifi_data_cache_duration) do - wifi_density_data_for_locations = Rails.cache.fetch('wifi_density_data', expires_in: wifi_data_cache_duration) do - fetch_hierarchical_wifi_density_data - end + wifi_density_data_for_locations = hierarchical_wifi_density_data aggregated_locations = {} - config_for_location[:cuit_location_ids].each do |cuit_location_id| + config_for_location[:cuit_location_ids]&.each do |cuit_location_id| location_data = wifi_density_data_for_locations[cuit_location_id.to_s] next if location_data.nil? aggregated_locations.merge!({cuit_location_id.to_s => location_data}) @@ -52,20 +50,54 @@ def self.percentage_for(location) end end + def self.location_wifi_configuration(location) + { + high: (location.wifi_connections_baseline.to_i if location.wifi_connections_baseline.present?), + cuit_location_ids: (location.access_point_ids.map(&:to_s) if location.access_point_ids.present?) + } + end + + # Obsolete def self.config_for_location_code(location_code) WIFI_DENSITY[:locations][location_code.to_sym] end + def self.raw_wifi_density_data + Rails.cache.fetch('raw_wifi_density_data', expires_in: wifi_data_cache_duration) do + cacheable_data = fetch_raw_wifi_density_data + now = DateTime.now + cacheable_data.each do |ap_id, location_data| + ap = AccessPoint.find_or_create_by(id: ap_id) + if ap.client_count != location_data['client_count'].to_i + ap.update(client_count: location_data['client_count'].to_i) + location_data['updated_at'] = now + else + location_data['updated_at'] = ap.updated_at + end + end + cacheable_data + end + end + def self.fetch_raw_wifi_density_data - if Rails.env.development? + if Rails.env.development? || Rails.env.test? JSON.parse(File.read(Rails.root.join('spec/fixtures/files/sample-wifi-density-response.json'))) else JSON.parse(Net::HTTP.get(URI(data_url))) end end + def self.hierarchical_wifi_density_data + Rails.cache.fetch('hierarchical_wifi_density_data', expires_in: wifi_data_cache_duration) do + fetch_hierarchical_wifi_density_data + end + end + def self.fetch_hierarchical_wifi_density_data - all_location_data = fetch_raw_wifi_density_data + all_location_data = raw_wifi_density_data + + stale_time = DateTime.now - 2.hours + all_location_data.reject! { |k, v| v['updated_at'] < stale_time } # Convert raw data into hierarchical data, generating top level entries # for any referenced parent_id values that don't exist. @@ -90,6 +122,8 @@ def self.fetch_hierarchical_wifi_density_data all_location_data end + # add child access point nodes to their parent's children hash + # but keep them in the top-level hash def self.recursively_collect_children(location_data, children = {}) if location_data['children'] location_data['children'].each do |child_location_id, child_location_data| @@ -104,5 +138,52 @@ def self.recursively_collect_children(location_data, children = {}) end children end + + def self.default_access_point_data_for_id(ap_id, suffix = "") + { 'id' => ap_id, 'name' => "Location #{ap_id}#{suffix}" } + end + + def self.access_point_trees(location = nil) + stored_ids = location ? location.access_point_ids.to_a : [] + aggregated_locations = raw_wifi_density_data.reduce({}) do |acc, entry| + ap_id, ap_data = *entry + acc[ap_id] ||= default_access_point_data_for_id(ap_id) + acc[ap_id].merge!(ap_data) + if parent_id = ap_data['parent_id'] + acc[parent_id] ||= default_access_point_data_for_id(parent_id) + end + acc + end + + roots = rollup_children(aggregated_locations).transform_values(&:deep_symbolize_keys) + stored_ids.each { |id| roots[id.to_s] ||= default_access_point_data_for_id(id, " [no current data]").symbolize_keys } + roots.map { |k,v| AccessPointStruct.new(**v) } + end + + # add child access point nodes to their parent's children hash + # and remove them from the top-level hash + def self.rollup_children(data = {}) + parent_ids = data.values.map { |v| v['parent_id'] }.compact + return data if parent_ids.length == 0 + + (data.keys - parent_ids).each do |leaf_id| + leaf = data[leaf_id] + next unless parent_id = leaf['parent_id'] + parent = data[parent_id] ||= default_access_point_data_for_id(parent_id) + (parent['children'] ||= {})[leaf_id] = data.delete(leaf_id) + end + rollup_children(data) + end + + class AccessPointStruct + attr_accessor :children, :id, :name, :parent_id, :updated_at + def initialize(children: nil, id:, name:, parent_id: nil, updated_at: nil, **args) + @id = id + @name = name + @parent_id = parent_id + @updated_at = updated_at + @children = (children || {}).transform_values { |c| AccessPointStruct.new(**c) } + end + end end end diff --git a/spec/hours/wifi_density_spec.rb b/spec/hours/wifi_density_spec.rb index 990cca0..813bb61 100644 --- a/spec/hours/wifi_density_spec.rb +++ b/spec/hours/wifi_density_spec.rb @@ -10,7 +10,7 @@ context ".percentage_for" do let(:butler_location) { - Location.new(code: 'butler-24') + Location.new(code: 'butler-24', wifi_connections_baseline: 1900, access_points: [AccessPoint.new(id: '115')]) } let(:journalism_location) { Location.new(code: 'journalism') @@ -50,8 +50,15 @@ end context ".fetch_hierarchical_wifi_density_data" do + let(:hierarchical_wifi_density_data) { described_class.fetch_hierarchical_wifi_density_data } + before do + hierarchical_wifi_density_data.each do |k, v| + v.delete('updated_at') + v['children']&.each {|v2| v2.delete('updated_at')} + end + end it "returns a hierarchical version of the raw wifi data with child locations nested under parents" do - expect(described_class.fetch_hierarchical_wifi_density_data).to eq(sample_hierarchical_wifi_density_data) + expect(hierarchical_wifi_density_data).to eq(sample_hierarchical_wifi_density_data) end end From e6e052792cc9a7e808928fb1f44cca728f2a9e62 Mon Sep 17 00:00:00 2001 From: Benjamin Armintor Date: Thu, 11 Jul 2024 14:59:28 -0400 Subject: [PATCH 4/7] remove obsolete template config for location in wifi_density (HOURS-131) --- config/wifi_density.template.yml | 66 -------------------------------- 1 file changed, 66 deletions(-) diff --git a/config/wifi_density.template.yml b/config/wifi_density.template.yml index f6f71e2..a6481e2 100644 --- a/config/wifi_density.template.yml +++ b/config/wifi_density.template.yml @@ -1,73 +1,7 @@ development: data_url: https://hours.library.columbia.edu/wifi-density.json data_cache_duration: <%= 10.minutes %> - locations: - avery: - cuit_location_ids: - - 124 - high: 100 - business: - cuit_location_ids: - - 96 - high: 100 - butler-24: - cuit_location_ids: - - 115 - high: 1900 - eastasian: - cuit_location_ids: - - 98 - high: 300 - lehman: - cuit_location_ids: - - 109 - high: 1300 - math: - cuit_location_ids: - - 141 - high: 600 - science-engineering: - cuit_location_ids: - - 100 - high: 240 - social-work: - cuit_location_ids: - - 123 - high: 100 test: data_url: https://hours.library.columbia.edu/wifi-density.json data_cache_duration: <%= 1.second %> - locations: - avery: - cuit_location_ids: - - 124 - high: 100 - business: - cuit_location_ids: - - 96 - high: 100 - butler-24: - cuit_location_ids: - - 115 - high: 1900 - eastasian: - cuit_location_ids: - - 98 - high: 300 - lehman: - cuit_location_ids: - - 109 - high: 1300 - math: - cuit_location_ids: - - 141 - high: 600 - science-engineering: - cuit_location_ids: - - 100 - high: 240 - social-work: - cuit_location_ids: - - 123 - high: 100 From f8706f6538184843cdbe03f8e3990ac610cd50db Mon Sep 17 00:00:00 2001 From: Benjamin Armintor Date: Thu, 11 Jul 2024 15:01:07 -0400 Subject: [PATCH 5/7] remove obsolete config parsing method in wifi_density (HOURS-131) --- lib/hours/wifi_density.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/hours/wifi_density.rb b/lib/hours/wifi_density.rb index b361e11..7628e17 100644 --- a/lib/hours/wifi_density.rb +++ b/lib/hours/wifi_density.rb @@ -57,11 +57,6 @@ def self.location_wifi_configuration(location) } end - # Obsolete - def self.config_for_location_code(location_code) - WIFI_DENSITY[:locations][location_code.to_sym] - end - def self.raw_wifi_density_data Rails.cache.fetch('raw_wifi_density_data', expires_in: wifi_data_cache_duration) do cacheable_data = fetch_raw_wifi_density_data From b3242be87849f2c0a882a2ee575fa190d549cfbe Mon Sep 17 00:00:00 2001 From: Benjamin Armintor Date: Fri, 12 Jul 2024 09:32:17 -0400 Subject: [PATCH 6/7] use translated key for suppressed display message consistently - spec 404 text when suppressed calendar is accessed publicly - HOURS-128 --- app/views/locations/show.html.erb | 2 +- spec/features/locations_spec.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/views/locations/show.html.erb b/app/views/locations/show.html.erb index b6da047..4f21bfc 100644 --- a/app/views/locations/show.html.erb +++ b/app/views/locations/show.html.erb @@ -5,7 +5,7 @@

<%= link_to @location.name, @location.url %>

<% if @location.suppress_display %> -

Public display of this location is suppressed.

+

<%= t("location.suppressed") %>

<% end %> <% if @location.primary_location %>

Check hours for: <%= link_to @location.primary_location.name, location_path(@location.primary_location.code) %>

diff --git a/spec/features/locations_spec.rb b/spec/features/locations_spec.rb index dde090e..d5104a5 100644 --- a/spec/features/locations_spec.rb +++ b/spec/features/locations_spec.rb @@ -34,6 +34,7 @@ it "does not display suppressed locations", js: false do visit("/locations/#{duanereade.code}") expect(page.status_code).to eq(404) + expect(page).to have_content("Location not found") end end From 0d4cee5c8a5ea0bb90ace66ea9c5bad05f50449c Mon Sep 17 00:00:00 2001 From: Benjamin Armintor Date: Fri, 12 Jul 2024 11:52:01 -0400 Subject: [PATCH 7/7] bootstrap classes on location edit form for readability (HOURS-128) --- app/helpers/edit_location_helper.rb | 10 +++++++--- app/views/locations/_location_form.html.erb | 6 +++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/helpers/edit_location_helper.rb b/app/helpers/edit_location_helper.rb index 3cb5cec..da6df0d 100644 --- a/app/helpers/edit_location_helper.rb +++ b/app/helpers/edit_location_helper.rb @@ -3,11 +3,15 @@ module EditLocationHelper def access_point_checkboxes(access_point_branches, count_iterator, current_values = []) content_tag(:ul) do access_point_branches.map do |branch| - branch_tag = content_tag(:li) do + branch_tag = content_tag(:li, class: ['form-check']) do cb_index = count_iterator.next - content_tag(:label, for: "location_access_points_#{cb_index}") do + content_tag(:label, for: "location_access_points_#{cb_index}", class: ['form-check-label']) do checked = current_values.include?(branch.id) - tag(:input, type: 'checkbox', value: branch.id, name: "location[access_points][]", id: "location_access_points_#{cb_index}", checked: checked).safe_concat("#{branch.name} #{branch.updated_at}") + cb_attrs = { + type: 'checkbox', value: branch.id, name: "location[access_points][]", + id: "location_access_points_#{cb_index}", checked: checked, class: ['form-check-input'] + } + tag(:input, cb_attrs).safe_concat("#{branch.name}").safe_concat(content_tag(:em, branch.updated_at ? "updated: #{branch.updated_at}" : "aggregate data", class: "ml-1")) end end if branch.children.size > 0 diff --git a/app/views/locations/_location_form.html.erb b/app/views/locations/_location_form.html.erb index 95e5835..8746475 100644 --- a/app/views/locations/_location_form.html.erb +++ b/app/views/locations/_location_form.html.erb @@ -22,9 +22,9 @@

Location Display Options

<% if current_user.administrator? %>
    -
  • <%= f.label(:suppress_display) { f.check_box(:suppress_display).safe_concat(" Suppress Display") } %>
  • -
  • <%= f.label(:front_page) { f.check_box(:front_page).safe_concat(" Front Page") } %>
  • -
  • <%= f.label(:primary) { f.check_box(:primary).safe_concat(" Primary") } %>
  • +
  • <%= f.label(:suppress_display, class: ['form-check-label']) { f.check_box(:suppress_display, class: ['form-check-input']).safe_concat("Suppress Display") } %>
  • +
  • <%= f.label(:front_page, class: ['form-check-label']) { f.check_box(:front_page, class: ['form-check-input']).safe_concat("Front Page") } %>
  • +
  • <%= f.label(:primary, class: ['form-check-label']) { f.check_box(:primary, class: ['form-check-input']).safe_concat("Primary") } %>
<%= f.label :primary_location %>