From 42d34a56c3a3bfd1f25e08418cf71e39d7f6e906 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Oliver=20G=C3=BCnther?= <mail@oliverguenther.de>
Date: Wed, 9 Oct 2019 08:35:25 +0200
Subject: [PATCH] Fix status parsing when nil

Unfortunately, representable is broken when passing null values into the
parsing representer. This will result in `property :foo` to be output as
`{ foo: nil }` despite having a reader that writes other stuff.
---
 .../projects/set_attributes_service.rb        |  5 +-
 .../project-status-edit-field.component.html  | 42 +++++++++--------
 .../project-status-edit-field.component.ts    |  4 ++
 .../app/modules/grids/grid/grid.component.ts  |  2 +-
 lib/api/v3/projects/project_representer.rb    | 26 +++++++----
 ...roject_payload_representer_parsing_spec.rb | 39 +++++++++-------
 spec/requests/api/v3/project_resource_spec.rb | 31 +++++++++++++
 .../projects/set_attributes_service_spec.rb   | 46 +++++++++++--------
 8 files changed, 130 insertions(+), 65 deletions(-)

diff --git a/app/services/projects/set_attributes_service.rb b/app/services/projects/set_attributes_service.rb
index f462098a56..8d64bda46d 100644
--- a/app/services/projects/set_attributes_service.rb
+++ b/app/services/projects/set_attributes_service.rb
@@ -33,7 +33,10 @@ module Projects
     private
 
     def set_attributes(attributes)
-      status_attributes = attributes.delete(:status) || {}
+      # Delete the status attribute which gets set by representable
+      # IFF the status_code is given as null
+      attributes.delete(:status)
+      status_attributes = attributes.delete(:status_attributes) || {}
 
       ret = super(attributes)
 
diff --git a/frontend/src/app/modules/fields/edit/field-types/project-status-edit-field.component.html b/frontend/src/app/modules/fields/edit/field-types/project-status-edit-field.component.html
index 5ff9f53407..ba20b5b76f 100644
--- a/frontend/src/app/modules/fields/edit/field-types/project-status-edit-field.component.html
+++ b/frontend/src/app/modules/fields/edit/field-types/project-status-edit-field.component.html
@@ -1,20 +1,22 @@
-<ng-select [items]="availableStatuses"
-           [(ngModel)]="resource['status']"
-           bindLabel="name"
-           bindValue="value"
-           class="project-status"
-           (open)="onOpen()"
-           (close)="onClose()"
-           (change)="handler.handleUserSubmit()"
-           [appendTo]="hiddenOverflowContainer"
-           [hideSelected]="true"
-           [disabled]="inFlight">
-  <ng-template ng-label-tmp let-item="item">
-    <span class="project-status--bulb" [ngClass]="item.colorClass"></span>
-    <span class="project-status--name" [ngClass]="item.colorClass">{{item.name}}</span>
-  </ng-template>
-  <ng-template ng-option-tmp let-item="item" let-index="index" let-search="searchTerm">
-    <span class="project-status--bulb" [ngClass]="item.colorClass"></span>
-    <span class="project-status--name" [ngClass]="item.colorClass">{{item.name}}</span>
-  </ng-template>
-</ng-select>
\ No newline at end of file
+<div (keydown.escape)="handler.handleUserCancel()">
+  <ng-select [items]="availableStatuses"
+             [(ngModel)]="resource.status"
+             bindLabel="name"
+             bindValue="value"
+             class="project-status"
+             (open)="onOpen()"
+             (close)="onClose()"
+             (change)="handler.handleUserSubmit()"
+             [appendTo]="hiddenOverflowContainer"
+             [hideSelected]="true"
+             [disabled]="inFlight">
+    <ng-template ng-label-tmp let-item="item">
+      <span class="project-status--bulb" [ngClass]="item.colorClass"></span>
+      <span class="project-status--name" [ngClass]="item.colorClass">{{item.name}}</span>
+    </ng-template>
+    <ng-template ng-option-tmp let-item="item" let-index="index" let-search="searchTerm">
+      <span class="project-status--bulb" [ngClass]="item.colorClass"></span>
+      <span class="project-status--name" [ngClass]="item.colorClass">{{item.name}}</span>
+    </ng-template>
+  </ng-select>
+</div>
\ No newline at end of file
diff --git a/frontend/src/app/modules/fields/edit/field-types/project-status-edit-field.component.ts b/frontend/src/app/modules/fields/edit/field-types/project-status-edit-field.component.ts
index 33bd2ee05e..2b0ff95025 100644
--- a/frontend/src/app/modules/fields/edit/field-types/project-status-edit-field.component.ts
+++ b/frontend/src/app/modules/fields/edit/field-types/project-status-edit-field.component.ts
@@ -72,4 +72,8 @@ export class ProjectStatusEditFieldComponent extends EditFieldComponent implemen
   public onClose() {
     // Nothing to do
   }
