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 e-mail sending information for e-mail node #891

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kazuhitoyokoi
Copy link
Member

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

The current e-mail node doesn't output e-mail sending information. Therefore, flow developers are not able to handle the process after sending e-mail using the complete node. For example, logging and sending notifications. Therefore, I added the handling to add the information.

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the forum/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@Steve-Mcl
Copy link
Contributor

Do you think it would be beneficial to also add an output to the email node? That way a flow msg can carry on and take appropriate actions based on the connected flow.

@dceejay
Copy link
Member

dceejay commented Mar 10, 2022

this is just duplicating the status output - Is it not possible to use that as-is ?

@Steve-Mcl
Copy link
Contributor

@dceejay thats partly why i suspect adding an output would be more beneficial.

@kazuhitoyokoi
Copy link
Member Author

kazuhitoyokoi commented Mar 10, 2022

@Steve-Mcl Output port of e-mail node is a simple way to solve the situation. But in my opinion, it is difficult for people to understand where is the end of the flow if additional flow follows the e-mail node.

@dceejay Yes, I found that the status node can be used to receive responses from the e-mail node. I taught about this tips in a Node-RED seminar but some people tried to use complete node. We discussed that the email node ideally sends the server response to the complete node.

@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Mar 10, 2022

@kazuhitoyokoi Regarding using the complete node to pass a msg further downstream is (graphically) not really a "flow". The complete node only has a non-visual connection to the source node - i.e. it is not very "flow like". It also means you need 1 complete node for every email node (if you want to understand where the msg came from)

If you believe that to be a problem, the output could always be optional (defaulted to current state of 0 outputs)

Please note: this is only my opinion. The decision will be made between yourself and Dave :)

@NetHans
Copy link

NetHans commented Oct 7, 2022

I think it makes more sense to add an output to the node. There are situations where the flow of a flow depends on whether an email could be sent successfully. In such cases, the part of the flow after the e-mail node must not run until the e-mail has been sent successfully. If I understand the proposed change correctly, the flow would continue at a different point without keeping reference to the flow itself. For the evaluation of status messages (logging etc.) this would certainly be ok. But not if the actual flow relies on the info whether an email has been sent successfully.

The extension by an output should be considered again here.

@hardillb
Copy link
Member

hardillb commented Oct 7, 2022

@NetHans but just because an email has been accepted by the mail server is no indication that it will actually be sent to the recipient

@NetHans
Copy link

NetHans commented Oct 7, 2022

@hardillb That's right. An e-mail always has the problem, due to the lack of a confirmation of receipt by the destination server, that you cannot tell whether it has really been delivered to the recipient.

The only indication of a halfway assured delivery can only be provided by the SMPT server. If I have understood the Change here correctly, exactly this information is collected and can be used for a decision. Of course, this is not 100% sure. But it is a clue to decide in the flow how to proceed.

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.

5 participants