Cleaning the categories for alpha agenda #885
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #885 +/- ##
===========================================
- Coverage 13.05% 13.02% -0.03%
===========================================
Files 450 450
Lines 3117 3124 +7
===========================================
Hits 407 407
- Misses 2710 2717 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lodewiges
changed the title
|
can you ask someone else to review this? Maybe Maarten or Jesse? |
|
nvm I have already left my thoughts on csvalpha/amber-api#438 |
DrumsnChocolate
left a comment
DrumsnChocolate
left a comment
There was a problem hiding this comment.
If we decide not to follow the route of what I'm suggesting in csvalpha/amber-api#438 (comment), then this is approved
WalkthroughConsolidated and renamed activity categories in constants and fixtures, added Changes
Sequence DiagramsequenceDiagram
participant User
participant Template as "iCal Template"
participant Controller as "ActivitiesIcalController"
participant Model as "currentUser"
participant API as "Backend"
User->>Template: toggle category checkboxes
Note over Template: selection tracked in controller state
User->>Template: click "Opslaan"
Template->>Controller: invoke saveCategories()
Controller->>Model: set icalCategories = [selectedValues]
Controller->>API: persist currentUser
alt success
API-->>Controller: success
Controller->>Template: show confirmation
else failure
API-->>Controller: error
Controller->>Template: show error
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
app/controllers/activities/ical.js (1)
1-1: Reconsider extending EditController.The
ActivitiesIcalControllernow extendsEditController, which providessubmit/cancelactions, flash notices, andModelSaveUtildesigned for form editing workflows. However, this controller displays iCal settings and uses a customsaveCategoriesmethod that doesn't leverage the inherited functionality. Consider whether extendingControlleris more appropriate, or clarify why the EditController's features are needed.Also applies to: 8-8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/constants.js(1 hunks)app/controllers/activities/ical.js(3 hunks)app/models/user.js(1 hunks)app/templates/activities/ical.hbs(3 hunks)mirage/factories/activity.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/activities/ical.js (1)
app/controllers/application/edit.js (1)
EditController(7-46)
🪛 GitHub Actions: Continuous Integration
app/templates/activities/ical.hbs
[error] 38-38: no-redundant-fn: 'fn' helpers without additional arguments are not allowed.
🪛 GitHub Check: Lint
app/templates/activities/ical.hbs
[failure] 38-38:
fn helpers without additional arguments are not allowed
app/controllers/activities/ical.js
[failure] 28-28:
Replace category with (category)
[failure] 27-27:
Replace category with (category)
[failure] 64-64:
Delete ⏎··⏎··
[failure] 57-57:
Insert ⏎······
[failure] 56-56:
Delete ··
🔇 Additional comments (7)
mirage/factories/activity.js (1)
14-27: LGTM!The factory category options correctly align with the updated
ActivityCategoriesconstant, maintaining consistency between test data and production code.app/templates/activities/ical.hbs (2)
10-11: LGTM!The updated text improves clarity for users.
21-21: LGTM!Explicit
this.context binding improves code clarity and follows modern Ember conventions.Also applies to: 59-59
app/controllers/activities/ical.js (2)
24-29: LGTM!The updated
categoriesParamsgetter correctly returns an array of selected category values, which aligns with the new persistence mechanism.
35-41: Verify backend support for category persistence.The URLs no longer include category query parameters. This means the backend must now read the user's selected categories from the
icalCategoriesattribute stored on the user model. Confirm that the corresponding backend changes (referenced in PR description: csvalpha/amber-api#438) are complete and deployed before merging this PR.app/constants.js (1)
22-35: ActivityCategories and GroupKinds represent separate domain concepts and do not require synchronization.
ActivityCategoriescategorizes activities (Algemeen, Sociëteit, etc.), whileGroupKindsclassifies group types (bestuur, commissie, dispuut, etc.). The values 'kiemgroep' and 'curiositas' are legitimate group types unrelated to the activity categories being updated in this PR. No changes toGroupKindsare needed.Likely an incorrect or invalid review comment.
app/models/user.js (1)
48-48: No changes needed. The code is correct as-is.The
icalCategoriesattribute requires no type annotation. Ember Data passes untyped @attr attributes through unchanged as JSON, and arrays serialize as JSON arrays by default without explicit type annotation. This approach aligns with the established pattern throughout the codebase, where many attributes (likefoodPreferences,address, etc.) are declared without type annotations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/activities/ical.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/activities/ical.js (1)
app/controllers/application/edit.js (1)
EditController(7-46)
🪛 ESLint
app/controllers/activities/ical.js
[error] 63-63: Insert ·
(prettier/prettier)
⏰ 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: Build
🔇 Additional comments (3)
app/controllers/activities/ical.js (3)
1-1: LGTM: Extending EditController is appropriate.The change to extend
EditControllerprovides the necessaryflashNoticeservice for user feedback in thesaveCategoriesmethod. This is a clean architectural decision.Also applies to: 8-8
26-28: Public API change: categoriesParams now returns an array.The getter now returns an array of selected category values instead of a query string. This is a breaking change, but appears intentional and coordinated with the template changes that use
saveCategoriesto persist selections.
35-40: Verify backend coordination for icalCategories.The iCal endpoint (
/ical/activities?key=...&user_id=...) no longer includes category parameters in the URL. Instead, categories are now stored in the user model (icalCategories). The backend must be updated to read this user attribute and filter activities accordingly when generating the iCal feed. Since this is a frontend repository, confirm that corresponding changes exist in the amber-api repository and that the/ical/activitiesendpoint has been updated to apply theicalCategoriesfilter.
combining dinsdag kring and woensdag kring in 1 category because all kringen are on Tuesday.
depended on csvalpha/amber-api#438
NEED TO BE MERGED AT THE SAME TIME
fixes #861 and #685
Summary by CodeRabbit
New Features
Documentation
Chores