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

Issues/611 - Email Enhancement #1673

Merged
merged 416 commits into from
May 15, 2017
Merged

Conversation

ravinderk
Copy link
Collaborator

Description

This PR will fix #1346

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@ravinderk
Copy link
Collaborator Author

@DevinWalker Currently, you will not able to see recipients setting in form meta box. The new form API support repeater field in form meta box. I will fix that after merging this to release/2.0

@DevinWalker DevinWalker changed the title Issues/611 Issues/611 - Email Enhancement May 10, 2017
@DevinWalker DevinWalker self-requested a review May 10, 2017 04:59
@DevinWalker
Copy link
Member

@mathetos can you get your eyes on this for me and give it a thorough test with core, per-form emails, recurring and other add-ons that modify or customize emails?

@mathetos
Copy link
Member

Looks really great and I love where this is going. I ❤️ that we'll be able to customize the Pending Donation email, the Donor Register email and many more that we never could easily before. That's a MAJOR bonus for our users.

Most likely I think we just need to merge and then iterate from here. but here's a few initial comments:

GENERAL COMMENTS/TWEAKS

  1. Tooltips are using qtip which is gone already. Don't we want them to be in hint.css format now?
  2. If you send a Test Pending Donation Email, the link doesn't go to any specific email. Is that intended, or should we make sure it goes somewhere?
  3. Not a fan of how the headings look:
    image
  4. I think we need a hover effect and maybe a tooltip over the green checkmark to indicate that the email can be enabled/disabled with a click. I see the tooltips for the one's that can't be disabled, but a hover effect on those that CAN be enabled/disabled would help, as would a tooltip.
  5. Margin/padding need to be worked out a bit on the columns. This can definitely come later though:
    image
    But as a reference:
td.name.column-name[data-colname="Email"] {
    padding: 13px 10px 0px 20px;
}
span.give-email-notification-status.dashicons {
    margin-left: 10px;
    margin-right: 10px;
}

BUGS

  • Saving Email Bug:
    PHP Notice upon refresh after saving the "New Pending Donation" email:
    Notice: Undefined index: email-access_notification in ...\wp-content\plugins\give\includes\admin\emails\class-email-access-email.php on line 263

  • FFM Email Tag Bug:
    The {all_custom_fields} email tag doesn't render in the email (maybe I'm not on the right branch or commit locally for that to work).

Here's a few things I tested, just to confirm:

  • I tested all the "Send Test Email" and "Preview" links -- good to go.
  • I tested editing all templates -- good to go.
  • I tested disabling emails and checking whether they were still sent -- good to go.

@ravinderk
Copy link
Collaborator Author

@mathetos Check updates

GENERAL COMMENTS/TWEAKS

  1. As I discussed with @DevinWalker, we will fix that in Bye bye Qtip2 and hello Hint.css Tooltips #1596
  2. Did you check email send notice or MailCatcher
    he67s1e1fx
  3. @DevinWalker will take care of this.
  4. Fixed (good suggestion).
    notification-status
  5. Fixed.

Bugs

  1. Fixed
  2. I will create separate PR in each addon which are defining email tags.

@mathetos
Copy link
Member

@ravinderk Looks good. Let me clarify #2 though. The email sends correctly, but the link inside the email doesn't lead to any specific page. If they test that email and click on that link I think it just goes to a 404. I'm not sure whether we can have it go somewhere more obvious or actionable, but it makes testing that email less effective.

@DevinWalker DevinWalker merged commit ed59d5e into impress-org:release/2.0 May 15, 2017
@ravinderk
Copy link
Collaborator Author

@mathetos In test email whatever content you see ( for example new donation email ), it is real content but email tag values are only dummy values which give you feel of the actual email. Email tag dummy values can be set by the developer.
Most of the time, to decode email tag we need an actual environment, that is why we have dummy values for them.

For example, receipt email tag will have a correct link only if a donor donates something.

@mathetos I agree with you that this will make testing email less effective for testing. But before making donation form live they can process donation from their end first by this way they will get an actual email.

@DevinWalker What is your views on that.

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.

None yet

3 participants