Update newsletter saving to reflect code review comments

- Switch to using full segment objects when saving newsletters
- Fix stale comment in newsletter editor's Newsletter model
- Fix typo in newsletter editor tests

assets/js/src/form/fields/selection.jsx

@@ -122,9 +122,10 @@ function(
         } else {
           value = e.target.value;
         }
+        var transformedValue = this.transformChangedValue(value);
         this.props.onValueChange({
          target: {
-            value: value,
+            value: transformedValue,
             name: this.props.field.name
           }
         });
@@ -148,6 +149,16 @@ function(
       }
       return item.id;
     },
+    // When it's impossible to represent the desired value in DOM,
+    // this function may be used to transform the placeholder value into
+    // desired value.
+    transformChangedValue: function(value) {
+      if(typeof this.props.field['transformChangedValue'] === 'function') {
+        return this.props.field.transformChangedValue.call(this, value);
+      } else {
+        return value;
+      }
+    },
     render: function() {
       const options = this.state.items.map((item, index) => {
         let label = this.getLabel(item);

assets/js/src/newsletter_editor/components/content.js

@@ -18,7 +18,8 @@ define([
       });
     },
     toJSON: function() {
-      // Remove stale attributes from resulting JSON object
+      // Use only whitelisted properties to ensure properties editor
+      // doesn't control don't change.
       return _.pick(SuperModel.prototype.toJSON.call(this), this.whitelisted);
     },
   });

assets/js/src/newsletters/send/notification.jsx

@@ -1,11 +1,13 @@
 define(
   [
     'mailpoet',
-    'newsletters/types/notification/scheduling.jsx'
+    'newsletters/types/notification/scheduling.jsx',
+    'underscore'
   ],
   function(
     MailPoet,
-    Scheduling
+    Scheduling,
+    _
   ) {
 
     var settings = window.mailpoet_settings || {};
@@ -42,6 +44,14 @@ define(
         getLabel: function(segment) {
           return segment.name + ' (' + parseInt(segment.subscribers).toLocaleString() + ')';
         },
+        transformChangedValue: function(segment_ids) {
+          var all_segments = this.state.items;
+          return _.map(segment_ids, function(id) {
+            return _.find(all_segments, function(segment) {
+              return segment.id === id;
+            });
+          });
+        },
         validation: {
           'data-parsley-required': true,
           'data-parsley-required-message': MailPoet.I18n.t('noSegmentsSelectedError')

assets/js/src/newsletters/send/standard.jsx

@@ -348,6 +348,14 @@ define(
         getLabel: function(segment) {
           return segment.name + ' (' + parseInt(segment.subscribers).toLocaleString() + ')';
         },
+        transformChangedValue: function(segment_ids) {
+          var all_segments = this.state.items;
+          return _.map(segment_ids, function(id) {
+            return _.find(all_segments, function(segment) {
+              return segment.id === id;
+            });
+          });
+        },
         validation: {
           'data-parsley-required': true,
           'data-parsley-required-message': MailPoet.I18n.t('noSegmentsSelectedError')

lib/API/Endpoints/Newsletters.php

@@ -40,9 +40,9 @@ class Newsletters extends APIEndpoint {
   }
 
   function save($data = array()) {
-    $segment_ids = array();
+    $segments = array();
     if(isset($data['segments'])) {
-      $segment_ids = $data['segments'];
+      $segments = $data['segments'];
       unset($data['segments']);
     }
 
@@ -58,14 +58,14 @@ class Newsletters extends APIEndpoint {
     if(!empty($errors)) {
       return $this->badRequest($errors);
     } else {
-      if(!empty($segment_ids)) {
+      if(!empty($segments)) {
         NewsletterSegment::where('newsletter_id', $newsletter->id)
           ->deleteMany();
 
-        foreach($segment_ids as $segment_id) {
+        foreach($segments as $segment) {
-          $id = (is_array($segment_id)) ? (int)$segment_id['id'] : (int)$segment_id;
+          if(!is_array($segment)) continue;
           $relation = NewsletterSegment::create();
-          $relation->segment_id = $id;
+          $relation->segment_id = (int)$segment['id'];
           $relation->newsletter_id = $newsletter->id;
           $relation->save();
         }

tests/javascript/newsletter_editor/components/content.spec.js

@@ -38,7 +38,7 @@ define([
       });
 
       describe('toJSON()', function() {
-        it('will only contain properties modifyable by the editor', function() {
+        it('will only contain properties modifiable by the editor', function() {
           var model = new (ContentComponent.NewsletterModel)({
             id: 19,
             subject: 'some subject',

tests/unit/API/NewslettersTest.php

@@ -82,6 +82,27 @@ class NewslettersTest extends MailPoetTest {
     expect($updated_newsletter->subject)->equals('My Updated Newsletter');
   }
 
+  function testItCanModifySegmentsOfExistingNewsletter() {
+    $segment_1 = Segment::createOrUpdate(array('name' => 'Segment 1'));
+    $fake_segment_id = 1;
+
+    $router = new Newsletters();
+    $newsletter_data = array(
+      'id' => $this->newsletter->id,
+      'subject' => 'My Updated Newsletter',
+      'segments' => array($segment_1->asArray(), $fake_segment_id)
+    );
+
+    $response = $router->save($newsletter_data);
+    expect($response->status)->equals(APIResponse::STATUS_OK);
+
+    $updated_newsletter =
+      Newsletter::findOne($this->newsletter->id)->withSegments();
+
+    expect(count($updated_newsletter->segments))->equals(1);
+    expect($updated_newsletter->segments[0]['name'])->equals('Segment 1');
+  }
+
   function testItCanSetANewsletterStatus() {
     $router = new Newsletters();
     // set status to sending