Skip to content

NoMethodError raised when voting comments from threads#7880

Merged
mrcasals merged 4 commits intodecidim:developfrom
i-need-another-coffee:fix/comment_notification_route
Apr 27, 2021
Merged

NoMethodError raised when voting comments from threads#7880
mrcasals merged 4 commits intodecidim:developfrom
i-need-another-coffee:fix/comment_notification_route

Conversation

@roxanaopr
Copy link
Copy Markdown
Contributor

🎩 What? Why?

When a comment is added you can up-vote or down-vote it and this will trigger a notification in which is included the link to the resource. For the comments from threads this action will raise an error:

NoMethodErrorSidekiq/ActionMailer::MailDeliveryJob
error
undefined method `route_name' for nil:NilClass

📌 Related Issues

Link your PR to an issue

  • Related to #?
  • Fixes #?

Testing

  • Add a reply to a comment.
  • Vote that comment from the reply.
  • Go to the notifications page
  • Error is displayed

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

Screenshot from 2021-04-22 10-04-42

♥️ Thank you!

@mrcasals
Copy link
Copy Markdown
Contributor

Hi @roxanaopr! Thanks for the PR, could you add some tests for it?

Thanks!

@mrcasals mrcasals added module: comments type: fix PRs that implement a fix for a bug labels Apr 22, 2021
end

def target_resource(t_resource)
t_resource.is_a?(Decidim::Comments::Comment) ? target_resource(t_resource.commentable) : t_resource
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try using comment.root_commentable to get to the parent and avoid the recursive call (and possible N+1s)?

@mrcasals mrcasals merged commit c129bf8 into decidim:develop Apr 27, 2021
@mrcasals mrcasals deleted the fix/comment_notification_route branch April 27, 2021 12:31
entantoencuanto added a commit that referenced this pull request Apr 30, 2021
* develop:
  Remove creation date from meeting card (#7922)
  Use NPM instead of yarn on CI (#7919)
  Validate nickname using correct regexp (#7900)
  Make webpacker build available in production (#7915)
  New Crowdin updates (#7911)
  Open attachments in new tab (#7912)
  Fix JS errors in the admin panel (#7903)
  Fix editor: remove br tags from inside a tags (#7901)
  Authorizable comment action for proposals (#6916)
  NoMethodError raised when voting comments from threads (#7880)
  Fix not signed in needs permission redirect for internal links (#7890)
  Fix meeting registrations questionnaire free text choice answers export (#7892)
  Store election verifiable results data in election (#7882)
  New Crowdin updates (#7884)
carlobeltrame added a commit to puzzle/decidim-zuerich that referenced this pull request Sep 6, 2021
Applies the fix from decidim/decidim#7880
This will be obsolete when we upgrade to decidim 0.25
rossriserose added a commit to rossriserose/DECIDIM-ZUERICH that referenced this pull request Dec 19, 2025
Applies the fix from decidim/decidim#7880
This will be obsolete when we upgrade to decidim 0.25
happysemicolon added a commit to happysemicolon/DECIDIM-ZUERICH that referenced this pull request Feb 10, 2026
Applies the fix from decidim/decidim#7880
This will be obsolete when we upgrade to decidim 0.25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: comments type: fix PRs that implement a fix for a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants