Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/v1/activities_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def generate_alias
def ical # rubocop:disable Metrics/AbcSize, Metrics/MethodLength
return head :unauthorized unless authenticate_user_by_ical_secret_key

requested_categories = params[:categories].try(:split, ',')
requested_categories = User.find_by(id: params[:user_id]).ical_categories

permitted_categories = (requested_categories & Activity.categories) ||
Activity.categories
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/v1/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def context
def excluded_display_properties
%i[created_at updated_at deleted_at activated_at archived_at password_digest activation_token
avatar activation_token_valid_till setup_complete otp_secret_key otp_required
ical_secret_key id]
ical_secret_key ical_categories id]
end

def otp_already_required_error
Expand Down
4 changes: 2 additions & 2 deletions app/models/activity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ class Activity < ApplicationRecord
after_save :copy_author_and_group_to_form!

def self.categories
%w[algemeen societeit vorming dinsdagkring woensdagkring
choose ifes ozon disputen kiemgroepen huizen extern eerstejaars curiositates]
%w[algemeen societeit vorming kring
choose ifes ozon disputen genootschapen huizen extern eerstejaars]
end

def full_day?
Expand Down
10 changes: 6 additions & 4 deletions app/resources/v1/user_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
:ifes_data_sharing_preference, :info_in_almanak, :almanak_subscription_preference,
:digtus_subscription_preference, :email, :birthday, :address, :postcode, :city,
:phone_number, :food_preferences, :vegetarian, :study, :start_study,
:picture_publication_preference, :ical_secret_key,
:picture_publication_preference, :ical_secret_key, :ical_categories,
:password, :avatar, :avatar_url, :avatar_thumb_url,
:user_details_sharing_preference, :allow_sofia_sharing, :trailer_drivers_license,
:sidekiq_access, :setup_complete
Expand Down Expand Up @@ -51,12 +51,13 @@
# Relationships
allowed_keys += %i[groups active_groups memberships mail_aliases mandates
group_mail_aliases permissions photos user_permissions]
allowed_keys += %i[ical_secret_key] if me?

allowed_keys += %i[ical_secret_key ical_categories] if me?
if update_or_me?
allowed_keys += %i[login_enabled otp_required activated_at emergency_contact
emergency_number ifes_data_sharing_preference info_in_almanak
almanak_subscription_preference digtus_subscription_preference
user_details_sharing_preference allow_sofia_sharing
user_details_sharing_preference allow_sofia_sharing ical_secret_key
sidekiq_access setup_complete]
end
allowed_keys += %i[picture_publication_preference] if read_or_me?
Expand All @@ -78,7 +79,8 @@
attributes += %i[otp_required password
user_details_sharing_preference allow_sofia_sharing
picture_publication_preference info_in_almanak
ifes_data_sharing_preference sidekiq_access setup_complete]
ifes_data_sharing_preference ical_secret_key sidekiq_access

Check failure on line 82 in app/resources/v1/user_resource.rb

View workflow job for this annotation

GitHub Actions / Lint

[Correctable] Layout/TrailingWhitespace: Trailing whitespace detected. (https://rubystyle.guide#no-trailing-whitespace)
setup_complete]
end

if user_can_create_or_update?(context)
Expand Down
19 changes: 19 additions & 0 deletions db/migrate/20241113104056_simplyfing_calender_options.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
class SimplyfingCalenderOptions < ActiveRecord::Migration[7.0]
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix typos in migration class name.

The class name contains typos: "Simplyfing" should be "Simplifying" and "Calender" should be "Calendar".

Apply this diff:

