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

Fix #1635 - Escape underscores in packages for markdown #2243

Closed

Conversation

robertmuehsig
Copy link
Contributor

Should fix this issue #1635

I tested it with this package https://www.nuget.org/packages/JQueryFileUpload_Demo_with_Backload/

Now he ReportPackageRequest creates such an MailBody with correct escapted underscore. Poor mans solution, but should work.

Email: admin (...)

Package: JQueryFileUpload_Demo_with_Backload
http://nuget.localtest.me/packages/JQueryFileUpload\_Demo\_with\_Backload/

Version: 1.9.2.4
http://nuget.localtest.me/packages/JQueryFileUpload\_Demo\_with\_Backload/1.9.2.4

User: admin (...)
http://nuget.localtest.me/profiles/admin

Reason:
The package was published as the wrong version

Message:
test

Message sent from NuGet Gallery


Only problem: The escaped PackageID is now also displayed in the mail subject - but I guess this is a very rare case and with this PR the link and the ID is correct in the mail body.

@jeffhandley
Copy link
Member

Oh, cool! There was also #1979, which was closed because we called it out of scope. But then there's also #1539 too, which is still open.

The underlying issue for #1979 and #1539 is that we always send Markdown-rendered emails, but some users can only receive plain text. And yes, they've reported it to us as a bug.

The fix you've made here will arguably make plain text messages a bit worse. Would you be open to augmenting this fix to also do the following?

  1. Create 2 copies of all messages we send: Markdown and Plain Text
  2. Modify our email sending code to send a multi-part message, with both HTML and Plain Text formats

This would probably require ditching MarkdownMailer, but that's fine with us.

@robertmuehsig
Copy link
Contributor Author

Sure - as far as I know the MarkdownMailer converts the markdown to html, right? So I would remove the MarkdownMailer and use another component for Markdown to HTML translation and also provide a pure Plain Text mail and send both as multipart message, right? Just to make sure I understand everything.

Is there currently a Markdown renderer included in the project? If not: Which one do you prefer?

@analogrelay
Copy link
Contributor

I think you are correct. MarkdownSharp is probably what we would prefer for now, as it's what MarkdownMailer used.

Thanks!

Moved MailSender stuff from the Markdown Mailer into the project.
@robertmuehsig
Copy link
Contributor Author

I got some time to work an this issue. Hope I find some more time on the weekend for the desired functionality.

@robertmuehsig
Copy link
Contributor Author

Sorry for being late. Current situation: I removed the MarkdownMailer Package, but I included the Source Code from @half-ogre.
My plan is to extend the IMailSender Send-Method. Current implementation:
void Send(
string fromAddress,
string toAddress,
string subject,
string markdownBody);

Idea:
void Send(
string fromAddress,
string toAddress,
string subject,
string plaintextBody
string markdownBody);

And then add the needed pure PlainText mailings, which can be totally different to the markdown => html mailing

I'm not an email expert, but currently there should already be a plain text mailing - the pure text in markdown syntax should be there as plaintext:

        AlternateView textView = AlternateView.CreateAlternateViewFromString(
            markdownBody,
            null,
            MediaTypeNames.Text.Plain);
        mailMessage.AlternateViews.Add(textView);

        AlternateView htmlView = AlternateView.CreateAlternateViewFromString(
            htmlBody,
            null,
            MediaTypeNames.Text.Html);
        mailMessage.AlternateViews.Add(htmlView);

Should I continue and add the plaintext parameter?

@dnfclas
Copy link

dnfclas commented May 21, 2015

@robertmuehsig, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@robertmuehsig
Copy link
Contributor Author

Is this pull request still interesting?

@yishaigalatzer
Copy link

@xavierdecoster can you please review?

@304NotModified
Copy link
Contributor

Bump!

@yishaigalatzer
Copy link

@qianjun22 this is an ancient PR, can we please figure out if we want it and merge ?

@skofman1
Copy link
Contributor

Closing this PR since it's very old and has a lot of merge conflicts.

@skofman1 skofman1 closed this Dec 30, 2016
@robertmuehsig
Copy link
Contributor Author

Is this something that is still a problem? I could create a new PR.

@skofman1
Copy link
Contributor

This: #1635 is still open, and I couldn't find any indication in the code that it was.. a new PR will be great.

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

7 participants