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

Use base_class when specifying recipient_type #384

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Use base_class when specifying recipient_type #384

merged 1 commit into from
Jan 24, 2024

Conversation

alikrugl
Copy link
Contributor

@alikrugl alikrugl commented Jan 24, 2024

Pull Request

Summary:
Updates app/models/concerns/noticed/deliverable.rb to ensure it stores the base model class for STI models in the recipient_type column of the polymorphic association.

Related Issue:
Fixed #383

Testing:
All Tests are passing
I have installed the feature in my app and tested it.

Checklist:

  • Code follows the project's coding standards
  • Tests have been added or updated to cover the changes
  • Documentation has been updated (if applicable)
  • All existing tests pass
  • Conforms to the contributing guidelines

This is my first time contributing to open source, so any feedback is appreciated!

@excid3
Copy link
Owner

excid3 commented Jan 24, 2024

I think this will be a good default. I would assume most STI models as recipients will have notifications associated on the base class. However, it's possible that they might not and this would not work in that situation. Those cases can be solved by overriding the recipient_attributes_for method to ignore the base class.

Looks like Rails handles this properly if the has_many :notifications is only defined on the subclass. Nice!

irb> Admin.first.notifications
  Admin Load (0.4ms)  SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT ?  [["LIMIT", 1]]
  Noticed::Notification Load (1.0ms)  SELECT "noticed_notifications".* FROM "noticed_notifications" WHERE "noticed_notifications"."recipient_id" = ? AND "noticed_notifications"."recipient_type" = ? /* loading for pp */ LIMIT ?  [["recipient_id", 1], ["recipient_type", "User"], ["LIMIT", 11]]

@excid3 excid3 merged commit 6343c76 into excid3:main Jan 24, 2024
45 checks passed
@alikrugl alikrugl deleted the recipient-type-should-be-set-to-base-class branch January 24, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Inaccurate recipient_type class name for STI model, when delivering the notification
2 participants