Prevent comment button on non-commentable proposals#29
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a commentability check to the proposals chatbot workflow: when the proposal component permits comments the bot sends interactive buttons (including a comment action) and delegates to CommentsWorkflow; when comments are disabled it sends a plain text preview and does not delegate. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Chatbot as ProposalsWorkflow
participant Comments as CommentsWorkflow
participant Platform as MessagingAPI
User->>Chatbot: request proposal details / click proposal
alt component_commentable? = true
Chatbot->>Platform: send interactive_buttons (includes comment button)
User->>Platform: click comment button
Platform->>Chatbot: action_input (comment)
Chatbot->>Comments: delegate comment flow
Comments->>Platform: send comment prompts / create comment
else component_commentable? = false
Chatbot->>Platform: send plain text message (body + preview_url)
Note over Platform,Chatbot: No delegation to CommentsWorkflow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Updates the chatbot proposals flow to respect the Proposals component “comments enabled” setting when presenting proposal details in WhatsApp, preventing users from seeing a comment button when commenting is disabled.
Changes:
- Conditionally renders an interactive “comment” button only when the component has comments enabled.
- Falls back to sending a plain text WhatsApp message (no buttons) when comments are disabled.
- Extends RSpec coverage to verify both enabled/disabled behaviors and updates existing media-related specs to set
comments_enabled: true.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/services/decidim/chatbot/workflows/proposals_workflow.rb | Adds a component commentability check and branches message type accordingly in send_proposal_details. |
| spec/services/workflows/proposals_workflow_spec.rb | Adds/updates specs to assert correct message format depending on comments_enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if component_comentable? | ||
| send_message!( | ||
| type: :interactive_buttons, | ||
| body_text: body, | ||
| header_image:, | ||
| footer_text: sanitize_text(proposal.creator_author&.presenter&.name, 60), | ||
| buttons: [ | ||
| { | ||
| id: "comment-#{proposal.id}", | ||
| title: I18n.t("decidim.chatbot.workflows.proposals.buttons.comment") | ||
| } | ||
| ] | ||
| ) | ||
| else | ||
| send_message!(body:, preview_url: true) | ||
| end |
There was a problem hiding this comment.
This change only hides the comment button when comments are disabled, but process_action_input will still delegate to CommentsWorkflow whenever a user sends a comment-<id> button id (e.g., from an older message or a crafted payload). Consider enforcing the same component_*commentable? check in the commentable_id branch and returning a non-interactive message when commenting is disabled.
![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)
spec/services/workflows/proposals_workflow_spec.rb (1)
317-334: Consider strengthening thebody:assertion.
body: anythingconfirms a key is present but doesn't verify content. Asserting that the body includes at least the proposal URL would make this test more meaningful and guard against regressions where the body is accidentally emptied.✨ Suggested improvement
expect(adapter).to receive(:send_message!).with( hash_including( - body: anything, + body: a_string_including(resource_url(clicked_proposal)), preview_url: true ) ) do |args|🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/workflows/proposals_workflow_spec.rb` around lines 317 - 334, The test currently uses body: anything which only verifies presence of a body; update the expectation in the adapter.send_message! call to assert the body includes the proposal URL (e.g. change body: anything to body: a_string_including(proposal.url) or body: a_string_including(proposal_url(proposal)) depending on your helper) while keeping preview_url: true and the post-check that args does not include :type; ensure you reference the existing proposals_component/proposal object used by the spec and keep subject.start to trigger the call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/services/decidim/chatbot/workflows/proposals_workflow.rb`:
- Around line 150-152: Rename the misspelled helper method component_comentable?
to component_commentable? and fix its boolean memoization so false is cached:
implement an explicit defined?/instance_variable_defined? guard (e.g., return
`@component_commentable` if defined?(`@component_commentable`)) and only then assign
`@component_commentable` = component&.settings&.comments_enabled?; update all call
sites in this class/workflow that referenced component_comentable? to the new
component_commentable? name.
---
Nitpick comments:
In `@spec/services/workflows/proposals_workflow_spec.rb`:
- Around line 317-334: The test currently uses body: anything which only
verifies presence of a body; update the expectation in the adapter.send_message!
call to assert the body includes the proposal URL (e.g. change body: anything to
body: a_string_including(proposal.url) or body:
a_string_including(proposal_url(proposal)) depending on your helper) while
keeping preview_url: true and the post-check that args does not include :type;
ensure you reference the existing proposals_component/proposal object used by
the spec and keep subject.start to trigger the call.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/services/decidim/chatbot/workflows/proposals_workflow.rbspec/services/workflows/proposals_workflow_spec.rb
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/services/workflows/proposals_workflow_spec.rb (1)
504-511: Strengthen the disabled-path assertion to prevent hidden delegation.Line 510 only checks
CommentsWorkflow#startis not called. Add an expectation thatCommentsWorkflow.newis also not invoked, so delegation is fully prevented.🧪 Suggested test hardening
it "sends a non-interactive message and does not delegate" do + expect(CommentsWorkflow).not_to receive(:new) expect(adapter).to receive(:send_message!).with( hash_including( body: I18n.t("decidim.chatbot.workflows.proposals.comments_disabled") ) ) expect(comments_workflow_instance).not_to receive(:start) subject.start end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/workflows/proposals_workflow_spec.rb` around lines 504 - 511, The test currently asserts that CommentsWorkflow#start is not called but doesn't prevent constructing a CommentsWorkflow; add an expectation that CommentsWorkflow.new is not invoked (e.g., expect(CommentsWorkflow).not_to receive(:new)) before calling subject.start so the spec ensures no delegation or instantiation occurs; keep the existing expect(comments_workflow_instance).not_to receive(:start) and place the new expectation alongside it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/locales/en.yml`:
- Around line 75-76: The copy for the localization key comment_received still
references the old button label "Submit comment"; update the string to use the
renamed label "Publish comment" so it matches the button text elsewhere (i.e.,
change the text in the comment_received value to end with "Publish comment").
---
Nitpick comments:
In `@spec/services/workflows/proposals_workflow_spec.rb`:
- Around line 504-511: The test currently asserts that CommentsWorkflow#start is
not called but doesn't prevent constructing a CommentsWorkflow; add an
expectation that CommentsWorkflow.new is not invoked (e.g.,
expect(CommentsWorkflow).not_to receive(:new)) before calling subject.start so
the spec ensures no delegation or instantiation occurs; keep the existing
expect(comments_workflow_instance).not_to receive(:start) and place the new
expectation alongside it.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/services/decidim/chatbot/workflows/proposals_workflow.rbconfig/locales/en.ymlconfig/locales/pt-BR.ymlspec/services/workflows/proposals_workflow_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/services/decidim/chatbot/workflows/proposals_workflow.rb
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| end | ||
|
|
||
| def component_commentable? | ||
| @component_commentable ||= component&.settings&.comments_enabled? |
There was a problem hiding this comment.
component_commentable? uses ||= memoization, which won’t cache a false result (so when comments are disabled it will recompute on every call). Use an instance_variable_defined?/defined? guard (or remove memoization entirely) so false/nil values are cached too.
| @component_commentable ||= component&.settings&.comments_enabled? | |
| return @component_commentable if instance_variable_defined?(:@component_commentable) | |
| @component_commentable = component&.settings&.comments_enabled? |
Description
This PR implements the feature to check if proposal component settings allow commenting before showing the comment button.
Changes
Spec Updates
Fixes
Resolves #27
Summary by CodeRabbit
New Features
Tests
Documentation