Add the sub provider field to UI #1173
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe pull request adds support for a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10-15 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #1173 +/- ##
========================================
Coverage 77.62% 77.62%
========================================
Files 54 54
Lines 1341 1341
========================================
Hits 1041 1041
Misses 300 300 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot write a pr description |
|
@lodewiges I've opened a new pull request, #1174, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
This PR adds a sub_provider field to the Users model for storing additional provider metadata. The field is conditionally displayed in Sofia account user forms based on a configurable label environment variable.
Key Changes:
- Added indexed
sub_providerstring column to users table via migration - Added configurable
SUBPROVIDER_LABELenvironment variable for customizing the field label - Updated controller permitted parameters to allow
sub_providerin create and update actions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| config/application.rb | Adds subprovider_label configuration fetched from SUBPROVIDER_LABEL environment variable with default value |
| app/views/users/_new_sofia_account_user_modal.html.erb | Adds conditional sub_provider input field with client-side uppercase transformation |
| app/views/users/_edit_sofia_account_modal.html.erb | Adds conditional sub_provider input field with client-side uppercase transformation |
| app/controllers/users_controller.rb | Permits sub_provider parameter in both permitted_attributes method (for create) and update_with_sofia_account action |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <% end %> | ||
| <%= f.input :email, label: 'E-mailadres', placeholder: 'E-mailadres', required: true %> | ||
| <% if Rails.application.config.x.subprovider_label.present? %> | ||
| <%= f.input :sub_provider, label: "SubProvider", placeholder: Rails.application.config.x.subprovider_label, input_html: { style: 'text-transform: uppercase;', onkeyup: 'this.value = this.value.toUpperCase();' } %> |
There was a problem hiding this comment.
The uppercase transformation is only applied on the client-side using inline JavaScript. This means if JavaScript is disabled or the user bypasses the client-side transformation, lowercase values could be saved to the database. Consider adding a server-side transformation in the User model using a before_save callback or before_validation callback to ensure the sub_provider value is always uppercase regardless of how it's submitted.
| <%= f.input :sub_provider, label: "SubProvider", placeholder: Rails.application.config.x.subprovider_label, input_html: { style: 'text-transform: uppercase;', onkeyup: 'this.value = this.value.toUpperCase();' } %> | |
| <%= f.input :sub_provider, label: "SubProvider", placeholder: Rails.application.config.x.subprovider_label, input_html: { style: 'text-transform: uppercase;' } %> |
| config.x.site_short_name = ENV.fetch('SITE_SHORT_NAME', 'SOFIA') | ||
| config.x.site_long_name = ENV.fetch('SITE_LONG_NAME', 'Streepsysteem voor de Ordentelijke Festiviteiten van Inleggend Alpha') | ||
| config.x.site_association = ENV.fetch('SITE_ASSOCIATION', 'C.S.V. Alpha') | ||
|
|
There was a problem hiding this comment.
The new SUBPROVIDER_LABEL environment variable is not documented in the .env.example file. This file should include all available environment variables with example values to help developers configure the application correctly. Consider adding a line like 'SUBPROVIDER_LABEL=Subprovider' to document this new configuration option.
| <%= f.input :name, label: 'Naam', placeholder: 'Naam', required: true %> | ||
| <%= f.input :email, label: 'E-mailadres', placeholder: 'E-mailadres', required: true %> | ||
| <% if Rails.application.config.x.subprovider_label.present? %> | ||
| <%= f.input :sub_provider, label: "SubProvider", placeholder: Rails.application.config.x.subprovider_label, input_html: { style: 'text-transform: uppercase;', onkeyup: 'this.value = this.value.toUpperCase();' } %> |
There was a problem hiding this comment.
The uppercase transformation is only applied on the client-side using inline JavaScript. This means if JavaScript is disabled or the user bypasses the client-side transformation, lowercase values could be saved to the database. Consider adding a server-side transformation in the User model using a before_save callback or before_validation callback to ensure the sub_provider value is always uppercase regardless of how it's submitted.
| <%= f.input :sub_provider, label: "SubProvider", placeholder: Rails.application.config.x.subprovider_label, input_html: { style: 'text-transform: uppercase;', onkeyup: 'this.value = this.value.toUpperCase();' } %> | |
| <%= f.input :sub_provider, label: "SubProvider", placeholder: Rails.application.config.x.subprovider_label, input_html: { style: 'text-transform: uppercase;' } %> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/users_controller.rb (1)
170-170: Consider using symbols for consistency.The parameter list uses strings (
%w[...]) while theupdate_with_sofia_accountmethod uses symbols (%i[...]) on line 143. While both work in Rails, using symbols consistently throughout the controller improves code uniformity.🔎 Optional refactor for consistency:
- params.require(:user).permit(%w[name email provider sub_provider]) + params.require(:user).permit(%i[name email provider sub_provider])
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/users_controller.rb(2 hunks)app/views/users/_edit_sofia_account_modal.html.erb(1 hunks)app/views/users/_modal.html.erb(1 hunks)config/application.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/users_controller.rb (1)
app/models/user.rb (1)
treasurer?(86-88)
⏰ 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)
config/application.rb (1)
78-79: LGTM!The configuration approach using
ENV.fetchwith anildefault is appropriate, enabling optional feature activation via environment variables. The conditional rendering in the views correctly checks for the presence of this config value.Note: A previous review comment already flagged that this environment variable should be documented in
.env.example.app/views/users/_edit_sofia_account_modal.html.erb (1)
24-26: Conditional rendering logic is correct.The conditional check ensures the SubProvider field only appears when the feature is enabled via environment configuration, which is the appropriate approach.
Note: A previous review comment already flagged that uppercase transformation should be implemented server-side to handle cases where JavaScript is disabled or bypassed.
app/controllers/users_controller.rb (1)
143-144: Parameter whitelisting is correct.The
sub_providerparameter is appropriately permitted for updates alongside other user attributes. The nestedsofia_account_attributeswhitelisting also correctly supports updating the associated Sofia account.Note: As flagged in previous review comments, consider adding server-side uppercase transformation (e.g.,
before_savecallback in the User model) to ensure data consistency regardless of client-side behavior.
| <% if Rails.application.config.x.subprovider_label.present? %> | ||
| <%= f.input :sub_provider, label: 'Subprovider', placeholder: Rails.application.config.x.subprovider_label, required: true, input_html: { style: 'text-transform: uppercase;', onkeyup: 'this.value = this.value.toUpperCase();' } %> | ||
| <% end %> |
There was a problem hiding this comment.
Standardize the label capitalization.
The label 'Subprovider' here differs from 'SubProvider' used in _edit_sofia_account_modal.html.erb (line 25). For consistency, use the same capitalization across both modals.
Note: A previous review comment already flagged that uppercase transformation should be implemented server-side.
🔎 Suggested fix to match the edit modal's capitalization:
- <%= f.input :sub_provider, label: 'Subprovider', placeholder: Rails.application.config.x.subprovider_label, required: true, input_html: { style: 'text-transform: uppercase;', onkeyup: 'this.value = this.value.toUpperCase();' } %>
+ <%= f.input :sub_provider, label: 'SubProvider', placeholder: Rails.application.config.x.subprovider_label, required: true, input_html: { style: 'text-transform: uppercase;', onkeyup: 'this.value = this.value.toUpperCase();' } %>📝 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.
| <% if Rails.application.config.x.subprovider_label.present? %> | |
| <%= f.input :sub_provider, label: 'Subprovider', placeholder: Rails.application.config.x.subprovider_label, required: true, input_html: { style: 'text-transform: uppercase;', onkeyup: 'this.value = this.value.toUpperCase();' } %> | |
| <% end %> | |
| <% if Rails.application.config.x.subprovider_label.present? %> | |
| <%= f.input :sub_provider, label: 'SubProvider', placeholder: Rails.application.config.x.subprovider_label, required: true, input_html: { style: 'text-transform: uppercase;', onkeyup: 'this.value = this.value.toUpperCase();' } %> | |
| <% end %> |
🤖 Prompt for AI Agents
In app/views/users/_modal.html.erb around lines 16 to 18, the input label reads
'Subprovider' but should match the capitalization used in
_edit_sofia_account_modal.html.erb ('SubProvider'); update the f.input label to
'SubProvider' for consistency and ensure the form still relies on server-side
normalization for uppercase (do not remove existing client-side transform).
Summary
Adds sub_provider field to Users for storing additional provider metadata.
Database changes:
New indexed sub_provider string column on users table (migration 20251209160709_add_user_subprovider.rb)
UI changes:
Added to new Sofia account user creation modal
Added to user settings edit modal under account data section
Input auto-uppercases via inline JavaScript
Controller changes:
Permitted in users_controller.rb for both create and update actions
Other information
Field appears in forms at:
app/views/users/_new_sofia_account_user_modal.html.erb (line 16)
app/views/users/_edit_sofia_account_modal.html.erb (line 24)
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.