Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport 'Implement push notifications for conversations' messages' to v0.27 #12511

Conversation

alecslupu
Copy link
Contributor

🎩 What? Why?

Backport #12475 to v0.27

♥️ Thank you!

* Refactor variables definitions in SendPushNotification spec

* Add spec for ConversationMailer

It also adds the factories to ease-up the specs

* Extract ConversationMailer methods to HasConversation concern

* Add PushNotificationMessage model

* Implement push notifications for conversations' messages

* Fix spec

* Simplify PushNotificationMessage model by removing useless attributes

* Fix url and icon methods in PushNotificationMessage

As a bonus, there are more examples in the spec.

* Add missing class in yardoc

* Clarify  signature

* Use more idiomatic way of checking for presence

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>

* Move module inclusion to the top for consistency

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>

* Add skip_injection on factory definition

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>

* Use alias_method for :user

* Fix rubocop offenses

---------

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
@alecslupu alecslupu added backport Pull Requests that are a backport for a fixed bug module: core type: fix PRs that implement a fix for a bug labels Feb 21, 2024
@alecslupu alecslupu marked this pull request as draft February 22, 2024 09:18
@alecslupu alecslupu added this to the 0.27.6 milestone Feb 22, 2024
@alecslupu alecslupu marked this pull request as ready for review February 27, 2024 06:36
Copy link
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.

The only thing I could found is that we're not using keyword arguments in PushNotificationMessage. I'd prefer to use the same signature, just in case we have to backport a fix in the future

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
@alecslupu
Copy link
Contributor Author

The only thing I could found is that we're not using keyword arguments in PushNotificationMessage. I'd prefer to use the same signature, just in case we have to backport a fix in the future

@andreslucena , i have applied the code suggestion, and for some reason, this does not work as expected with ruby 3.0.x

image

In order to move forward the PR, I would revert to my initial PR.

@andreslucena
Copy link
Member

Let me check it out in a couple of minutes

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
@andreslucena
Copy link
Member

Merging with the failing check from codecov

@andreslucena andreslucena merged commit c0fac7e into release/0.27-stable Feb 27, 2024
42 of 43 checks passed
@andreslucena andreslucena deleted the backport/0.27/implement-push-notifications-f-12475 branch February 27, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Pull Requests that are a backport for a fixed bug module: core type: fix PRs that implement a fix for a bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants