Make product name mutable again - #1007
Conversation
lodewiges
left a comment
lodewiges
left a comment
There was a problem hiding this comment.
Looks good, i would agree that we should implement a warning when somebody changes a product
| subject(:product) { create(:product) } | ||
|
|
||
| it { expect(product.update(name: 'new_name')).to be false } | ||
| end |
There was a problem hiding this comment.
instead of removing the test we could edit it
|
bump |
1 similar comment
|
bump |
|
Warning Rate limit exceeded@lodewiges has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRemoved the Product model's name-readonly enforcement, enabled editing of product names in the price lists form, added a client-side confirmation when renaming an existing product, and updated the model spec expectation to accept name changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as PriceLists Form (JS)
participant Server as Rails App
participant Product as Product Model
rect rgb(240, 240, 255)
Note over UI,Product: Old flow (before change)
User->>UI: Edit product name and submit
UI->>Server: PUT /products/:id (name: "new")
Server->>Product: update(name: "new")
Product->>Product: validate :name_readonly
Product-->>Server: Validation error (reject)
Server-->>UI: 422 Unprocessable Entity (show error)
end
rect rgb(240, 255, 240)
Note over UI,Product: New flow (after change)
User->>UI: Edit product name and submit
UI->>UI: if product.id && name != original_name -> prompt confirm
alt user cancels
UI-->>User: abort save
else user confirms
UI->>Server: PUT /products/:id (name: "new")
Server->>Product: update(name: "new")
Product-->>Server: success
Server-->>UI: 200 OK (update succeeded)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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: 0
🧹 Nitpick comments (1)
app/views/price_lists/index.html.erb (1)
63-63: Consider implementing the user warning mentioned in the PR description.Removing
:disabled="product.id"now allows users to edit existing product names, which aligns with the PR objective. However, the PR description notes that the original concern from PR #595 was users unintentionally modifying product names when they meant to create new products, and proposes considering a user-facing warning (tooltip or dialog) to address this.No such warning mechanism appears to be implemented in this change. Consider adding a confirmation dialog or tooltip when users edit an existing product's name to prevent accidental modifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/models/product.rb(0 hunks)app/views/price_lists/index.html.erb(2 hunks)spec/models/product_spec.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/product.rb
🧰 Additional context used
🧬 Code graph analysis (1)
spec/models/product_spec.rb (1)
app/controllers/products_controller.rb (1)
update(18-26)
⏰ 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 (2)
spec/models/product_spec.rb (1)
21-25: LGTM! Test correctly validates the new mutable behavior.The test now expects
product.update(name: 'new_name')to returntrue, which aligns with the removal of the readonly validation on product names. This validates that the feature works as intended.app/views/price_lists/index.html.erb (1)
28-28: Good fix for trailing whitespace.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## staging #1007 +/- ##
===========================================
- Coverage 75.17% 74.44% -0.74%
===========================================
Files 55 51 -4
Lines 1116 1088 -28
===========================================
- Hits 839 810 -29
- Misses 277 278 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…a/sofia into fix/mutable_product_name
2 participants
fixes #937
Basically reverts #598
The concern with #595 was that users were modifying product names when they really just wanted to create a new product. Should we provide the user with a warning, like a tooltip or a dialog?
Summary by CodeRabbit
New Features
Bug Fixes
Style