Changed name dinsdag kring and remove woensdag kring #438
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #438 +/- ##
========================================
Coverage 99.92% 99.92%
========================================
Files 197 197
Lines 2658 2665 +7
========================================
+ Hits 2656 2663 +7
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lodewiges
changed the title
|
This PR will need to be merged at the same time the accompanying pr on amber/ui is getting merged. I could also be writen in such a way that there is no need for this however this requires 2 PR and manual changing all activities instead of using a migration |
|
I think you need to write csvalpha/amber-ui#876 rather than https://github.com/csvalpha/amber-ui/#876 but on top of that, you probably meant csvalpha/amber-ui#885 instead.
That sounds like a not so amazing outcome if that happens. I have not taken the time to determine how the ical links work, so I will postpone reviewing this PR until I have the time and energy to figure that out |
This is an example link from icalender feature on the site you can see the "dinsdagkring,woensdagkring" part of the link, this will be replaced by "kring" with this PR. This is because the names in the link correspond with the names your able to select. This is why most likely all links are going to break. I personally think that we should notify all members in advance and update it. |
bump? |
|
okay, so i did some more research this will not break all links, if people dont update the link. the only affect is that kring doesnt showup anymore |
I agree.
That's good to hear. But it's still important to notify people. This whole thing makes me wonder: is there an easy way to prevent people from having to update their ical links everytime we change a category? Ideally, we would have a situation where we give the user a single link, https://csvalpha.nl/ical/activities?key=privatestuff&user_id=1 |
In fact, we would even be able to leave the current ical links as they are; simply ignore the |
|
you are correct it would be the best option, i think we should made a issue so we can do this in the future |
made one: #483 |
|
I will implement those changes in this PR, this makes sure no links break TO DO
|
|
i started adding #483 into this PR, I am however getting some difficulties be cause the categories that the users has selected are not stored on the server as far as I know |
|
THis has more prio |
|
Caution Review failedThe pull request is closed. WalkthroughThe PR introduces persistent iCal category preferences by adding an Changes
Sequence DiagramsequenceDiagram
participant Client
participant ActivityController
participant User
participant Database
Client->>ActivityController: GET /ical?categories=kring,agora&key=...&user_id=X
ActivityController->>Database: Fetch User by ID
Database-->>ActivityController: User data
alt User has stored ical_categories
ActivityController->>ActivityController: Use `@user.ical_categories`
else First time or empty stored categories
ActivityController->>ActivityController: permitted = requested ∩ Activity.categories
alt permitted set not empty
ActivityController->>Database: Store permitted to user.ical_categories
Database-->>ActivityController: Saved
else permitted set empty
ActivityController->>ActivityController: permitted = Activity.categories (fallback)
end
end
ActivityController->>Database: Fetch Activities filtered by permitted categories
Database-->>ActivityController: Activities
ActivityController-->>Client: iCal feed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/controllers/v1/activities_controller.rb(1 hunks)app/controllers/v1/users_controller.rb(1 hunks)app/models/activity.rb(1 hunks)app/resources/v1/user_resource.rb(3 hunks)db/migrate/20241113104056_simplyfing_calender_options.rb(1 hunks)db/schema.rb(1 hunks)spec/factories/activities.rb(1 hunks)spec/models/activity_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/resources/v1/user_resource.rb (1)
app/policies/user_policy.rb (1)
me?(58-60)
🪛 GitHub Actions: Continuous Integration
app/resources/v1/user_resource.rb
[error] 8-8: SyntaxError: unexpected ',', likely missing a trailing comma on the previous line in attributes list (V1::UserResource).
🔇 Additional comments (6)
app/models/activity.rb (1)
36-39: LGTM: Category consolidation implemented correctly.The streamlined category list properly replaces the separate "dinsdagkring" and "woensdagkring" entries with a single "kring" category, aligning with the PR objectives and migration logic.
db/schema.rb (1)
563-563: LGTM: Schema change reflects migration correctly.The new
ical_categoriescolumn is properly defined as a string array with an appropriate default value.app/controllers/v1/users_controller.rb (1)
109-113: LGTM: Properly excludes ical_categories from batch import responses.Adding
ical_categoriesto the exclusion list is consistent with the existing pattern for user-specific configuration fields.spec/factories/activities.rb (1)
12-15: LGTM: Test factory updated to match new categories.The factory correctly reflects the streamlined category list from the Activity model.
spec/models/activity_spec.rb (1)
179-187: LGTM: Test data updated for new category set.The test correctly uses the streamlined category list, ensuring proper coverage of the
humanized_categorymethod.app/resources/v1/user_resource.rb (1)
78-84: Verify: Should ical_secret_key be user-creatable?Adding
ical_secret_keytocreatable_fieldsallows users to set their own iCal secret keys. Typically, secret keys should be auto-generated by the system for security reasons to ensure sufficient entropy and uniqueness.Please verify this is the intended behavior. If users need to regenerate their keys, consider providing a separate "regenerate" endpoint instead of allowing arbitrary values.
If this was added unintentionally, apply this diff:
if me?(context) attributes += %i[otp_required password user_details_sharing_preference allow_sofia_sharing picture_publication_preference info_in_almanak - ifes_data_sharing_preference ical_secret_key sidekiq_access + ifes_data_sharing_preference sidekiq_access setup_complete] end
| @@ -0,0 +1,19 @@ | |||
| class SimplyfingCalenderOptions < ActiveRecord::Migration[7.0] | |||
There was a problem hiding this comment.
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.
| 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.
| 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 |
There was a problem hiding this comment.
🛠️ 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.
| 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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/resources/v1/user_resource.rb (2)
54-54: Remove extra blank line.This blank line was flagged in the previous review and should be removed for consistency.
Apply this diff:
- allowed_keys += %i[ical_secret_key ical_categories] if me?
60-60: Remove duplicate ical_secret_key.The previous review flagged that
:ical_secret_keyis added toallowed_keyson line 55 whenme?is true, and again here in theupdate_or_me?block. Sinceme?is a subset ofupdate_or_me?(line 153 confirms this), this creates redundant entries.Apply this diff:
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 ical_secret_key + user_details_sharing_preference allow_sofia_sharing sidekiq_access setup_complete]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/resources/v1/user_resource.rb(3 hunks)db/schema.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- db/schema.rb
🧰 Additional context used
🧬 Code graph analysis (1)
app/resources/v1/user_resource.rb (1)
app/policies/user_policy.rb (1)
me?(58-60)
🪛 GitHub Check: Lint
app/resources/v1/user_resource.rb
[failure] 82-82:
[Correctable] Layout/TrailingWhitespace: Trailing whitespace detected. (https://rubystyle.guide#no-trailing-whitespace)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (2)
app/resources/v1/user_resource.rb (2)
7-7: Syntax error fixed - comma now present.The missing comma after
:ical_categoriesthat was flagged in the previous review has been corrected.
55-55: LGTM - ical_categories correctly exposed for own data.The addition of
:ical_categoriesalongside:ical_secret_keywhenme?is true properly exposes the new calendar categories field when users view their own data.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/v1/activities_controller.rb (1)
43-43: Inconsistent behavior: birthdays uses URL parameters while activities use stored preferences.Line 43 calls
ical_add_birthdays?(requested_categories), which checks the URL parameter, while activity categories usestored_categories(line 35). This creates inconsistent behavior:Example:
- User has stored categories:
["kring", "algemeen"]- User changes URL to:
?categories=societeit(without "birthdays")- Result: Gets
kring+algemeenactivities (from stored), but birthdays inclusion changes based on URLFor consistency, birthdays should also use stored preferences, or both should respect URL parameters.
Option 1: Store birthdays preference with categories
# In the storage logic if stored_categories.empty? && requested_categories.present? # Store whether birthdays was included @user.update( ical_categories: requested_categories & Activity.categories, ical_include_birthdays: requested_categories.include?('birthdays') ) end # Later if @user.ical_include_birthdays # add birthdays endOption 2: Always respect URL parameters for both
if requested_categories.present? permitted_categories = requested_categories & Activity.categories else permitted_categories = @user.ical_categories & Activity.categories end # birthdays also respects URL or stored if ical_add_birthdays?(requested_categories || @user.ical_categories) # add birthdays end
🧹 Nitpick comments (1)
app/controllers/v1/activities_controller.rb (1)
25-25: Remove extra blank lines for consistency.Lines 25 and 38 have extra blank lines that don't add to readability.
Apply this diff:
stored_categories = @user.ical_categories - requested_categories = params[:categories]&.split(',')permitted_categories = Activity.categories if permitted_categories.empty? - activities_for_ical(permitted_categories).each do |act|Also applies to: 38-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/v1/activities_controller.rb(1 hunks)app/resources/v1/user_resource.rb(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/resources/v1/user_resource.rb (1)
app/policies/user_policy.rb (1)
me?(58-60)
app/controllers/v1/activities_controller.rb (1)
app/models/activity.rb (1)
categories(36-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Lint
🔇 Additional comments (1)
app/resources/v1/user_resource.rb (1)
7-7: No action required—ical_categoriesis properly updatable.The parent class
ApplicationResourcedefinesupdatable_fields(context)to returncreatable_fields(context). Sinceical_categoriesis included increatable_fieldswhenme?(context), it is automatically updatable. The codebase confirms this pattern is working:app/controllers/v1/activities_controller.rbline 32 actively updates the field with@user.update(ical_categories: ...).
required for csvalpha/amber-ui#885, they depend on eachother
fixes #437
fixes #483
Tested it by emulating a calender request event through api
i did not test if requessting a no existing category does something but that should work
Summary by CodeRabbit
Release Notes
New Features
Changes