Skip to content

Fix push notifications URL method#10017

Merged
ahukkanen merged 4 commits intodevelopfrom
fix/push-notification-presenter-undef-method-9914
Nov 29, 2022
Merged

Fix push notifications URL method#10017
ahukkanen merged 4 commits intodevelopfrom
fix/push-notification-presenter-undef-method-9914

Conversation

@ferblape
Copy link
Copy Markdown
Contributor

@ferblape ferblape commented Nov 2, 2022

🎩 What? Why?

This PR fixes the error reported for some kind of notifications.

📌 Related Issues

Testing

  • Subscribe to a process
  • Generate a push notification with a model that doesn't have a reported_content_url method (like a Component, Blogs::Post, User, are the ones that I see in Sentry)
  • Expect to receive the notification

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@ferblape ferblape force-pushed the fix/push-notification-presenter-undef-method-9914 branch from 8e2c3f9 to bea89e2 Compare November 3, 2022 09:48
andreslucena
andreslucena requested changes Nov 3, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

As this is a fix, can you add a spec with the failing example 🙏🏽? It should be in https://github.com/decidim/decidim/blob/develop/decidim-core/spec/presenters/decidim/push_notification_presenter_spec.rb
Thanks

@andreslucena andreslucena added module: core type: fix PRs that implement a fix for a bug project: PWA Barcelona City Council contract labels Nov 3, 2022
@ferblape
Copy link
Copy Markdown
Contributor Author

ferblape commented Nov 4, 2022

Yep, totally, I was just playing with the possible solution.

@ferblape ferblape force-pushed the fix/push-notification-presenter-undef-method-9914 branch from bea89e2 to 964891f Compare November 7, 2022 05:22
andreslucena
andreslucena requested changes Nov 8, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

I think we can improve it with an specific case from the original issue, what do you think?

ferblape and others added 2 commits November 13, 2022 05:52
@ferblape @andreslucena
Improve specs
a67b1a1 
Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
@ferblape
Detail
11d42bf
@ferblape
Copy link
Copy Markdown
Contributor Author

ferblape commented Nov 13, 2022

I think we can improve it with an specific case from the original issue, what do you think?

OK, although my test already covered the error after updating it in 964891f but now is more complete, so ok to me and ready to review again @andreslucena

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Nov 14, 2022

I think we can improve it with an specific case from the original issue, what do you think?

OK, although my test already covered the error after updating it in 964891f but now is more complete, so ok to me and ready to review again @andreslucena

The initial report wasn't that the comment URL wasn't the full comment URL, but that there was an exception with other resources. So, I guess that with this solution we're solving two bugs for the price of one 😄

@andreslucena andreslucena changed the title Fix notifications URL method Fix push notifications URL method Nov 14, 2022
andreslucena
andreslucena approved these changes Nov 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

👍🏽

@ahukkanen ahukkanen merged commit 75c1d19 into develop Nov 29, 2022
@ahukkanen ahukkanen deleted the fix/push-notification-presenter-undef-method-9914 branch November 29, 2022 17:54
entantoencuanto added a commit that referenced this pull request Dec 16, 2022
@entantoencuanto
Merge branch 'develop' into feature/redesign-meetings
65addab 
* develop:
  Redesign: action buttons (#9852)
  Integrate reported users in Global moderation (#10018)
  Fix resource_icon with component or manifest nil (#10134)
  Revent Docker actions to Ubuntu 20.04 due to OpenSSL issues (#10142)
  Fix push notifications URL method (#10017)
  Fix typo in README (#10110)
  Add correct call for conference speaker (#10061)
  Fix missing fields on duplicate meetings functionality (#9899)
  Fix translations missing on admin log (#9889)
@alecslupu alecslupu mentioned this pull request Jan 26, 2023
Merged
@alecslupu alecslupu mentioned this pull request Jul 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andreslucena andreslucena andreslucena approved these changes

@ahukkanen ahukkanen ahukkanen approved these changes

Assignees

No one assigned

Labels

module: core project: PWA Barcelona City Council contract type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Exception in PushNotificationPresenter (undefined method `reported_content_url')

3 participants

@ferblape @andreslucena @ahukkanen