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

Add CC/BCC fields for Sensei emails #7014

Merged
merged 13 commits into from
Aug 10, 2023
Merged

Add CC/BCC fields for Sensei emails #7014

merged 13 commits into from
Aug 10, 2023

Conversation

merkushin
Copy link
Member

@merkushin merkushin commented Jul 11, 2023

Resolves #7001

In this PR:

  • I moved adding our fields to Sensei_Settings class, to have only one place for all fields.
  • I used a custom type for the field ("email_list") and a corresponding validation methods.
  • These methods don't remove duplicated emails. Wasn't sure if we need it. Also it will affect validation in its current state.

Proposed Changes

  • Add CC/BCC fields for Sensei emails.

Testing Instructions

  1. Go to Sensei LMS -> Settings -> Email -> Settings.
  2. Try to set invalid emails for CC & BCC.
  3. It is expected to see an error message for each field.
  4. Set different emails for CC and BCC.
  5. Create a course with lessons.
  6. Take the course as a student and complete it.
  7. Check your emails. It is expected to get an email in accounts you set for CC & BCC. The email that was sent to BCC shouldn't have that address in recipients.

Deprecated Code

  • Sensei\Internal\Emails\Email_Settings_Tab::add_reply_to_setting – adding of these settings moved to \Sensei_Settings.

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

@merkushin merkushin added [Status] Needs Documentation Requires documentation updates Emails labels Jul 11, 2023
@merkushin merkushin self-assigned this Jul 11, 2023
@merkushin merkushin marked this pull request as ready for review July 11, 2023 22:14
@donnapep
Copy link
Collaborator

@merkushin Did you mean to add a reviewer to this or does it still need some work?

@merkushin merkushin requested a review from a team July 20, 2023 13:22
@merkushin
Copy link
Member Author

@donnapep oops, looks like I forgot to add reviewers🤦🏻‍♂️

@donnapep
Copy link
Collaborator

For validation, could we reuse the same validation strategy that is currently being used for the other email addresses for consistency's sake?

Screenshot 2023-07-24 at 8 00 27 AM

$fields['email_cc'] = array(
'name' => __( 'CC', 'sensei-lms' ),
'description' => __( 'Enter email addresses to CC on all emails. Separate multiple email addresses with commas.', 'sensei-lms' ),
'type' => 'email_list',
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we make the type email and add the multiple attribute, it should work with multiple email addresses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum, I'll try it. But I think that produces multiple fields, doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I found what you meant. Trying to use it.

@merkushin
Copy link
Member Author

@donnapep

For validation, could we reuse the same validation strategy that is currently being used for the other email addresses for consistency's sake?

I think it works only for a single email in the field. That's the browser validation that comes with the email input type. It is good to have it, but we should have a counterpart on the backend. Anyway, I'll try it with #7014 (comment)

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #7014 (49d83b6) into trunk (7dbccb4) will increase coverage by 1.07%.
Report is 1 commits behind head on trunk.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7014      +/-   ##
============================================
+ Coverage     48.07%   49.15%   +1.07%     
- Complexity    10467    10470       +3     
============================================
  Files           573      573              
  Lines         44164    44218      +54     
  Branches        402      402              
============================================
+ Hits          21234    21736     +502     
+ Misses        22603    22155     -448     
  Partials        327      327              
Files Changed Coverage Δ
includes/class-sensei.php 23.52% <ø> (ø)
includes/class-sensei-settings-api.php 15.47% <100.00%> (+3.84%) ⬆️
includes/class-sensei-settings.php 70.70% <100.00%> (+65.78%) ⬆️
includes/internal/emails/class-email-sender.php 99.02% <100.00%> (+0.03%) ⬆️
...ludes/internal/emails/class-email-settings-tab.php 85.82% <100.00%> (-12.41%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bf75ce...49d83b6. Read the comment docs.

@merkushin merkushin added this to the 4.16.1 milestone Jul 24, 2023
@merkushin
Copy link
Member Author

@donnapep I updated the PR.

m1r0
m1r0 previously approved these changes Aug 3, 2023
Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

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

Looks good and tests well. 👍

@m1r0
Copy link
Member

m1r0 commented Aug 4, 2023

Just curious, does it make sense to have tests for these fields?

@merkushin
Copy link
Member Author

Thanks @m1r0. Added tests.

Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

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

Thank you! 🙇

@merkushin merkushin merged commit b054cb7 into trunk Aug 10, 2023
20 of 21 checks passed
@merkushin merkushin deleted the add/email-cc-bcc branch August 10, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Emails [Status] Needs Documentation Requires documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Cc and Bcc email settings
3 participants