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

Send Update Notifications Only Option Broken #338

Closed
NRay7882 opened this issue Jan 2, 2020 · 29 comments
Closed

Send Update Notifications Only Option Broken #338

NRay7882 opened this issue Jan 2, 2020 · 29 comments
Labels
bug Something isn't working stale issue marked as old news

Comments

@NRay7882
Copy link

NRay7882 commented Jan 2, 2020

Describe the bug
Using v0.80.67, one of the new features in Main Settings is Notifications. Attempting to use this module by enabling it breaks the default configuration and leaves the feature disabled.

To Reproduce
Steps to reproduce the behavior:

  1. From the Statping instance, open the Settings > Main Settings > Notifications page, URL ending in /settings#v-pills-notifications.
  2. Move the "Send Updates Only" slider to the right by clicking on the button.
  3. Clicking the "Save Settings" button immediately reloads the Main Settings page with the Description, Domain, Custom Footer and Timezone set to default values.

Expected Behavior
The setting should stay enabled and allow the sending of notifications only when the status of a services changes.

Screenshots
Screen Shot 2020-01-02 at 5 15 48 PM

Gitter GitHub release Build Status

@maxkratz
Copy link

maxkratz commented Jan 3, 2020

@NRay7882 I can absolutely understand the behaviour. This also happens with my installation. I also tried a fresh install to eliminate config errors.

@MFYDev
Copy link

MFYDev commented Jan 13, 2020

Same issue happenes on me. I am using 0.80.69
@hunterlong Please have a check on this. Really weird.

@MFYDev
Copy link

MFYDev commented Jan 13, 2020

@hunterlong Hi there, I noted that you have already reply to many other issues which were reported later than this one. I really do not know why you ignore this issue.
Would you please give some feedback?
Just tried on 0.80.70 and this still happenes.

@hunterlong hunterlong added the bug Something isn't working label Jan 13, 2020
@rogierlommers
Copy link

Same problem here; running Statping 0.80.71.

@MFYDev
Copy link

MFYDev commented Jan 18, 2020

@rogierlommers Seriously? 0.80.71? The latest is 0.80.70, isn't it?

@rogierlommers
Copy link

@rogierlommers Seriously? 0.80.71? The latest is 0.80.70, isn't it?

No, I'm running the latest docker stable tag; it's giving me 0.80.71. See screenshot below.

Screenshot 2020-01-18 at 19 57 02

@MFYDev
Copy link

MFYDev commented Jan 19, 2020

@rogierlommers Wow I did not expect that docker has the newer version. I am always using the binary version. You make me wanna change to the docker version HAHA 😂

@jamiew0w
Copy link

Can confirm this is still happening. It also wipes out the custom footer along with the timezone settings so if you toggle it and restart the service you need to reset them up. It's not saved to the config.yml

@hunterlong Could we have variables in the config.yml to hard-define footer/timezone?

Setting domain: http://blah in the config.yml has stopped that part being reset so the above seems like a logical fix.

@l0nax
Copy link
Collaborator

l0nax commented Jan 29, 2020

I'm the developer of this Issue.
So I hope I can help you fixing this Issue.

First of all can you guys please send the Error/ Debug log which the Server produces?
I will try to reproduce the Error when I'm at work.

@l0nax
Copy link
Collaborator

l0nax commented Jan 29, 2020

@hunterlong Could we have variables in the config.yml to hard-define footer/timezone?

Why would you like to "hard code" this Code?
If you think that this will fix the problem then this is more like a lightweight workaround, because it does not really fix the Issue which produces this error.

@jamiew0w
Copy link

Oh for sure, it's just a workaround but it does solve #3 as these settings aren't persistent at the moment.

@l0nax
Copy link
Collaborator

l0nax commented Jan 29, 2020

What has this to do with Issue #3? Since it handles about the Plugin System/ Documentation.

@schemen
Copy link

schemen commented Jan 30, 2020

I can also confirm that this happens. Also it seems to mess with the groups: I cannot create new ones:

statup | ERRO[0407] template: main:38:116: executing "content" at <.GroupId>: nil pointer evaluating *core.Group.Name type=handlers

Using the latest docker image

@l0nax
Copy link
Collaborator

l0nax commented Jan 30, 2020

@schemen can you also please provide your (debug) logs?

This would be really helpful.

@schemen
Copy link

schemen commented Jan 30, 2020

@l0nax Could you elaborate how to enable debug logs? is it via Env variable VERBOSE? a certain level?

@kuetemeier
Copy link

Hi,

I do not have much time to provide a direct patch, but these types of problems usually have the same pattern. So I took a quick look at this.

As I thought both forms call the same action settings:

