You might want to add goimports to your IDE settings to run automatically on save. I've found this helps me from having to go back later and reformat all the files.

Let's change this to validate instead of ping. This will match the pattern that we're doing with the IMAP additions here

Ok. But using verbs in the URL in REST API is discouraged. It's recommended to use nouns. It'd be "/validation"

I'd prefer to leave it validate. I understand that REST API semantics prefer for everything in the URL to be a resource, but to me validate reads cleaner in that we're validating a resource, not referring to a new validation.

I appreciate the suggestion, though! Keep up the great work.

Minor nit - I try not to use _'s in variable names where possible. In this case, whs would be fine.

You likely won't need this. Pinging a webhook could be API-only and a button on the webhooks page.

Please remove the go.sum and go.mod files before the final review. I'll add dependency management at some point, but that should be separate from the work here.

Ok

You'll want to include a Secret here as well to handle signing.

Same minor nit as above - whs would work fine here.

Super minor nit - I'd rename this to wh instead of whook. Keeps it cleaner and matches what you have in the models package above.

I'd make sign be a function off Webhook:

func (wh *Webhook) sign(data interface{}) (string, error) {}

Or something similar.

Can we change this placeholder to http://example.com/webhook or something to indicate what the field should look like?

fixed

You're missing the is_active column in this migration.

I'm not, you're looking at the old code.

Also, why are you using an integer field for is_active? We use boolean fields in migrations now and this seems like another good use case for that.

is_active is boolean now

I see. There're 2 migrations, in one of them it's missing

This is the latest code right? Like, the code right here that we're commenting on is for the SQLite migrations, and it doesn't have is_active. MySQL does, but SQLite doesn't.

SQLite does not have a separate Boolean storage class. Instead, Boolean values are stored as integers 0 (false) and 1 (true).

https://www.sqlite.org/datatype3.html

Sqlite doesn't support boolean, why are we using them in all the migrations for Sqlite?

It doesn't have a separate concept for booleans, but does support a boolean data type- it just represents them internally as integers:

sqlite> .schema webhooks
CREATE TABLE IF NOT EXISTS "webhooks" (
    "id" integer primary key autoincrement,
    "title" varchar(255),
    "url" varchar(1000),
    "secret" varchar(255),
    "is_active" boolean
);
sqlite> select * from webhooks;
1|Test Webhook|http://127.0.0.1:9999/webhook|mysecret|0

I use them both for clearer database structure, since it's clear that this is a true/false kind of field rather than an integer, as well as to try and avoid any possible weird issues with the underlying ORM, gorm.

Fixed

This can be cleaned up a bit by flipping the conditionals and removing the else:

if err != nil {
    JSONResponse(w, models.Response{Success: false, Message: err.Error()}, http.StatusBadRequest)
    return
}
JSONResponse(w, wh, http.StatusOK)

How will flipping the conditionals clean up the code? We expect the condition without an error first. And that's what I'm checking first.

And there have to exist 2 conditions here.

		err = webhook.Send(webhook.EndPoint{URL: wh.URL, Secret: wh.Secret}, "")
		if err == nil {
			JSONResponse(w, wh, http.StatusOK)
		} else {
			JSONResponse(w, models.Response{Success: false, Message: err.Error()}, http.StatusBadRequest)
		}

How do you say we could do this?

		err = webhook.Send(webhook.EndPoint{URL: wh.URL, Secret: wh.Secret}, "")
		if err != nil {
			JSONResponse(w, models.Response{Success: false, Message: err.Error()}, http.StatusBadRequest)
		} 

Return nothing in case of "no error"?

My recommendation is really just encouraging the pattern we use everywhere else in the code, which is also what Go itself recommends. That is, a pattern like:

if err != nil {
    handle the error
    return
}
continue execution

The reason this is encouraged is that it keeps the nesting to a minimum so we don't wind up with things like:

if err == nil {
    big block of code that happens if things go as planned
    if err == nil {
        another big block of code
        etc, etc
    } else {
        handle the error
    }
} else {
    handle the error
}

So in my proposal we're handling the error first, then if the error was nil, you'll find the JSONResponse(w, wh, http.StatusOK) at the end of the suggested code- this would be the default thing to run if no errors occurred. That's the way all of the other HTTP handlers work- this is the only one that has a different pattern when it doesn't need to.

Agree. Fixed

First, it seems like this doesn't work? You only run the body of the function if err != nil, so if there was an error.

It appears that this is the correct way to structure this:

if err == nil {
	for _, wh := range whs {
		whEndPoints = append(whEndPoints, webhook.EndPoint{
			URL:    wh.URL,
			Secret: wh.Secret,
		})
	}
	webhook.SendAll(whEndPoints, e
} else {
	log.Errorf("error getting active webhooks: %v", err)
}

Also notice the updated error message to give more context on what the error was. And in this case, it appears we're making a design decision to fail-open if the webhooks encounter errors. I'm ok with this for now.

fixed

On the other models validate is a function off the struct, and I think we should keep that pattern here. This would make it:

func (wh *Webhook) Validate() error {...}

fixed

Please move these errors to exported variables at the top of the file (see the other models/ files for examples, such as models/campaign.go)

fixed

Please move these errors to exported variables at the top of the file (see the other models/ files for examples, such as models/campaign.go)

fixed

We'll want to escape this to prevent xss. You can use escapeHtml for this.

We'll want to escape this to prevent xss. You can use escapeHtml for this.

If we're returning an error from the webhook ping, we should display it to the user here.

fixed

We have a way to make pretty checkboxes that I'd encourage you to use. You can find an example here.

fixed