+
+  public handleKeyDown($event:KeyboardEvent) {
+    console.debug($event);
+  }
 }
diff --git a/frontend/src/app/modules/grids/grid/grid.component.ts b/frontend/src/app/modules/grids/grid/grid.component.ts
index 2b850b1152..7b005a702d 100644
--- a/frontend/src/app/modules/grids/grid/grid.component.ts
+++ b/frontend/src/app/modules/grids/grid/grid.component.ts
@@ -73,7 +73,7 @@ export class GridComponent implements OnDestroy, OnInit {
     let registration = this.widgetsService.registered.find((reg) => reg.identifier === widget.identifier);
 
     if (!registration) {
-      debugLog(`No widget registered with identifier ${widget.identifier}`);
+      // debugLog(`No widget registered with identifier ${widget.identifier}`);
 
       return null;
     } else {
diff --git a/lib/api/v3/projects/project_representer.rb b/lib/api/v3/projects/project_representer.rb
index 8a046f0122..06a5396d54 100644
--- a/lib/api/v3/projects/project_representer.rb
+++ b/lib/api/v3/projects/project_representer.rb
@@ -46,6 +46,13 @@ module API
 
         self_link
 
+        def to_h
+          # Representable is broken when passing nil as parameters
+          # it will set the property :status and :statusExplanation
+          # regardless of what the setter actually does
+          super.except(:status, :statusExplanation)
+        end
+
         link :createWorkPackage,
              cache_if: -> { current_user_allowed_to(:add_work_packages, context: represented) } do
           {
@@ -163,19 +170,22 @@ module API
         date_time_property :updated_at
 
         property :status,
+                 name_source: ->(*) { I18n.t('activerecord.attributes.project/status.code') },
                  render_nil: true,
                  getter: ->(*) {
                    next unless status&.code
 
                    status.code.to_s.tr('_', ' ')
                  },
-                 setter: ->(fragment:, **) {
-                   self.status ||= {}
-                   status[:code] =
-                     if fragment.nil?
+                 reader: ->(doc:, represented:, **) {
+                   next unless doc.key?('status')
+
+                   represented.status_attributes ||= {}
+                   represented.status_attributes[:code] =
+                     if doc['status'].nil?
                        nil
                      else
-                       fragment.strip.tr(' ', '_').underscore.to_sym
+                       doc['status'].strip.tr(' ', '_').underscore.to_sym
                      end
                  }
 
@@ -186,9 +196,9 @@ module API
                                                       object: self,
                                                       plain: false)
                  },
-                 setter: ->(fragment:, **) {
-                   self.status ||= {}
-                   status[:explanation] = fragment["raw"]
+                 setter: ->(fragment:, represented:, **) {
+                   represented.status_attributes ||= {}
+                   represented.status_attributes[:explanation] = fragment["raw"]
                  }
 
         def _type
diff --git a/spec/lib/api/v3/projects/project_payload_representer_parsing_spec.rb b/spec/lib/api/v3/projects/project_payload_representer_parsing_spec.rb
index 7c9951a7b3..eedd04ea27 100644
--- a/spec/lib/api/v3/projects/project_payload_representer_parsing_spec.rb
+++ b/spec/lib/api/v3/projects/project_payload_representer_parsing_spec.rb
@@ -49,11 +49,11 @@ describe ::API::V3::Projects::ProjectPayloadRepresenter, 'parsing' do
       end
 
       it 'updates code' do
-        project = representer.from_hash(hash)
-        expect(project.status[:code])
+        project_obj = representer.from_hash(hash)
+        expect(project_obj.status_attributes[:code])
           .to eql(:on_track)
 
-        expect(project.status[:explanation])
+        expect(project_obj.status_attributes[:explanation])
           .to eql('status code explanation')
       end
 
@@ -65,14 +65,14 @@ describe ::API::V3::Projects::ProjectPayloadRepresenter, 'parsing' do
         end
 
         it 'does not set code' do
-          project = representer.from_hash(hash)
-          expect(project.status[:code])
+          project_obj = representer.from_hash(hash)
+          expect(project_obj.status_attributes[:code])
             .to be_nil
         end
 
         it 'updates explanation' do
-          project = representer.from_hash(hash)
-          expect(project.status[:explanation])
+          project_obj = representer.from_hash(hash)
+          expect(project_obj.status_attributes[:explanation])
             .to eql('status code explanation')
         end
       end
@@ -85,14 +85,14 @@ describe ::API::V3::Projects::ProjectPayloadRepresenter, 'parsing' do
         end
 
         it 'does set code' do
-          project = representer.from_hash(hash)
-          expect(project.status[:code])
+          project_obj = representer.from_hash(hash)
+          expect(project_obj.status_attributes[:code])
             .to eql :off_track
         end
 
         it 'does not set explanation' do
-          project = representer.from_hash(hash)
-          expect(project.status[:explanation])
+          project_obj = representer.from_hash(hash)
+          expect(project_obj.status_attributes[:explanation])
             .to be_nil
         end
       end
@@ -105,13 +105,20 @@ describe ::API::V3::Projects::ProjectPayloadRepresenter, 'parsing' do
         end
 
         it 'does set status to nil' do
-          project = representer.from_hash(hash).to_h
+          project_obj = representer.from_hash(hash).to_h
 
-          expect(project)
-            .to be_key(:status)
+          expect(project_obj)
+            .to have_key(:status_attributes)
 
-          expect(project[:status])
-            .to be_nil
+          status_attributes = project_obj[:status_attributes]
+          expect(status_attributes)
+            .to have_key(:code)
+
+          expect(status_attributes)
+            .not_to have_key(:explanation)
+
+          expect(status_attributes[:code])
+            .to eq nil
         end
       end
     end
diff --git a/spec/requests/api/v3/project_resource_spec.rb b/spec/requests/api/v3/project_resource_spec.rb
index 2ed09096a2..a798b585f2 100644
--- a/spec/requests/api/v3/project_resource_spec.rb
+++ b/spec/requests/api/v3/project_resource_spec.rb
@@ -496,6 +496,37 @@ describe 'API v3 Project resource', type: :request, content_type: :json do
       end
     end
 
+    context 'with a nil status' do
+      let(:body) do
+        {
+          status: nil,
+          statusExplanation: {
+            raw: "Some explanation."
+          }
+        }
+      end
+
+      it 'alters the status' do
+        expect(last_response.body)
+          .to be_json_eql(nil.to_json)
+          .at_path('status')
+
+        status = project.status.reload
+        expect(status.code).to be_nil
+        expect(status.explanation).to eq 'Some explanation.'
+
+        expect(last_response.body)
+          .to be_json_eql(
+                {
+                  "format": "markdown",
+                  "html": "<p>Some explanation.</p>",
+                  "raw": "Some explanation."
+                }.to_json
+              )
+          .at_path("statusExplanation")
+      end
+    end
+
     context 'with a status' do
       let(:body) do
         {
diff --git a/spec/services/projects/set_attributes_service_spec.rb b/spec/services/projects/set_attributes_service_spec.rb
index d25b9f9703..64081f44b7 100644
--- a/spec/services/projects/set_attributes_service_spec.rb
+++ b/spec/services/projects/set_attributes_service_spec.rb
@@ -37,8 +37,8 @@ describe Projects::SetAttributesService, type: :model do
 
     allow(contract)
       .to receive(:new)
-      .with(project, user)
-      .and_return(contract_instance)
+            .with(project, user)
+            .and_return(contract_instance)
 
     contract
   end
@@ -69,11 +69,11 @@ describe Projects::SetAttributesService, type: :model do
     before do
       allow(project)
         .to receive(:valid?)
-        .and_return(project_valid)
+              .and_return(project_valid)
 
       expect(contract_instance)
         .to receive(:validate)
-        .and_return(contract_valid)
+              .and_return(contract_valid)
     end
 
     subject { instance.call(call_attributes) }
@@ -102,7 +102,7 @@ describe Projects::SetAttributesService, type: :model do
       end
 
       context 'identifier default value' do
-        context 'with a default identifier configured', with_settings: { sequential_project_identifiers: true } do
+        context 'with a default identifier configured', with_settings: {sequential_project_identifiers: true} do
           context 'with an identifier provided' do
             let(:call_attributes) do
               {
@@ -120,7 +120,7 @@ describe Projects::SetAttributesService, type: :model do
             it 'sets a default identifier' do
               allow(Project)
                 .to receive(:next_identifier)
-                .and_return('ipsum')
+                      .and_return('ipsum')
 
               expect(subject.result.identifier)
                 .to eql 'ipsum'
@@ -128,7 +128,7 @@ describe Projects::SetAttributesService, type: :model do
           end
         end
 
-        context 'without a default identifier configured', with_settings: { sequential_project_identifiers: false } do
+        context 'without a default identifier configured', with_settings: {sequential_project_identifiers: false} do
           context 'with an identifier provided' do
             let(:call_attributes) do
               {
@@ -146,7 +146,7 @@ describe Projects::SetAttributesService, type: :model do
             it 'stays nil' do
               allow(Project)
                 .to receive(:next_identifier)
-                .and_return('ipsum')
+                      .and_return('ipsum')
 
               expect(subject.result.identifier)
                 .to be_nil
@@ -155,7 +155,7 @@ describe Projects::SetAttributesService, type: :model do
         end
       end
 
-      context 'public default value', with_settings: { default_projects_public: true } do
+      context 'public default value', with_settings: {default_projects_public: true} do
         context 'with a value for is_public provided' do
           let(:call_attributes) do
             {
@@ -177,7 +177,7 @@ describe Projects::SetAttributesService, type: :model do
         end
       end
 
-      context 'enabled_module_names default value', with_settings: { default_projects_modules: ['lorem', 'ipsum'] } do
+      context 'enabled_module_names default value', with_settings: {default_projects_modules: ['lorem', 'ipsum']} do
         context 'with a value for enabled_module_names provided' do
           let(:call_attributes) do
             {
@@ -220,7 +220,7 @@ describe Projects::SetAttributesService, type: :model do
         before do
           allow(Type)
             .to receive(:default)
-            .and_return default_types
+                  .and_return default_types
         end
 
         context 'with a value for types provided' do
@@ -259,8 +259,10 @@ describe Projects::SetAttributesService, type: :model do
         context 'with a value provided' do
           let(:call_attributes) do
             {
-              status: 'on track',
-              statusExplanation: 'A magic dwells in each beginning.'
+              status_attributes: {
+                code: 'on_track',
+                explanation: 'A magic dwells in each beginning.'
+              }
             }
           end
 
@@ -288,8 +290,10 @@ describe Projects::SetAttributesService, type: :model do
           context 'with a value provided' do
             let(:call_attributes) do
               {
-                status: 'on_track',
-                statusExplanation: 'A magic dwells in each beginning.'
+                status_attributes: {
+                  code: 'on_track',
+                  explanation: 'A magic dwells in each beginning.'
+                }
               }
             end
 
@@ -312,8 +316,10 @@ describe Projects::SetAttributesService, type: :model do
           context 'with an invalid code' do
             let(:call_attributes) do
               {
-                status: 'bogus',
-                statusExplanation: 'A magic dwells in each beginning.'
+                status_attributes: {
+                  code: 'bogus',
+                  explanation: 'A magic dwells in each beginning.'
+                }
               }
             end
 
@@ -345,8 +351,10 @@ describe Projects::SetAttributesService, type: :model do
           context 'with a value provided' do
             let(:call_attributes) do
               {
-                status: 'at_risk',
-                statusExplanation: 'Still some magic there.'
+                status_attributes: {
+                  code: 'at_risk',
+                  explanation: 'Still some magic there.'
+                }
               }
             end