-class SimplyfingCalenderOptions < ActiveRecord::Migration[7.0]
+class SimplifyingCalendarOptions < ActiveRecord::Migration[7.0]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class SimplyfingCalenderOptions < ActiveRecord::Migration[7.0]
class SimplifyingCalendarOptions < ActiveRecord::Migration[7.0]
🤖 Prompt for AI Agents
In db/migrate/20241113104056_simplyfing_calender_options.rb around line 1, the
migration class name contains typos ("Simplyfing" -> "Simplifying" and
"Calender" -> "Calendar"); rename the class to match Rails migration naming
conventions (class SimplifyingCalendarOptions < ActiveRecord::Migration[7.0])
and update the filename if you want it to reflect the corrected class name, then
run rails db:migrate:status or the test suite to ensure the migration loads with
the corrected class name.

def up
add_column :users, :ical_categories, :string, array: true, default: []
Activity.where(category: 'dinsdagkring').find_each do |activity|
activity.update(category: 'kring')
end

Activity.where(category: 'woensdagkring').find_each do |activity|
activity.update(category: 'kring')
end
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Use update_all for better performance and avoid validation issues.

Using .update in migrations has several drawbacks:

  • Triggers validations and callbacks, which can fail unexpectedly during migration
  • Significantly slower for large datasets due to individual updates
  • Can cause issues if model validations change over time

Apply this diff to use update_all instead:

-    Activity.where(category: 'dinsdagkring').find_each do |activity|
-      activity.update(category: 'kring')
-    end
-
-    Activity.where(category: 'woensdagkring').find_each do |activity|
-      activity.update(category: 'kring')
-    end
+    Activity.where(category: 'dinsdagkring').update_all(category: 'kring')
+    Activity.where(category: 'woensdagkring').update_all(category: 'kring')
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Activity.where(category: 'dinsdagkring').find_each do |activity|
activity.update(category: 'kring')
end
Activity.where(category: 'woensdagkring').find_each do |activity|
activity.update(category: 'kring')
end
Activity.where(category: 'dinsdagkring').update_all(category: 'kring')
Activity.where(category: 'woensdagkring').update_all(category: 'kring')
🤖 Prompt for AI Agents
In db/migrate/20241113104056_simplyfing_calender_options.rb around lines 4 to
10, the migration uses Activity.find_each with activity.update which triggers
validations/callbacks and is slow; replace those per-record updates with a
single relation-level update_all call (e.g., Activity.where(category:
'dinsdagkring').update_all(category: 'kring') and similarly for 'woensdagkring')
so the change runs in a single SQL UPDATE and bypasses model
validations/callbacks.

end

def down
remove_column :users, :ical_categories
Activity.where(category: 'kring').find_each do |activity|
activity.update(category: 'dinsdagkring')
end
end
end
1 change: 1 addition & 0 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,7 @@
t.string "nickname"
t.boolean "trailer_drivers_license", default: false, null: false
t.boolean "setup_complete", default: false, null: false
t.string "ical_categories", default: [], array: true
t.index ["deleted_at"], name: "index_users_on_deleted_at"
t.index ["email"], name: "index_users_on_email", unique: true
t.index ["login_enabled"], name: "index_users_on_login_enabled"
Expand Down
4 changes: 2 additions & 2 deletions spec/factories/activities.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
start_time { Faker::Time.between(from: 1.day.ago, to: Time.zone.today) }
end_time { Faker::Time.between(from: 1.day.from_now, to: 2.days.from_now) }
category do
%w[algemeen societeit vorming dinsdagkring woensdagkring
choose ifes ozon disputen kiemgroepen huizen extern eerstejaars curiositates].sample
%w[algemeen societeit vorming kring
choose ifes ozon disputen genootschapen huizen extern eerstejaars].sample
end
publicly_visible { false }

Expand Down
4 changes: 2 additions & 2 deletions spec/models/activity_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,8 @@
context 'when it is another category' do
let(:record) do
build_stubbed(:activity,
category: %w[algemeen sociëteit vorming dinsdagkring woensdagkring
disputen kiemgroepen huizen extern curiositates].sample)
category: %w[algemeen sociëteit vorming kring
disputen genootschapen huizen extern].sample)
end

it { expect(record.humanized_category).to eq record.category.capitalize }
Expand Down
Loading