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

Include the umbracoApplicationUrl Host in the Healthcheck email notifier #3595

Merged
merged 5 commits into from Nov 27, 2018

Conversation

Projects
None yet
5 participants
@darrenferguson
Copy link
Contributor

commented Nov 12, 2018

Include the umbracoApplicationUrl Host in the Healthcheck email notifier as when you have multiple sites it is difficult to identify which site that the results are for.

Prerequisites

  • I have added steps to test this contribution in the description below:
  1. Set umbracoApplicationUrl correctly in umbracoSettings.config
  2. Enabled email notification in HealthChecks.config
  3. Setup SMTP details correctly in Web.config
  4. Start umbraco solution
  5. Received email (screenshot attached) - with modified subject line.

Excerpt from HealthCheck.config looks like:

Ensure that the firstRunTime attribute is blank for testing.

Note, the notification isn't set right away on startup - it runs on a background scheduled task so may take a minute or two to trigger.

If there's an existing issue for this PR then this fixes: N/A

Description

Include the umbracoApplicationUrl Host in the Healthcheck email notifier as when you have multiple sites it is difficult to identify which site that the results are for.
untitled

in the appropriate config/lang.xml file the key scheduledHealthCheckEmailSubject can be modified to contain a token for the host that the healthcheck is running on e.g.:

"Umbraco Health Check Status : {0}"

Also - if you don't want the host in the email subject - you can just remove the token "{0}".

darrenferguson added some commits Nov 12, 2018

Include the umbracoApplicationUrl in the Healtcheck email notifier - …
…as when you have multiple sites it is difficult to identify which site that the results are for
Include the umbracoApplicationUrl in the Healtcheck email notifier su…
…bject - refine previous commit to only include hostname - as some email services don't like URLs in subjects. Also update the language files to have a token for the host so that it could be removed if you didn't want it there for some reason.
@ronaldbarendse

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2018

You should get the application URL from ApplicationContext.Current.UmbracoApplicationUrl (but avoid the singleton if you have a reference to the application context), because if the setting isn't defined, it will be automatically be determined:

public string UmbracoApplicationUrl

Also, instead of the try/catch, use Uri.TryCreate() and maybe fall back to just use the whole URL instead of nothing? That way, the subject won't end with a colon... And maybe remove the space before the colon, so it becomes: Umbraco Health Check Status: {0} or put it in parentheses, e.g. Umbraco Health Check Status ({0}).

@darrenferguson

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

@ronaldbarendse thanks for the feedback, i've made a further commit - taking the comments into consideration.

@nul800sebastiaan nul800sebastiaan changed the title Include the umbracoApplicationUrl Hosy in the Healthcheck email notifier Include the umbracoApplicationUrl Host in the Healthcheck email notifier Nov 20, 2018

@poornimanayar

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

Hi @darrenferguson ,

First of all, apologies for the delayed acknowledgement. One of us shall give this a test and let you know how it goes! Thanks a lot for the work. We shall keep you posted.

Regards,
Poornima, Community Pull Request team

Small corrections
Use var instead of types, only log in Debug mode else you'll get too many warnings in the log, tokenize localized string correctly.

@ghost ghost assigned umbracoci Nov 27, 2018

@nul800sebastiaan nul800sebastiaan merged commit 4e887b8 into umbraco:dev-v7 Nov 27, 2018

0 of 2 checks passed

Cms Continuous in progress
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Thanks @darrenferguson ! I've made some additional changes but this is all good, in order for notifications to work, you must set the umbracoApplicationUrl in web.config anyway, so this should always work just fine.

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

@darrenferguson One tip: in your repository, the next time you do a PR, make sure to start a branch first. Right now you're stuck because I squashed and merged these commits.

The problem at the moment is that you've already put some commits in dev-v7 in your fork, and our repo now is different. So I would recommend:

  • Delete your fork
  • Delete your local clone of the fork
  • Re-fork Umbraco into your GitHub account
  • Clone the new fork

Now you're all clean, for the next time you start doing something, start a new branch (we usually just name it temp-something). Then you can commit, push and send a PR. While you wait for us to merge, you can switch back to dev-v7, start a new branch and work on something else.

Even better is if you switch back to dev-v7 and then sync with the main repository first: https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/.github/CONTRIBUTING_DETAILED.md#keeping-your-umbraco-fork-in-sync-with-the-main-repository

@darrenferguson

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

@nul800sebastiaan uggh "var" :)

Thanks for the detail. Yes - sorry, absolutely should have made a branch first. For some reason, I couldn't find those branching guidelines!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.