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

Additional Outlook Versions, Yahoo Replies, Better support for multi-client reply chains, and Dependency Update #15

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

acterry
Copy link

@acterry acterry commented Apr 15, 2020

Changes

Selector Changes

  • Added support for Outlook 2007/2010 American and Outlook 2013/16/19 American and International
    • changes based on selectors removed from talon when they switched to regexes in this commit
  • Made all querySelectorAll queries case-insensitive since I found some lowercased hex color codes in some outlook emails in my data

fixed zero-based array issue in findOutlookSplitterWithQuerySelector code

  • this may have undone something that was intentional, since the original outlook-mixed test case didn't have a new message it in., just a forward and some replies
  • code would fail detection unless at least two splitters were found and it would leave the first quoted message in the result.
  • corrected outlook-mixed test case based on changes. Previously sample message only included a forward and several replies

Added new cut function to handle replies using the yahoo_quoted class

Add support for removing Yahoo replies that use the yahoo_quoted class

Handle reply chains among multiple clients by simply running all cut function

Removing replies from HTML messages is based on executing a list of email client-specific cut functions. Previously the code would stop after the first client-specific cut function found something to remove.

This didn't work well when the message had several replies in it and the cut function only removed a portion of the reply chain.

I'm simply running all of the cut functions -- not stopping after the first hit. This comes with a performance penalty, but still allows each cut function to continue to have freedom of implementation -- instead of forcing them to report back the DOM position and using centralized code to remove signature elements.

Updated all dependencies to latest stable, except for Mocha.

  • Stopped Mocha at latest version of 6.x since 7.0 drops Node v6 support and I'm not sure what versions planer needs to support.

I'm running Node 12 LTS on my dev box. I didn't check, but I imagine that some other dependency that I upgraded has likely dropped Node 6 support. Not sure if that's important or not.

acterry and others added 18 commits April 13, 2020 06:05
…add debug npm command to attach to chrome devtools for debugging
… code was fail detection unless at least two splitters were found and it would leave the first quoted message in the result.
Add Support for Additional Versions of Outlook and Dependency Update
…functions on each message instead of prioritizing order
…s-clients

handle reply chains among multiple clients by simply running all cut …
Detect replies using the yahoo_quoted class
@acterry
Copy link
Author

acterry commented May 8, 2020

@l8on, @eshenfield, @jylauril,

I hope all of you are doing well. Is this project still being maintained?

I've added a few more enhancements in my fork after issuing this PR a few weeks back.

@l8on
Copy link
Contributor

l8on commented May 10, 2020

Hey @acterry thanks for the ping.
I will take a look in more detail later this week, but on first glance one blocker would be the upgrade to coffeescript@2.

If you could downgrade the coffeescript back to the latest 1.x.x version, we would happily include your improvements in the published version after a successful Pull Request review.

@acterry
Copy link
Author

acterry commented May 11, 2020

@l8on,

Thanks for the reply. No problem, I’ll take a look at that downgrade.

I added a few other features to my fork since issuing this PR a few weeks back.

If you’re interested in considering all of these, I can downgrade my form to Coffeescript 1.x.x and update this PR.

@acterry
Copy link
Author

acterry commented May 11, 2020

Actually, this PR already has all of my subsequent changes in it since the request is to merge my master into your master. When I initially did it, I wasn't planning any more additions.

If you want, I can close this PR and issue a new one from the branch specific to this change.

In the meantime, I'm going to update the description of this PR.

@acterry acterry changed the title Add Support for Additional Versions of Outlook and Dependency Update Additional Outlook Versions, Yahoo Replies, Better support for multi-client reply chains, and Dependency Update May 11, 2020
@l8on
Copy link
Contributor

l8on commented May 11, 2020

@acterry Don't worry about issuing a new PR, and thank you for updating the description.

To respond to some info in your description:

I'm running Node 12 LTS on my dev box. I didn't check, but I imagine that some other dependency that I upgraded has likely dropped Node 6 support. Not sure if that's important or not.

We need to support Node 8+ with planer.

This comes with a performance penalty, but still allows each cut function to continue to have freedom of implementation -- instead of forcing them to report back the DOM position and using centralized code to remove signature elements.

I am a little concerned about this performance hit, but appreciate the improved completeness of the solution. Given this change in behavior, we may consider a major version bump for these changes such that we don't surprise anyone with bigger email processing bills.


Once the coffeescript version is downgraded, I look forward to taking a deeper look and testing it out myself.

@acterry
Copy link
Author

acterry commented May 11, 2020

@l8on , thanks for the comments.

I'll switch my environment to the latest 8.x Node version, make the necessary changes, and switch to CoffeeScript 1.x.x.

I'll let you know when I'm done. It should be sometime this week.

@acterry
Copy link
Author

acterry commented Feb 27, 2021

@l8on , sorry for the huge delay on this one. It’s on my list to resolve.

just want to make sure that Coffeescript 1.x and Node 8 are still the targets.

@l8on
Copy link
Contributor

l8on commented Mar 1, 2021

@acterry Yup, Coffeescript 1.x and Node 8+ are still the targets for this module.

@felixfbecker
Copy link

The fixes in this PR would be really awesome to have!

@acterry
Copy link
Author

acterry commented Jan 17, 2024

@felixfbecker , thanks for the reminder. It fell off my radar long ago.

However, I still use the code from my fork and it would be nice to have them upstream -- assuming that the maintainers are still interested in them.

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.

3 participants