In source/tmpl/settings.gohtml

Line 38: <form method="POST" action="settings">
Line 168: <form method="POST" action="settings">

And if I'm right the handler is in handlers/settings.go

After the line 39:

func saveSettingsHandler(w http.ResponseWriter, r *http.Request) {
	var err error
	form := parseForm(r)
	app := core.CoreApp
	name := form.Get("project")
	if name != "" {
		app.Name = name
	}
	description := form.Get("description")
	if description != app.Description {
		app.Description = description
	}
	style := form.Get("style")
	if style != app.Style {
		app.Style = style
	}
	footer := form.Get("footer")
	if footer != app.Footer.String {
		app.Footer = types.NewNullString(footer)
	}
	domain := form.Get("domain")
	if domain != app.Domain {
		app.Domain = domain
	}
	timezone := form.Get("timezone")
	timeFloat, _ := strconv.ParseFloat(timezone, 10)
	app.Timezone = float32(timeFloat)

	app.UpdateNotify = types.NewNullBool(form.Get("update_notify") == "true")

	app.UseCdn = types.NewNullBool(form.Get("enable_cdn") == "on")
	core.CoreApp, err = core.UpdateCore(app)
	if err != nil {
		log.Errorln(fmt.Sprintf("issue updating Core: %v", err.Error()))
	}

	//notifiers.OnSettingsSaved(core.CoreApp.ToCore())
	ExecuteResponse(w, r, "settings.gohtml", core.CoreApp, "settings")
}

There is the Error! When e.g. the "notification" form (line 168 in the template) calls the handler, the fields for "description" is not there... it's empty

and the code

	description := form.Get("description")
	if description != app.Description {
		app.Description = description
	}

reads:

	description := "" // is empty
	if "" != app.Description {
		app.Description = ""
	}

And that's why it gets deleted.

Quick solution: use a second handler for the notification page, e.g. "settings2".

Hope that helps. May be I find some time for a patch in the next week. But if someone should be faster than me, I would like that very much.

@ksarnelli
Copy link

Any ETA on a fix for this or is there a workaround?

@kuetemeier
Copy link

@ksarnelli As you can see above I opened a pull request with a bug fix patch. Don‘t know why it takes so long to merge it. Didn‘t get a response from the maintainers yet.

@hunterlong could you please take a view on this. My Mailboxes get flooded with status messsges. We need this (or another) fix as soon as possible. Many thanks in advance.

@MFYDev
Copy link

MFYDev commented Feb 13, 2020

@kuetemeier To be honest, I sent him an email once, and he never reply to me. The issues I reported also like this. Really do not know why these take so long. My mailbox also flooded with the status messages. Thank you in advance for a click to merge. @hunterlong

@thedmeyer
Copy link

Any update on merging #361 ?

@kuetemeier
Copy link

nope... no response

@ksarnelli
Copy link

I also asked in the Slack channel over a week ago and got no response there either.

@github-actions
Copy link

This issue hasn't had any updates in a while. If this is still a problem, please create a new issue.

@github-actions github-actions bot added the stale issue marked as old news label Apr 23, 2020
@l0nax
Copy link
Collaborator

l0nax commented Apr 28, 2020

This is not stale.

@l0nax l0nax reopened this Apr 28, 2020
@l0nax l0nax removed the stale issue marked as old news label Apr 28, 2020
@github-actions
Copy link

This issue hasn't had any updates in a while. If this is still a problem, please create a new issue.

@github-actions github-actions bot added the stale issue marked as old news label May 29, 2020
@NRay7882
Copy link
Author

Using the option to "Only notify one time when service hits an error" causes no down / offline notifications to be sent to Slack, so unless I constantly check the dashboard vs the Slack alerts I cannot tell which services are actually down.

Referenced in #470

@github-actions github-actions bot removed the stale issue marked as old news label May 31, 2020
@NRay7882
Copy link
Author

Currently running v0.90.54, while the original issue of the Settings UI becoming blank is no longer an issue, the feature itself still has a problem with sending Slack notifications. As previously mentioned, any of our services configured to use Only notify one time when service hits an error are not producing Offline slack alerts. We only receive the online notifications:

Screen Shot 2020-06-22 at 11 10 36 AM

Screen Shot 2020-06-22 at 11 11 56 AM

@SlothCroissant
Copy link

I can confirm this behaves the same way with Pushover.

@github-actions
Copy link

This issue hasn't had any updates in a while. If this is still a problem, please create a new issue.

@github-actions github-actions bot added the stale issue marked as old news label Aug 18, 2020
@github-actions github-actions bot closed this as completed Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale issue marked as old news
Projects
None yet
Development

No branches or pull requests