From 2f6bd05ca0fc1d5070114df03544b184aa870017 Mon Sep 17 00:00:00 2001 From: Kailash Nadh Date: Sat, 13 Feb 2021 17:55:10 +0530 Subject: [PATCH] Fix the optin-in, form re-subscribe behaviour. If a user is already subscribed to an optin list but hasn't confirmed, subscribing using the same e-mail id from the public form now re-sends the optin e-mail while also showing an appropriate message on the frontend rather than just saying "subscribed successfully". https://github.com/knadh/listmonk/issues/266 https://github.com/knadh/listmonk/issues/264 --- cmd/public.go | 12 ++++--- cmd/subscribers.go | 87 +++++++++++++++++++++++----------------------- i18n/en.json | 1 + queries.sql | 9 +++-- 4 files changed, 60 insertions(+), 49 deletions(-) diff --git a/cmd/public.go b/cmd/public.go index fdecbb4..0db2d1a 100644 --- a/cmd/public.go +++ b/cmd/public.go @@ -324,14 +324,18 @@ func handleSubscriptionForm(c echo.Context) error { // Insert the subscriber into the DB. req.Status = models.SubscriberStatusEnabled req.ListUUIDs = pq.StringArray(req.SubListUUIDs) - if _, err := insertSubscriber(req.SubReq, app); err != nil && err != errSubscriberExists { + _, _, hasOptin, err := insertSubscriber(req.SubReq, app) + if err != nil { return c.Render(http.StatusInternalServerError, tplMessage, makeMsgTpl(app.i18n.T("public.errorTitle"), "", fmt.Sprintf("%s", err.(*echo.HTTPError).Message))) } - return c.Render(http.StatusOK, tplMessage, - makeMsgTpl(app.i18n.T("public.subTitle"), "", - app.i18n.Ts("public.subConfirmed"))) + msg := "public.subConfirmed" + if hasOptin { + msg = "public.subOptinPending" + } + + return c.Render(http.StatusOK, tplMessage, makeMsgTpl(app.i18n.T("public.subTitle"), "", app.i18n.Ts(msg))) } // handleLinkRedirect redirects a link UUID to its original underlying link diff --git a/cmd/subscribers.go b/cmd/subscribers.go index b36703c..1ded438 100644 --- a/cmd/subscribers.go +++ b/cmd/subscribers.go @@ -79,7 +79,11 @@ func handleGetSubscriber(c echo.Context) error { id, _ = strconv.Atoi(c.Param("id")) ) - sub, err := getSubscriber(id, app) + if id < 1 { + return echo.NewHTTPError(http.StatusBadRequest, app.i18n.T("globals.messages.invalidID")) + } + + sub, err := getSubscriber(id, "", "", app) if err != nil { return err } @@ -273,14 +277,13 @@ func handleCreateSubscriber(c echo.Context) error { } // Insert the subscriber into the DB. - sub, err := insertSubscriber(req, app) + sub, isNew, _, err := insertSubscriber(req, app) if err != nil { - if err == errSubscriberExists { - return echo.NewHTTPError(http.StatusBadRequest, app.i18n.T("subscribers.emailExists")) - } - return err } + if !isNew { + return echo.NewHTTPError(http.StatusBadRequest, app.i18n.T("subscribers.emailExists")) + } return c.JSON(http.StatusOK, okResp{sub}) } @@ -321,11 +324,11 @@ func handleUpdateSubscriber(c echo.Context) error { } // Send a confirmation e-mail (if there are any double opt-in lists). - sub, err := getSubscriber(int(id), app) + sub, err := getSubscriber(int(id), "", "", app) if err != nil { return err } - _ = sendOptinConfirmation(sub, []int64(req.Lists), app) + _, _ = sendOptinConfirmation(sub, []int64(req.Lists), app) return c.JSON(http.StatusOK, okResp{sub}) } @@ -355,7 +358,7 @@ func handleSubscriberSendOptin(c echo.Context) error { app.i18n.Ts("globals.messages.notFound", "name", "{globals.terms.subscriber}")) } - if err := sendOptinConfirmation(out[0], nil, app); err != nil { + if _, err := sendOptinConfirmation(out[0], nil, app); err != nil { return echo.NewHTTPError(http.StatusInternalServerError, app.i18n.T("subscribers.errorSendingOptin")) } @@ -619,56 +622,53 @@ func handleExportSubscriberData(c echo.Context) error { return c.Blob(http.StatusOK, "application/json", b) } -// insertSubscriber inserts a subscriber and returns the ID. -func insertSubscriber(req subimporter.SubReq, app *App) (models.Subscriber, error) { +// insertSubscriber inserts a subscriber and returns the ID. The first bool indicates if +// it was a new subscriber, and the second bool indicates if the subscriber was sent an optin confirmation. +func insertSubscriber(req subimporter.SubReq, app *App) (models.Subscriber, bool, bool, error) { uu, err := uuid.NewV4() if err != nil { - return req.Subscriber, err + return req.Subscriber, false, false, err } req.UUID = uu.String() - err = app.queries.InsertSubscriber.Get(&req.ID, + isNew := true + if err = app.queries.InsertSubscriber.Get(&req.ID, req.UUID, req.Email, strings.TrimSpace(req.Name), req.Status, req.Attribs, req.Lists, - req.ListUUIDs) - if err != nil { + req.ListUUIDs); err != nil { if pqErr, ok := err.(*pq.Error); ok && pqErr.Constraint == "subscribers_email_key" { - return req.Subscriber, errSubscriberExists + isNew = false + } else { + // return req.Subscriber, errSubscriberExists + app.log.Printf("error inserting subscriber: %v", err) + return req.Subscriber, false, false, echo.NewHTTPError(http.StatusInternalServerError, + app.i18n.Ts("globals.messages.errorCreating", + "name", "{globals.terms.subscriber}", "error", pqErrMsg(err))) } - - app.log.Printf("error inserting subscriber: %v", err) - return req.Subscriber, echo.NewHTTPError(http.StatusInternalServerError, - app.i18n.Ts("globals.messages.errorCreating", - "name", "{globals.terms.subscriber}", "error", pqErrMsg(err))) } - // Fetch the subscriber's full data. - sub, err := getSubscriber(req.ID, app) + // Fetch the subscriber's full data. If the subscriber already existed and wasn't + // created, the id will be empty. Fetch the details by e-mail then. + sub, err := getSubscriber(req.ID, "", strings.ToLower(req.Email), app) if err != nil { - return sub, err + return sub, false, false, err } // Send a confirmation e-mail (if there are any double opt-in lists). - _ = sendOptinConfirmation(sub, []int64(req.Lists), app) - return sub, nil + num, _ := sendOptinConfirmation(sub, []int64(req.Lists), app) + return sub, isNew, num > 0, nil } -// getSubscriber gets a single subscriber by ID. -func getSubscriber(id int, app *App) (models.Subscriber, error) { - var ( - out models.Subscribers - ) +// getSubscriber gets a single subscriber by ID, uuid, or e-mail in that order. +// Only one of these params should have a value. +func getSubscriber(id int, uuid, email string, app *App) (models.Subscriber, error) { + var out models.Subscribers - if id < 1 { - return models.Subscriber{}, - echo.NewHTTPError(http.StatusBadRequest, app.i18n.T("globals.messages.invalidID")) - } - - if err := app.queries.GetSubscriber.Select(&out, id, nil); err != nil { + if err := app.queries.GetSubscriber.Select(&out, id, uuid, email); err != nil { app.log.Printf("error fetching subscriber: %v", err) return models.Subscriber{}, echo.NewHTTPError(http.StatusInternalServerError, app.i18n.Ts("globals.messages.errorFetching", @@ -733,8 +733,9 @@ func exportSubscriberData(id int64, subUUID string, exportables map[string]bool, } // sendOptinConfirmation sends a double opt-in confirmation e-mail to a subscriber -// if at least one of the given listIDs is set to optin=double -func sendOptinConfirmation(sub models.Subscriber, listIDs []int64, app *App) error { +// if at least one of the given listIDs is set to optin=double. It returns the number of +// opt-in lists that were found. +func sendOptinConfirmation(sub models.Subscriber, listIDs []int64, app *App) (int, error) { var lists []models.List // Fetch double opt-in lists from the given list IDs. @@ -742,12 +743,12 @@ func sendOptinConfirmation(sub models.Subscriber, listIDs []int64, app *App) err if err := app.queries.GetSubscriberLists.Select(&lists, sub.ID, nil, pq.Int64Array(listIDs), nil, models.SubscriptionStatusUnconfirmed, models.ListOptinDouble); err != nil { app.log.Printf("error fetching lists for opt-in: %s", pqErrMsg(err)) - return err + return 0, err } // None. if len(lists) == 0 { - return nil + return 0, nil } var ( @@ -764,9 +765,9 @@ func sendOptinConfirmation(sub models.Subscriber, listIDs []int64, app *App) err if err := app.sendNotification([]string{sub.Email}, app.i18n.T("subscribers.optinSubject"), notifSubscriberOptin, out); err != nil { app.log.Printf("error sending opt-in e-mail: %s", err) - return err + return 0, err } - return nil + return len(lists), nil } // sanitizeSQLExp does basic sanitisation on arbitrary diff --git a/i18n/en.json b/i18n/en.json index 2cfe0fa..20eb6f2 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -259,6 +259,7 @@ "public.privacyWipeHelp": "Delete all your subscriptions and related data from the database permanently.", "public.sub": "Subscribe", "public.subConfirmed": "Subscribed successfully.", + "public.subOptinPending": "An e-mail has been sent to you to confirm your subscription(s).", "public.subConfirmedTitle": "Confirmed", "public.subName": "Name (optional)", "public.subNotFound": "Subscription not found.", diff --git a/queries.sql b/queries.sql index d63c3e4..6290423 100644 --- a/queries.sql +++ b/queries.sql @@ -1,8 +1,13 @@ -- subscribers -- name: get-subscriber --- Get a single subscriber by id or UUID. -SELECT * FROM subscribers WHERE CASE WHEN $1 > 0 THEN id = $1 ELSE uuid = $2 END; +-- Get a single subscriber by id or UUID or email. +SELECT * FROM subscribers WHERE + CASE + WHEN $1 > 0 THEN id = $1 + WHEN $2 != '' THEN uuid = $2::UUID + WHEN $3 != '' THEN email = $3 + END; -- name: subscriber-exists -- Check if a subscriber exists by id or UUID.