Refactor subsbscription status option on the import page.

- Refactor subimporter New*() funcs to take opt structs. - Refactor and simplify Vue code. - Remove redundant i18n entries and use existing ones. - Remove redundant subimporter constants and use existing ones. - Consider 'overwrite' option for subscription status as well. - Write Cypress integration tests for the new feature.
knadh · Jun 6, 2021 · 868fae6 · 868fae6
1 parent 7ca08f0
commit 868fae6
Show file tree

Hide file tree

Showing 16 changed files with 579 additions and 534 deletions.
diff --git a/cmd/import.go b/cmd/import.go
@@ -8,18 +8,10 @@ import (
 	"strings"
 
 	"github.com/knadh/listmonk/internal/subimporter"
+	"github.com/knadh/listmonk/models"
 	"github.com/labstack/echo"
 )
 
-// reqImport represents file upload import params.
-type reqImport struct {
-	Mode               string `json:"mode"`
-	SubscriptionStatus string `json:"subscriptionStatus"`
-	Overwrite          bool   `json:"overwrite"`
-	Delim              string `json:"delim"`
-	ListIDs            []int  `json:"lists"`
-}
-
 // handleImportSubscribers handles the uploading and bulk importing of
 // a ZIP file of one or more CSV files.
 func handleImportSubscribers(c echo.Context) error {
@@ -31,21 +23,34 @@ func handleImportSubscribers(c echo.Context) error {
 	}
 
 	// Unmarsal the JSON params.
-	var r reqImport
-	if err := json.Unmarshal([]byte(c.FormValue("params")), &r); err != nil {
+	var opt subimporter.SessionOpt
+	if err := json.Unmarshal([]byte(c.FormValue("params")), &opt); err != nil {
 		return echo.NewHTTPError(http.StatusBadRequest,
 			app.i18n.Ts("import.invalidParams", "error", err.Error()))
 	}
 
-	if r.Mode != subimporter.ModeSubscribe && r.Mode != subimporter.ModeBlocklist {
+	// Validate mode.
+	if opt.Mode != subimporter.ModeSubscribe && opt.Mode != subimporter.ModeBlocklist {
 		return echo.NewHTTPError(http.StatusBadRequest, app.i18n.T("import.invalidMode"))
 	}
 
-	if r.SubscriptionStatus != subimporter.SubscriptionStatusUnconfirmed && r.SubscriptionStatus != subimporter.SubscriptionStatusConfirmed {
-		return echo.NewHTTPError(http.StatusBadRequest, app.i18n.T("import.invalidSubscriptionStatus"))
+	// If no status is specified, pick a default one.
+	if opt.SubStatus == "" {
+		switch opt.Mode {
+		case subimporter.ModeSubscribe:
+			opt.SubStatus = models.SubscriptionStatusUnconfirmed
+		case subimporter.ModeBlocklist:
+			opt.SubStatus = models.SubscriptionStatusUnsubscribed
+		}
+	}
+
+	if opt.SubStatus != models.SubscriptionStatusUnconfirmed &&
+		opt.SubStatus != models.SubscriptionStatusConfirmed &&
+		opt.SubStatus != models.SubscriptionStatusUnsubscribed {
+		return echo.NewHTTPError(http.StatusBadRequest, app.i18n.T("import.invalidSubStatus"))
 	}
 
-	if len(r.Delim) != 1 {
+	if len(opt.Delim) != 1 {
 		return echo.NewHTTPError(http.StatusBadRequest, app.i18n.T("import.invalidDelim"))
 	}
 
@@ -74,15 +79,16 @@ func handleImportSubscribers(c echo.Context) error {
 	}
 
 	// Start the importer session.
-	impSess, err := app.importer.NewSession(file.Filename, r.Mode, r.SubscriptionStatus, r.Overwrite, r.ListIDs)
+	opt.Filename = file.Filename
+	impSess, err := app.importer.NewSession(opt)
 	if err != nil {
 		return echo.NewHTTPError(http.StatusInternalServerError,
 			app.i18n.Ts("import.errorStarting", "error", err.Error()))
 	}
 	go impSess.Start()
 
 	if strings.HasSuffix(strings.ToLower(file.Filename), ".csv") {
-		go impSess.LoadCSV(out.Name(), rune(r.Delim[0]))
+		go impSess.LoadCSV(out.Name(), rune(opt.Delim[0]))
 	} else {
 		// Only 1 CSV from the ZIP is considered. If multiple files have
 		// to be processed, counting the net number of lines (to track progress),
@@ -95,7 +101,7 @@ func handleImportSubscribers(c echo.Context) error {
 			return echo.NewHTTPError(http.StatusInternalServerError,
 				app.i18n.Ts("import.errorProcessingZIP", "error", err.Error()))
 		}
-		go impSess.LoadCSV(dir+"/"+files[0], rune(r.Delim[0]))
+		go impSess.LoadCSV(dir+"/"+files[0], rune(opt.Delim[0]))
 	}
 
 	return c.JSON(http.StatusOK, okResp{app.importer.GetStats()})

diff --git a/frontend/cypress/integration/import.js b/frontend/cypress/integration/import.js
@@ -7,12 +7,19 @@ describe('Import', () => {
 
   it('Imports subscribers', () => {
     const cases = [
-      { mode: 'check-subscribe', status: 'enabled', count: 102 },
-      { mode: 'check-blocklist', status: 'blocklisted', count: 102 },
+      { chkMode: 'subscribe', status: 'enabled', chkSubStatus: 'unconfirmed', subStatus: 'unconfirmed', overwrite: true, count: 102 },
+      { chkMode: 'subscribe', status: 'enabled', chkSubStatus: 'confirmed', subStatus: 'confirmed', overwrite: true, count: 102 },
+      { chkMode: 'subscribe', status: 'enabled', chkSubStatus: 'unconfirmed', subStatus: 'confirmed', overwrite: false, count: 102 },
+      { chkMode: 'blocklist', status: 'blocklisted', chkSubStatus: 'unsubscribed', subStatus: 'unsubscribed', overwrite: true, count: 102 },
     ];
 
     cases.forEach((c) => {
-      cy.get(`[data-cy=${c.mode}] .check`).click();
+      cy.get(`[data-cy=check-${c.chkMode}] .check`).click();
+      cy.get(`[data-cy=check-${c.chkSubStatus}] .check`).click();
+
+      if (!c.overwrite) {
+        cy.get(`[data-cy=overwrite]`).click();
+      }
 
       if (c.status === 'enabled') {
         cy.get('.list-selector input').click();
@@ -39,12 +46,39 @@ describe('Import', () => {
         cy.expect(parseInt($el.text().trim())).to.equal(c.count);
       });
 
+      // Subscriber status.
       cy.get('tbody td[data-label=Status]').each(($el) => {
         cy.wrap($el).find(`.tag.${c.status}`);
       });
 
+      // Subscription status.
+      cy.get('tbody td[data-label=E-mail]').each(($el) => {
+        cy.wrap($el).find(`.tag.${c.subStatus}`);
+      });
+
       cy.loginAndVisit('/subscribers/import');
       cy.wait(100);
     });
   });
+
+  it('Imports subscribers incorrectly', () => {
+    cy.resetDB();
+    cy.loginAndVisit('/subscribers/import');
+
+    cy.get('.list-selector input').click();
+    cy.get('.list-selector .autocomplete a').first().click();
+    cy.get('input[name=delim]').clear().type('|');
+
+    cy.fixture('subs.csv').then((data) => {
+      cy.get('input[type="file"]').attachFile({
+        fileContent: data.toString(),
+        fileName: 'subs.csv',
+        mimeType: 'text/csv',
+      });
+    });
+
+    cy.get('button.is-primary').click();
+    cy.wait(250);
+    cy.get('section.wrap .has-text-danger');
+  });
 });
diff --git a/frontend/src/views/Import.vue b/frontend/src/views/Import.vue
@@ -8,7 +8,7 @@
         <div>
           <div class="columns">
             <div class="column">
-              <b-field :label="$t('import.mode')">
+              <b-field :label="$t('import.mode')" :addons="false">
                 <div>
                   <b-radio v-model="form.mode" name="mode"
                     native-value="subscribe"
@@ -21,52 +21,47 @@
               </b-field>
             </div>
             <div class="column">
-              <b-field :label="$t('import.subscriptionStatus')">
-                <div>
-                  <div v-if="form.mode === 'subscribe'" style="display:block">
-                    <b-radio
-                      v-model="form.subscriptionStatus"
-                      name="subscriptionStatus"
-                      native-value="unconfirmed"
-                      data-cy="check-unconfirmed">
-                      {{ $t('import.unconfirmed') }}
-                    </b-radio>
-                  </div>
-                  <div v-if="form.mode === 'subscribe'" style="display:block">
-                    <b-radio
-                      v-model="form.subscriptionStatus"
-                      name="subscriptionStatus"
-                      native-value="confirmed"
-                      data-cy="check-confirmed">
-                      {{ $t('import.confirmed') }}
-                    </b-radio>
-                  </div>
-                  <div v-if="form.mode === 'blocklist'" style="display:block">
-                    <b-radio
-                      v-model="form.subscriptionStatus"
-                      name="subscriptionStatus"
-                      native-value="unsubscribed"
-                      data-cy="check-unsubscribed">
-                      {{ $t('import.unsubscribed') }}
-                    </b-radio>
-                  </div>
-                </div>
+              <b-field :label="$t('globals.fields.status')" :addons="false">
+                <template v-if="form.mode === 'subscribe'">
+                  <b-radio
+                    v-model="form.subStatus"
+                    name="subStatus"
+                    native-value="unconfirmed"
+                    data-cy="check-unconfirmed">
+                    {{ $t('subscribers.status.unconfirmed') }}
+                  </b-radio>
+                  <b-radio
+                    v-model="form.subStatus"
+                    name="subStatus"
+                    native-value="confirmed"
+                    data-cy="check-confirmed">
+                    {{ $t('subscribers.status.confirmed') }}
+                  </b-radio>
+                </template>
+
+                <b-radio v-else
+                  v-model="form.subStatus"
+                  name="subStatus"
+                  native-value="unsubscribed"
+                  data-cy="check-unsubscribed">
+                  {{ $t('subscribers.status.unsubscribed') }}
+                </b-radio>
               </b-field>
             </div>
+
             <div class="column">
               <b-field v-if="form.mode === 'subscribe'"
                 :label="$t('import.overwrite')"
                 :message="$t('import.overwriteHelp')">
                 <div>
-                  <b-switch v-model="form.overwrite" name="overwrite" />
+                  <b-switch v-model="form.overwrite" name="overwrite" data-cy="overwrite" />
                 </div>
               </b-field>
             </div>
+
             <div class="column">
-              <b-field :label="$t('import.csvDelim')" :message="$t('import.csvDelimHelp')"
-                class="delimiter">
-                <b-input v-model="form.delim" name="delim"
-                  placeholder="," maxlength="1" required />
+              <b-field :label="$t('import.csvDelim')" :message="$t('import.csvDelimHelp')" class="delimiter">
+                <b-input v-model="form.delim" name="delim" placeholder="," maxlength="1" required />
               </b-field>
             </div>
           </div>
@@ -187,7 +182,7 @@ export default Vue.extend({
     return {
       form: {
         mode: 'subscribe',
-        subscriptionStatus: 'unconfirmed',
+        subStatus: 'unconfirmed',
         delim: ',',
         lists: [],
         overwrite: true,
@@ -206,15 +201,15 @@ export default Vue.extend({
   },
 
   watch: {
-    form: {
-      handler(val) {
-        if (val.mode === 'subscribe') {
-          this.form.subscriptionStatus = 'unconfirmed';
-        } else if (val.mode === 'blocklist') {
-          this.form.subscriptionStatus = 'unsubscribed';
+    'form.mode': function formMode() {
+      // Select the appropriate status radio whenever mode changes.
+      this.$nextTick(() => {
+        if (this.form.mode === 'subscribe') {
+          this.form.subStatus = 'unconfirmed';
+        } else {
+          this.form.subStatus = 'unsubscribed';
         }
-      },
-      deep: true,
+      });
     },
   },
 
@@ -317,7 +312,7 @@ export default Vue.extend({
       const params = new FormData();
       params.set('params', JSON.stringify({
         mode: this.form.mode,
-        subscriptionStatus: this.form.subscriptionStatus,
+        subscription_status: this.form.subStatus,
         delim: this.form.delim,
         lists: this.form.lists.map((l) => l.id),
         overwrite: this.form.overwrite,

diff --git a/i18n/de.json b/i18n/de.json
@@ -186,6 +186,7 @@
     "import.invalidFile": "Ungültige Datei: {error}",
     "import.invalidMode": "Ungültiger Modus",
     "import.invalidParams": "Ungültiger Parameter: {error}",
+    "import.invalidSubStatus": "Invalid subscription status",
     "import.listSubHelp": "Listen die abonniert werden.",
     "import.mode": "Modus",
     "import.overwrite": "Überschreiben?",

diff --git a/i18n/en.json b/i18n/en.json
@@ -185,16 +185,12 @@
     "import.invalidDelim": "Delimiter should be a single character.",
     "import.invalidFile": "Invalid file: {error}",
     "import.invalidMode": "Invalid mode",
-    "import.invalidSubscriptionStatus": "Invalid subscription status",
     "import.invalidParams": "Invalid params: {error}",
+    "import.invalidSubStatus": "Invalid subscription status",
     "import.listSubHelp": "Lists to subscribe to.",
     "import.mode": "Mode",
-    "import.subscriptionStatus": "Subscription Status",
-    "import.confirmed": "Confirmed",
-    "import.unconfirmed": "Unconfirmed",
-    "import.unsubscribed": "Unsubscribed",
     "import.overwrite": "Overwrite?",
-    "import.overwriteHelp": "Overwrite name and attribs of existing subscribers?",
+    "import.overwriteHelp": "Overwrite name, attribs, subscription status of existing subscribers?",
     "import.recordsCount": "{num} / {total} records",
     "import.stopImport": "Stop import",
     "import.subscribe": "Subscribe",

diff --git a/i18n/es.json b/i18n/es.json
@@ -186,6 +186,7 @@
     "import.invalidFile": "Archivo inválido: {error}",
     "import.invalidMode": "Modo inválido",
     "import.invalidParams": "Paramétros inválidos: {error}",
+    "import.invalidSubStatus": "Invalid subscription status",
     "import.listSubHelp": "Listas a subscribir",
     "import.mode": "Modo",
     "import.overwrite": "¿Sobre-escribir?",

diff --git a/i18n/fr.json b/i18n/fr.json
@@ -186,6 +186,7 @@
     "import.invalidFile": "Fichier non valide : {error}",
     "import.invalidMode": "Mode invalide",
     "import.invalidParams": "Paramètres non valides : {error}",
+    "import.invalidSubStatus": "Invalid subscription status",
     "import.listSubHelp": "Abonner aux listes",
     "import.mode": "Mode",
     "import.overwrite": "Écraser ?",

diff --git a/i18n/it.json b/i18n/it.json
@@ -186,6 +186,7 @@
     "import.invalidFile": "File non valido: {error}",
     "import.invalidMode": "Modalità non valida",
     "import.invalidParams": "Parametri non validi: {error}",
+    "import.invalidSubStatus": "Invalid subscription status",
     "import.listSubHelp": "Liste a cui iscriversi.",
     "import.mode": "Modalità",
     "import.overwrite": "Sovrascrivere?",

diff --git a/i18n/ml.json b/i18n/ml.json
@@ -186,6 +186,7 @@
     "import.invalidFile": " ഫയൽ അസാധുവാണ് : {error}",
     "import.invalidMode": "ശൈലി അസാധുവാണ്",
     "import.invalidParams": "പരാമുകൾ അസാധുവാണ്: {error}",
+    "import.invalidSubStatus": "Invalid subscription status",
     "import.listSubHelp": "വരിക്കാരനാകാനുള്ള ലിസ്റ്റുകൾ.",
     "import.mode": "ശൈലി",
     "import.overwrite": "തിരുത്തിയെഴുതട്ടേ?",

diff --git a/i18n/pl.json b/i18n/pl.json
@@ -186,6 +186,7 @@
     "import.invalidFile": "Nieprawidłowy plik: {error}",
     "import.invalidMode": "Nieprawidłowy tryp",
     "import.invalidParams": "Nieprawidłowe parametry: {error}",
+    "import.invalidSubStatus": "Invalid subscription status",
     "import.listSubHelp": "Listy do subskrybowania.",
     "import.mode": "Tryb",
     "import.overwrite": "Nadpisać?",

diff --git a/i18n/pt-BR.json b/i18n/pt-BR.json
@@ -186,6 +186,7 @@
     "import.invalidFile": "Arquivo inválido: {error}",
     "import.invalidMode": "Modo inválido",
     "import.invalidParams": "Parâmetros inválidos: {error}",
+    "import.invalidSubStatus": "Invalid subscription status",
     "import.listSubHelp": "Listas para inscrever.",
     "import.mode": "Modo",
     "import.overwrite": "Sobrescrever?",

diff --git a/i18n/pt.json b/i18n/pt.json
@@ -186,6 +186,7 @@
     "import.invalidFile": "Ficheiro inválido: {error}",
     "import.invalidMode": "Modo inválido",
     "import.invalidParams": "Parâmetros inválidos: {error}",
+    "import.invalidSubStatus": "Invalid subscription status",
     "import.listSubHelp": "Listas a subscrever.",
     "import.mode": "Modo",
     "import.overwrite": "Sobrescrever?",

diff --git a/i18n/ru.json b/i18n/ru.json
@@ -186,6 +186,7 @@
     "import.invalidFile": "Неверный файл: {error}",
     "import.invalidMode": "Неверный режим",
     "import.invalidParams": "Неверные параметры: {error}",
+    "import.invalidSubStatus": "Invalid subscription status",
     "import.listSubHelp": "Списки для подписки.",
     "import.mode": "Режим",
     "import.overwrite": "Перезаписать?",