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

Werkzeug für die aktuell eingestellten Mailvorlagen #19

Merged
merged 10 commits into from
Oct 7, 2017

Conversation

schnumsel
Copy link

Hallo Matthias,

also, vielleicht mal zum drüberschaun. Habe das Modul so erweitert, dass unter System -> Tools ein E-Mail-Preview Tool existiert, mit dem man die aktuellen Mails anschaun kann, also auch solche, die z.B. mit Yireo-E-Mail-Override aus Templates und nicht aus der DB erzeugt werden.

Viele Grüße,

Willi

schnumsel and others added 7 commits January 17, 2017 10:57
To be able to deploy it
…lace

# Conflicts:
#	app/code/local/Bodenbelag/Marketplace/Model/Dropship/Vendor/Statement.php
#	app/code/local/Bodenbelag/Marketplace/Model/DropshipPaypalAdaptive/Adaptive.php
#	app/code/local/Bodenbelag/Marketplace/Model/Observer.php
#	app/code/local/Bodenbelag/Marketplace/etc/config.xml

Moving additional Templates from bodenbelag/transaction-mails to bodenbelag/theme (using magento-snm/transaction-mails instead)
…lace

# Conflicts:
#	app/code/local/Bodenbelag/Marketplace/Model/Dropship/Vendor/Statement.php
#	app/code/local/Bodenbelag/Marketplace/Model/DropshipPaypalAdaptive/Adaptive.php
#	app/code/local/Bodenbelag/Marketplace/Model/Observer.php
#	app/code/local/Bodenbelag/Marketplace/etc/config.xml

Moving additional Templates from bodenbelag/transaction-mails to bodenbelag/theme (using magento-snm/transaction-mails instead)
…lace

# Conflicts:
#	app/code/local/Bodenbelag/Marketplace/Model/Dropship/Vendor/Statement.php
#	app/code/local/Bodenbelag/Marketplace/Model/DropshipPaypalAdaptive/Adaptive.php
#	app/code/local/Bodenbelag/Marketplace/Model/Observer.php
#	app/code/local/Bodenbelag/Marketplace/etc/config.xml

Moving additional Templates from bodenbelag/transaction-mails to bodenbelag/theme (using magento-snm/transaction-mails instead)
composer.json Outdated
@@ -1,5 +1,5 @@
{
"name": "magento-hackathon/e-mail-preview",
"name": "schnumsel/e-mail-preview",
Copy link
Member

Choose a reason for hiding this comment

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

I think this should still be magento-hackathon.

Copy link
Author

Choose a reason for hiding this comment

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

Of course, your right. I did this, not knowing, i could define another branch to fetch the schnumsel stuff via composer.

.gitignore Outdated
@@ -0,0 +1 @@
# Created by .ignore support plugin (hsz.mobi)
Copy link
Member

Choose a reason for hiding this comment

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

This file should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

Well, I like to have .gitignore in order not to accidentially commit .idea folder. How do you guys handle this? Via IDE-Settings?

Choose a reason for hiding this comment

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

My PhpStorm project root is never the Git repository itself.

Also helpful: http://stackoverflow.com/questions/5724455/can-i-make-a-user-specific-gitignore-file

Copy link
Member

Choose a reason for hiding this comment

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

Personally I would consider a user specific .gitignore bad practise, who cares about a couple of extra lines in a .gitignore file. Not that there aren't use cases for it, but generally speaking.

@schnumsel The current .gitignore is effectively empty though, might as well not have it.

Copy link
Member

Choose a reason for hiding this comment

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

The .idea folder is in the project root, the extension is usually in a subfolder. Hence, no need for such a .gitignore. Additionally, as said by @edannenberg, it is empty anyway and can/should be removed.

@mzeis
Copy link
Member

mzeis commented Sep 2, 2017

Hi Willi,

thank you for your contribution!

If you still would like this to be merged please can you improve the things mentioned by Simon and push again to your branch so I can have a look?

@bambamboole
Copy link

@schnumsel It would be really nice if you could improve the things because the feature is super useful!

@schnumsel
Copy link
Author

Well, there it is. Sorry I did the changes in the master-branch not in the dev-willi-branch. Well finally it is done.
Best regards,
Willi

Copy link
Member

@mzeis mzeis 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 very much for updating!

I reviewed your PR and found one problem causing the new functionality to break.

Please let's also discuss whether we should implement the open todos marked in the code or remove the comments. As I don't use this extension in any project anymore I might not finish these todos myself. :-)

<method>hackathonEmailpreviewRenderEmailBefore</method>
</hackathon_emailpreview_contact>
<hackathon_emailpreview_admin_password_forgot>
<class>hackathon_emailpreview/mail_type_adminPasswordForgot</class>
Copy link
Member

Choose a reason for hiding this comment

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

This class is missing in the PR, causing a warning (and therefore error in developer mode). Please can you add it?

Copy link
Author

Choose a reason for hiding this comment

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

I'll try to check it up these days, maybe next friday since I have a day off...

return $this;
}

//@todo change logic here
Copy link
Member

Choose a reason for hiding this comment

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

What would be necessary to get this todo done (question also applies for the other @todo comments)?

Copy link
Author

Choose a reason for hiding this comment

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

same here. quite a long time, since i changed the code :)

@mzeis mzeis self-assigned this Sep 10, 2017
…ing, only the reference was wrong)

Implemented Oauth and System Errors Mails (got rid of the todos)
Removed product alert in favor of the existing product alert price and product alert stock observer classes.
@schnumsel
Copy link
Author

So hello again,

I promised Friday and I managed it (well 12 Minutes left :)
Hope that's it. Haven't delt a long time with this module..

Best regards,

Willi

@mzeis
Copy link
Member

mzeis commented Sep 15, 2017

Thank you, that's commitment! :-) I will be away for two weeks but I'll do the review afterwards.

@mzeis mzeis changed the base branch from master to dev October 7, 2017 11:37
@mzeis mzeis mentioned this pull request Oct 7, 2017
@mzeis mzeis merged commit 88bac2d into magento-hackathon:dev Oct 7, 2017
@mzeis mzeis added this to the 1.1.0 milestone Oct 7, 2017
@mzeis
Copy link
Member

mzeis commented Oct 7, 2017

Thank you again very much for your contribution! The changes are now in the develop branch.

Had to do some dirty changes to get it working with the dev branch quickly but will clean up a bit before releasing the new version.

@schnumsel
Copy link
Author

Well, unfortunately the dirty hacks did actually break something. My "Hackathon_EmailPreview_Block_Adminhtml_Email_Preview" is quite different compared to the final one.
I am missing specially the Increment ID Field, no order testing without.
How could we proceed?
Best regards, Willi

@mzeis
Copy link
Member

mzeis commented Oct 10, 2017

I'm sorry about the Increment ID field - I fixed this now in dcb8ac8. The other PR introduced dependencies between fields, hiding the form element.

My "Hackathon_EmailPreview_Block_Adminhtml_Email_Preview" is quite different compared to the final one.

Actually that's not because of the "hack" (the one I meant I already removed), that's because the master branch you branched from was quite different from the dev branch where other PRs had been merged before.

Of course, the goal should be to provide a stable v1.1.0. Even if the code is not perfect existing functionality shouldn't break though of course, that's important.

For a future release, it might make sense to do more refactoring, maybe drop the dependency on PHP 5.3 to allow to profit from new languages feature. As I honestly can't spare much time for this I cannot give a guarantee when I might do 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.

7 participants