Draft
Conversation
- Include notify_emails_to and notify_emails_to_locale fields in the Push model. - Update push_params to permit new fields based on push kind. - Render notify emails fields in the relevant forms: _form, _files_form, _url_form, _qr_form. - Trigger email notifications upon push creation.
- Update error messages to use plain text instead of translation function. - Change condition for invalid email addresses to use 'unless' for better readability.
…ields - Introduced `smtp_configured?` method in ApplicationHelper to determine if SMTP settings are properly configured based on the environment. - Updated forms (_form, _files_form, _qr_form, _url_form) to conditionally render notify emails fields only when SMTP is configured. - Added tests for the new method and its integration with the push creation process, ensuring proper behavior in different environments.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request implements an auto-dispatch email notification feature for pushes, allowing users to specify email recipients who should be notified when a push is created. The feature includes multi-language support via an optional locale parameter.
Changes:
- Added database columns
notify_emails_to(text) andnotify_emails_to_locale(string) to the pushes table - Implemented email validation, mailer, job, and UI components to support the notification feature
- Added comprehensive test coverage for validators, models, mailers, jobs, and integration scenarios
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| db/migrate/20260202120000_add_notify_emails_to_pushes.rb | Adds database columns for email notification recipients and locale preference |
| db/schema.rb | Updates schema version and adds the new columns to pushes table |
| app/models/push.rb | Includes the NotifyEmailsTo concern to add email notification functionality |
| app/models/concerns/pwpush/notify_emails_to.rb | Concern that adds validation and send_creation_emails method to trigger email jobs |
| app/validators/multiple_emails_validator.rb | Custom validator for comma-separated email addresses (max 5, no duplicates, valid format) |
| app/mailers/push_created_mailer.rb | Mailer that sends notification emails with the secret URL and locale support |
| app/views/push_created_mailer/notify.html.erb | HTML email template for push creation notifications |
| app/views/push_created_mailer/notify.text.erb | Plain text email template for push creation notifications |
| app/views/shared/_notify_emails_to.html.erb | Form partial for email notification recipients input field |
| app/views/shared/_notify_emails_to_locale.html.erb | Form partial for email notification language selection |
| app/views/pushes/_form.html.erb | Adds email notification fields to text push form (when SMTP configured) |
| app/views/pushes/_files_form.html.erb | Adds email notification fields to file push form (when SMTP configured) |
| app/views/pushes/_url_form.html.erb | Adds email notification fields to URL push form (when SMTP configured) |
| app/views/pushes/_qr_form.html.erb | Adds email notification fields to QR code push form (when SMTP configured) |
| app/jobs/send_push_created_email_job.rb | Background job that sends push creation notification emails |
| app/helpers/application_helper.rb | Adds smtp_configured? helper to determine if email functionality should be shown |
| app/controllers/pushes_controller.rb | Updates push_params to permit email notification parameters and calls send_creation_emails after push creation |
| test/mailers/previews/push_created_mailer_preview.rb | Preview for the push creation email in development |
| test/validators/multiple_emails_validator_test.rb | Comprehensive tests for the email validator |
| test/models/push_notify_emails_test.rb | Tests for push model email notification behavior |
| test/mailers/push_created_mailer_test.rb | Tests for the mailer functionality including locale support |
| test/unit/send_push_created_email_job_test.rb | Tests for the email job |
| test/integration/notify_emails_creation_test.rb | Integration tests for the complete email notification flow |
| test/helpers/application_helper_test.rb | Tests for the smtp_configured? helper method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Moved email parsing logic to the Pwpush::NotifyEmailsTo module for better organization and reuse. - Updated MultipleEmailsValidator to use the new parsing method and enforce case-insensitive duplicate checks. - Enhanced PushCreatedMailer to utilize the new email parsing method. - Adjusted email templates to handle potential nil values for brand title and tagline. - Added validation for notify_emails_to_locale to ensure it matches available locales. - Introduced throttling settings for push creation to limit requests per IP.
- Introduced a new configuration option `pushes_per_day` in settings.yml to limit the number of push creations per IP address to 4500 per 24 hours. - This enhancement aims to reduce spam and manage server load effectively.
- Clear notify_emails_to and notify_emails_to_locale for anonymous users in PushesController. - Update email templates to improve clarity and security messaging, including important notes about link expiration and access requirements. - Conditionally render notify emails fields in forms only for logged-in users. - Add tests to ensure proper behavior for push creation and email notifications based on user authentication status.
- Introduced a test to ensure that the notify_emails_to and notify_emails_to_locale fields cannot be changed during a push update. - Added a validation test for handling consecutive commas in email input, normalizing to a valid list. - These enhancements improve the integrity of email notification settings and user input validation.
- Updated PushCreatedMailer to include the sender's email in the subject line for better clarity. - Refactored MultipleEmailsValidator to add error messages to the base instead of specific attributes, improving consistency in error reporting. - Adjusted tests to reflect changes in error message handling and ensure validation logic is correctly verified.
- Adjusted the subject line in PushCreatedMailer to ensure the sender's email is displayed correctly. - Updated corresponding test to verify that the subject includes the expected text for notification emails.
- Introduced tests to validate the handling of duplicate emails in notify_emails_to and to ensure invalid notify_emails_to_locale is correctly rejected. - Added tests to verify that the email subject includes the user's email or defaults to "Someone" when no user is associated with the push. - Enhanced tests to confirm that the email body includes expiration days and views, improving the clarity of notifications sent to users.
- Updated the test for the notify method in PushCreatedMailer to include a message for clarity when asserting the presence of the email subject. - Added a comment to clarify the expected format of the subject line, improving the documentation within the test.
- Introduced `format_time_remaining` and `format_minutes_duration` methods in PushesHelper to format push expiration times for better clarity in notifications. - Updated PushCreatedMailer to include formatted duration in email body, enhancing user understanding of link validity. - Modified email templates to display the formatted duration and ensure proper HTML rendering of the secret link. - Added tests to verify the inclusion of duration and view limits in email notifications, ensuring accurate communication to users.
- Introduced tests to validate the behavior of creating pushes with duplicate notify_emails_to, ensuring a 422 response is returned. - Added a test to confirm that a blank notify_emails_to does not enqueue a job and is stored as blank. - Enhanced tests to verify that created pushes with notify_emails_to correctly reflect expiration and view limits in the email body. - Added tests for the PushCreatedMailer to ensure correct recipient handling and inclusion of time unit words in the email body.
…for time formatting - Simplified the push creation throttling logic in Rack::Attack by introducing a constant for the relevant paths and using a lambda for clarity. - Added comprehensive tests for `format_time_remaining` and `format_minutes_duration` methods to ensure accurate time formatting in notifications. - Enhanced existing tests to cover various scenarios for time formatting, improving the robustness of the helper methods.
- Introduced tests to verify the structure of the HTML part of the notification email, ensuring it includes an h1 heading, a secret link anchor, and an Important Notes section. - Added tests for the text part of the notification email to confirm the inclusion of the secret URL and Important Notes. - These enhancements improve the robustness of email notification tests, ensuring proper formatting and content delivery.
… notifications - Introduced tests to verify that the Important Notes section in the HTML part of the notification email contains exactly three list items. - Added a test to ensure the secret link in the HTML part has a full clickable URL, enhancing the robustness of email content validation. - These additions improve the reliability of email notifications by confirming proper formatting and link functionality.
- Introduced a test to ensure that the text of the secret link in the HTML part of the notification email matches the href URL. - This addition enhances the validation of email content, ensuring that the link text is accurate and improves the overall reliability of email notifications.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…PushesHelper - Changed the parameter description for the format_minutes_duration method to clarify that it accepts an Integer representing the number of minutes, enhancing code readability and understanding.
…eatedMailer - Modified the secret URL generation logic in ApplicationHelper to prioritize the method locale over the params locale, ensuring consistent behavior when called from mailers. - Updated PushCreatedMailer to utilize the new secret URL method, enhancing clarity and maintainability. - Adjusted tests in ApplicationHelperTest to reflect the updated priority logic for locale handling in secret URL generation.
…nd improve time formatting - Modified the email templates in PushCreatedMailer to default to "Someone" when the user email is not present, enhancing clarity in notifications. - Updated the text rendering of the expiration message to utilize the format_time_remaining method, ensuring accurate time representation in the email body. - Removed unused duration text variable from the mailer, streamlining the code.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # The throttle is registered in this test (same logic as config/initializers/rack_attack.rb). | ||
|
|
||
| THROTTLE_TEST_IP = "192.168.1.100" | ||
| PUSH_CREATION_PATHS = %w[/p /p.json /f /f.json /r /r.json].freeze |
There was a problem hiding this comment.
The PUSH_CREATION_PATHS constant is duplicated in three places: the initializer (config/initializers/rack_attack.rb:38) and two test files (test/unit/rack_attack_pushes_throttle_test.rb:11 and test/integration/rack_attack_pushes_throttle_test.rb:11). If new push creation paths are added in the future, all three locations must be updated. Consider defining this constant in a shared location (e.g., a constants file or the Rack::Attack configuration itself) and referencing it in the tests to maintain a single source of truth.
Suggested change
| PUSH_CREATION_PATHS = %w[/p /p.json /f /f.json /r /r.json].freeze | |
| PUSH_CREATION_PATHS = Rack::Attack::PUSH_CREATION_PATHS |
Collaborator
Author
There was a problem hiding this comment.
let's keep as is for simplicity
…onment - Updated the notify_emails_to concern to run the SendPushCreatedEmailJob inline in the development environment, allowing immediate email delivery for testing purposes. - Added comments to clarify the rationale behind this change, facilitating easier testing with tools like Mailbin without the need for a job queue.
Updated the logic for forced HTTPS links in the application helper to simplify the code. The new implementation uses `gsub!` to replace "http" with "https" when the FORCE_SSL environment variable is set, ensuring that the request is not already using SSL.
Updated the order of controller registrations in the index.js file for improved organization. The 'gdpr' and 'pwgen' controllers were repositioned, ensuring a clearer structure and consistency in the registration process.
Deleted the test case that verified the inclusion of a secure URL when the FORCE_SSL environment variable is set, as it is no longer relevant to the current implementation.
Collaborator
Author
|
Refactoring dropdown to unite with secret_url partial - not urgent but it could reduce some code. |
Updated the secret URL bar and notification email locale components to utilize a shared dropdown partial for improved consistency and maintainability. The new implementation enhances accessibility by providing appropriate ARIA labels and streamlines the rendering logic for language selection.
…ainability Updated the locale dropdown partial to enhance code clarity by introducing local variables for labels and classes. Streamlined the rendering logic for language options and improved accessibility with consistent ARIA labels. This refactor also consolidates the dropdown item styles for better consistency across the component.
- Standardized spacing in multiple files for improved readability. - Ensured proper newline at the end of files where missing. - Updated string interpolation syntax for consistency in the `format_days_remaining` method. - Enhanced validation inclusion syntax in `notify_emails_to` for clarity.
- Adjusted width of the notify emails locale menu for better responsiveness. - Improved the select dropdown controller to handle label updates more effectively. - Refactored the locale dropdown partial for better layout and accessibility, including consistent class usage and streamlined rendering of language options.
- Removed the separate locale partial and integrated its functionality directly into the notify emails partial. - Updated test cases to reflect changes in the partial structure and ensure proper rendering of the locale dropdown.
- Updated the way delivery errors are handled by directly setting `raise_delivery_errors` on the mail object. - Simplified logging to use `mail.to.size` for recipient count instead of `mail.message.to.size` for clarity.
pglombardo
reviewed
Mar 31, 2026
app/assets/stylesheets/custom.css
Outdated
| overflow-y: auto; | ||
| } | ||
| } | ||
|
|
Owner
|
pglombardo
reviewed
Mar 31, 2026
app/mailers/push_created_mailer.rb
Outdated
| locale = @push.notify_emails_to_locale.presence | ||
| I18n.with_locale(locale || I18n.default_locale) do | ||
| @secret_url = secret_url(@push, with_retrieval_step: @push.retrieval_step, locale: locale) | ||
| @subject = "#{@push.user&.email.presence || _("Someone")} #{_("has sent you a Push")}" |
pglombardo
reviewed
Mar 31, 2026
| end | ||
|
|
||
| # Enqueue job to send creation email if notify_emails_to is present. | ||
| # (OSS: always allow; Pro gates with account_is_premium?) |
Owner
There was a problem hiding this comment.
Don't need to mention Pro strategies in OSS
pglombardo
reviewed
Mar 31, 2026
| # background job processor (e.g. Solid Queue), which makes it easy to try the feature | ||
| # with Mailbin or similar. Tradeoff: job exceptions surface inline in the request rather | ||
| # than asynchronously. To get async behavior in development, use perform_later here and | ||
| # run your job backend; tests use perform_later and rely on ActiveJob test helpers. |
pglombardo
reviewed
Mar 31, 2026
| SendPushCreatedEmailJob.perform_now(id) | ||
| else | ||
| SendPushCreatedEmailJob.perform_later(id) | ||
| end |
Owner
There was a problem hiding this comment.
No needed - just use perform_later
pglombardo
reviewed
Mar 31, 2026
|
|
||
| <%= _('Please find the secret link below to access the sensitive information.') %> | ||
|
|
||
| <%= _('Secret link:') %> <%= @secret_url %> |
pglombardo
reviewed
Mar 31, 2026
| define_user_signed_in(true) | ||
| assert_not show_notify_emails_field? | ||
| end | ||
|
|
Owner
There was a problem hiding this comment.
Feature specific tests in feature specific test file
pglombardo
reviewed
Mar 31, 2026
|
|
||
| def define_user_signed_in(signed_in) | ||
| (class << self; self; end).define_method(:user_signed_in?) { signed_in } | ||
| end |
Owner
There was a problem hiding this comment.
Re-evaluate - even in tests user_signed_in? should be provided by devise.
- Removed the `select_dropdown_controller` as its functionality is no longer needed. - Simplified the locale dropdown rendering in the secret URL bar and notify emails partials. - Updated styles for better responsiveness and consistency across the application.
- Updated the PushCreatedMailer to use the new email parsing method and adjusted the subject line for consistency. - Enhanced the Push model by adding validations for notify_emails_to and notify_emails_to_locale, and included methods for email parsing and sending creation emails. - Removed the Pwpush::NotifyEmailsTo concern as its functionality has been integrated into the Push model. - Simplified email notification views to reflect the changes in the subject and removed unnecessary user email references. - Updated tests to align with the new structure and removed obsolete tests related to notify emails.
- Updated PushCreatedMailer to utilize the new Pwpush::NotifyEmailsTo module for email parsing. - Removed redundant email parsing and validation methods from the Push model. - Cleaned up the Push model by removing unused validations related to notify_emails_to and notify_emails_to_locale. - Adjusted the structure of the Push model to streamline email notification logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Updating the mailer to pass user for the subject
On create if
notify_emails_tois present the app enqueuesSendPushCreatedEmailJob, which sends one email to all given addresses with the secret link. Optionalnotify_emails_to_localesets the email language and the?locale=in the link.Related Issue
https://github.com/apnotic/pwpush-pro/issues/1448
Type of Change
Checklist
rake test)