Merge pull request #4225 from oliverguenther/feature/expand-error-columns

[22556] Error handling for other required/invalid fields of a WP row

app/assets/stylesheets/content/_in_place_editing.sass

@@ -158,7 +158,8 @@
   @include grid-block
   overflow: visible
 
-.inplace-edit--read-value
+.inplace-edit--read-value,
+.wp-edit-field
   @include grid-block
   padding: 0.375rem
 
@@ -222,7 +223,8 @@ a.inplace-edit--icon-wrapper
 // need to specify the a explicitly as otherwise
 // the default class will win
 a.inplace-editing--trigger-link,
-.inplace-editing--trigger-link
+.inplace-editing--trigger-link,
+td.-editable
   @include grid-block
   color: $body-font-color
   font-weight: inherit
@@ -235,8 +237,13 @@ a.inplace-editing--trigger-link,
     text-decoration: none
     color: $body-font-color
 
-    .inplace-editing--container
+    .inplace-editing--container,
+    .wp-edit-field
       border-color: $inplace-edit--border-color
+
+      &.-active
+        border-color: transparent
+
     .inplace-edit--icon-wrapper
       visibility: visible
 
@@ -245,9 +252,10 @@ a.inplace-editing--trigger-link,
   .inplace-edit--read-value
     display: block
 
-.inplace-editing--container
+.inplace-editing--container,
+.wp-edit-field
   @include grid-block
-  border-color: #fff
+  border-color: transparent
   border-style: solid
   border-radius: 2px
   border-width: 1px
@@ -370,3 +378,24 @@ a.inplace-editing--trigger-link,
 
       i
         vertical-align: text-top
+
+
+.inplace-edit.-small .inplace-edit--read,
+.inplace-edit.-small .inplace-edit--write,
+.wp-edit-field.-small
+  height: 30px
+  padding-bottom: 3px
+  padding-top: 3px
+
+  .inplace-edit--read-value
+    padding-top: 0
+    padding-bottom: 0
+
+  select, input
+    height: 28px
+    padding-top: 0
+    padding-bottom: 0
+    line-height: normal
+
+.inplace-edit.-small
+  margin-top: 3px

app/assets/stylesheets/content/_work_packages_table_edit.sass

@@ -0,0 +1,63 @@
+//-- copyright
+// OpenProject is a project management system.
+// Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+//
+// This program is free software; you can redistribute it and/or
+// modify it under the terms of the GNU General Public License version 3.
+//
+// OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+// Copyright (C) 2006-2013 Jean-Philippe Lang
+// Copyright (C) 2010-2013 the ChiliProject Team
+//
+// This program is free software; you can redistribute it and/or
+// modify it under the terms of the GNU General Public License
+// as published by the Free Software Foundation; either version 2
+// of the License, or (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program; if not, write to the Free Software
+// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+//
+// See doc/COPYRIGHT.rdoc for more details.
+//++
+
+.-editable
+  .wp-edit-field
+    &.-active
+      padding: 0
+
+    &.-error
+      background: $nm-color-error-background
+      border-color: $nm-color-error-border
+
+      &:hover
+        border-color: lighten($nm-color-error-border, 10%)
+
+    form
+      width: 100%
+
+  &:hover .wp-edit-field.-error:hover
+    border-color: $nm-color-error-border
+
+.wp-table--cell
+  cursor: not-allowed
+
+  &.-editable
+    cursor: pointer
+
+.work-package-table--container .generic-table tbody
+  td
+    .wp-edit-field .-hidden-overflow
+      overflow: hidden
+      text-overflow: ellipsis
+    &.-editable
+      padding-top: 0
+      padding-bottom: 0
+      display: table-cell
+      width: auto
+

app/assets/stylesheets/default.css.sass

@@ -69,6 +69,7 @@
 @import content/simple_filters
 @import content/advanced_filters
 @import content/work_packages_table
+@import content/work_packages_table_edit
 @import content/attributes_key_value
 @import content/attributes_group
 @import content/attributes_table

frontend/app/components/api/api-v3/api-v3.config.ts

@@ -42,6 +42,10 @@ function apiV3Config(apiV3, halTransform) {
 
     return data;
   });
+
+  apiV3.setErrorInterceptor(response => {
+    response.data = halTransform(response.data);
+  });
 }
 
 angular

frontend/app/components/api/api-v3/hal-transform/hal-transform.config.ts

@@ -26,11 +26,12 @@
 // See doc/COPYRIGHT.rdoc for more details.
 // ++
 
-function halTransformConfig(halTransformTypes, HalResource, WorkPackageResource, CollectionResource) {
+function halTransformConfig(halTransformTypes, HalResource, WorkPackageResource, CollectionResource, ErrorResource) {
   angular.extend(halTransformTypes, {
     'default': HalResource,
     WorkPackage: WorkPackageResource,
-    Collection: CollectionResource
+    Collection: CollectionResource,
+    Error: ErrorResource
   });
 }
 

frontend/app/components/api/api-v3/hal/error-resource.service.ts

@@ -0,0 +1,68 @@
+//-- copyright
+// OpenProject is a project management system.
+// Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+//
+// This program is free software; you can redistribute it and/or
+// modify it under the terms of the GNU General Public License version 3.
+//
+// OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+// Copyright (C) 2006-2013 Jean-Philippe Lang
+// Copyright (C) 2010-2013 the ChiliProject Team
+//
+// This program is free software; you can redistribute it and/or
+// modify it under the terms of the GNU General Public License
+// as published by the Free Software Foundation; either version 2
+// of the License, or (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program; if not, write to the Free Software
+// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+//
+// See doc/COPYRIGHT.rdoc for more details.
+//++
+
+function errorResource(HalResource:typeof op.HalResource, NotificationsService:any) {
+  class ErrorResource extends HalResource {
+    public errors:any[];
+    public message:string;
+    public details:any;
+    public errorIdentifier:string;
+
+    public get errorMessages():string[] {
+      if (this.isMultiErrorMessage()) {
+        return this.errors.map(error => error.message);
+      } else {
+        return [this.message];
+      }
+    }
+
+    public isMultiErrorMessage() {
+      return this.errorIdentifier === 'urn:openproject-org:api:v3:errors:MultipleErrors';
+    }
+
+    public showErrorNotification() {
+      var messages = this.errorMessages;
+      if (messages.length > 1) {
+        NotificationsService.addError('', messages);
+      } else {
+        NotificationsService.addError(messages[0]);
+      }
+    }
+
+    public getInvolvedColumns():string[] {
+      var columns = this.details ? [{ details: this.details }] : this.errors;
+      return columns.map(field => field.details.attribute);
+    }
+  }
+
+  return ErrorResource;
+}
+
+angular
+  .module('openproject.api')
+  .factory('ErrorResource', errorResource);

frontend/app/components/query/query-service.service.ts

@@ -213,6 +213,10 @@ function QueryService(Query,
       return this.getQuery().getSelectedColumns();
     },
 
+    getSelectedColumnNames: function() {
+      return this.getSelectedColumns().map(column => column.name);
+    },
+
     setSelectedColumns: function(selectedColumnNames) {
       query.dirty = true;
       var currentColumns = this.getSelectedColumns();

frontend/app/components/wp-edit/wp-edit-field/wp-edit-field.directive.html

@@ -1,4 +1,5 @@
-<div ng-click="vm.isEditable && vm.activate()" class="wp-edit-field">
+<div ng-click="vm.isEditable && vm.activate()" class="wp-edit-field -small"
+     ng-class="{ '-error': vm.errorenous, '-active': vm.active }">
   <form ng-if="vm.workPackage && vm.active"
         ng-submit="vm.submit()">
 
@@ -6,5 +7,5 @@
 
   </form>
 
-  <ng-transclude ng-hide="vm.active"></ng-transclude>
+  <ng-transclude class="-hidden-overflow" ng-hide="vm.active"></ng-transclude>
 </div>

frontend/app/components/wp-edit/wp-edit-field/wp-edit-field.directive.ts

@@ -32,13 +32,14 @@ import {Field} from "./wp-edit-field.module";
 
 
 export class WorkPackageEditFieldController {
-  public formCtrl: WorkPackageEditFormController;
+  public formCtrl:WorkPackageEditFormController;
   public fieldName:string;
   public field:Field;
+  public errorenous:boolean;
 
   protected _active:boolean = false;
 
-  constructor(protected wpEditField:WorkPackageEditFieldService) {
+  constructor(protected wpEditField:WorkPackageEditFieldService, protected $element) {
   }
 
   public get workPackage() {
@@ -50,9 +51,8 @@ export class WorkPackageEditFieldController {
   }
 
   public submit() {
-    this.formCtrl.updateWorkPackage().then(() => {
+    this.formCtrl.updateWorkPackage()
-      this.deactivate();
+      .then(() => this.deactivate());
-    });
   }
 
   public activate() {
@@ -69,22 +69,37 @@ export class WorkPackageEditFieldController {
     return this._active = false;
   }
 
+  public setErrorState(error = true) {
+    this.errorenous = error;
+    this.$element.toggleClass('-error', error)
+  }
+
   protected setupField():ng.IPromise<any> {
-    return this.formCtrl.loadSchema().then(schema =>  {
+    return this.formCtrl.loadSchema().then(schema => {
       this.field = this.wpEditField.getField(
         this.workPackage, this.fieldName, schema[this.fieldName]);
     });
   }
 }
 
-function wpEditFieldLink(scope, element, attrs, controllers:
+function wpEditFieldLink(scope, element, attrs, controllers:[WorkPackageEditFormController, WorkPackageEditFieldController]) {
-                           [WorkPackageEditFormController, WorkPackageEditFieldController]) {
 
   controllers[1].formCtrl = controllers[0];
+  controllers[1].formCtrl.fields[scope.vm.fieldName] = scope.vm;
 
   element.click(event => {
     event.stopImmediatePropagation();
   });
+
+  // Mark the td field if it is inline-editable
+  // We're resolving the non-form schema here since its loaded anyway for the table
+  scope.vm.formCtrl.loadSchema().then(schema => {
+    if (schema[scope.vm.fieldName].writable) {
+      element.addClass('-editable');
+    }
+  })
+
+  element.addClass(scope.vm.fieldName);
 }
 
 function wpEditField() {

frontend/app/components/wp-edit/wp-edit-form.directive.ts

@@ -28,8 +28,9 @@
 
 export class WorkPackageEditFormController {
   public workPackage;
+  public fields = {};
 
-  constructor(protected NotificationsService, protected $q) {
+  constructor(protected NotificationsService, protected $q, protected QueryService, protected $timeout) {
   }
 
   public loadSchema() {
@@ -40,21 +41,46 @@ export class WorkPackageEditFormController {
     var deferred = this.$q.defer();
 
     this.workPackage.save()
-    .then(() => {
+      .then(() => {
-      deferred.resolve();
+        angular.forEach(this.fields, field => field.setErrorState(false));
-    })
+        deferred.resolve();
-    .catch(error => {
+      })
-      if (error && error.data && error.data.message) {
+      .catch((error) => {
-        this.NotificationsService.addError(error.data.message);
+        if (!error.data) {
-      } else {
+          this.NotificationsService.addError("An internal error has occcurred.");
-        this.NotificationsService.addError("An internal error has occcurred.");
+          return deferred.reject([]);
-      }
+        }
 
-      return deferred.reject();
+        error.data.showErrorNotification();
-    });
+        this.handleSubmissionErrors(error.data, deferred)
+      });
 
     return deferred.promise;
   }
+
+  private handleSubmissionErrors(error:any, deferred:any) {
+
+    // Process single API errors
+    this.handleErrorenousColumns(error.getInvolvedColumns());
+    return deferred.reject();
+  }
+
+  private handleErrorenousColumns(columns:string[]) {
+    var selected = this.QueryService.getSelectedColumnNames();
+    var active = _.find(this.fields, (f:any) => f.active);
+    columns.reverse().map(name => {
+      if (selected.indexOf(name) === -1) {
+        selected.splice(selected.indexOf(active.fieldName) + 1, 0, name);
+      }
+    });
+
+    this.QueryService.setSelectedColumns(selected);
+    this.$timeout(_ => {
+      angular.forEach(this.fields, (field) => {
+        field.setErrorState(columns.indexOf(field.fieldName) !== -1);
+      });
+    });
+  }
 }
 
 function wpEditForm() {

frontend/app/components/wp-table/wp-table.directive.html

@@ -111,10 +111,10 @@
           </td>
 
           <td ng-repeat="column in columns"
-              class="{{column.name}}"
               lang="{{column.custom_field && column.custom_field.name_locale || locale}}"
               wp-edit-field="column.name"
-              ng-class="column.name == 'id' && '-short'">
+              class="wp-table--cell"
+              ng-class="{ '-short': column.name == 'id' }">
             <wp-td attribute="column.name"
                    schema="row.object.schema"
                    object="row.object"

frontend/app/components/wp-table/wp-td/wp-td.directive.html

@@ -1,4 +1,7 @@
-<span ng-switch="vm.displayType" wp-field="vm.workPackage" field-name="vm.attribute">
+<span class="wp-table--cell-span"
+      ng-switch="vm.displayType"
+      wp-field="vm.workPackage"
+      field-name="vm.attribute">
   <progress-bar ng-switch-when="Percent"
                 progress="vm.displayText"
                 width="80px">

spec/features/work_packages/inline_edit/edit_work_packages_spec.rb

@@ -0,0 +1,117 @@
+require 'spec_helper'
+
+describe 'Inline editing work packages', js: true do
+  let(:dev_role) do
+    FactoryGirl.create :role,
+                       permissions: [:view_work_packages,
+                                     :add_work_packages]
+  end
+  let(:dev) do
+    FactoryGirl.create :user,
+                       firstname: 'Dev',
+                       lastname: 'Guy',
+                       member_in_project: project,
+                       member_through_role: dev_role
+  end
+  let(:manager_role) do
+    FactoryGirl.create :role,
+                       permissions: [:view_work_packages,
+                                     :edit_work_packages]
+  end
+  let(:manager) do
+    FactoryGirl.create :user,
+                       firstname: 'Manager',
+                       lastname: 'Guy',
+                       member_in_project: project,
+                       member_through_role: manager_role
+  end
+  let(:type) { FactoryGirl.create :type }
+  let(:type2) { FactoryGirl.create :type }
+  let(:project) { FactoryGirl.create(:project, types: [type, type2]) }
+  let(:work_package) {
+    FactoryGirl.create(:work_package,
+                       author: dev,
+                       project: project,
+                       type: type,
+                       subject: 'Foobar')
+  }
+
+  let(:wp_table) { Pages::WorkPackagesTable.new(project) }
+  let(:fields) { InlineEditField.new(wp_table, work_package) }
+
+  let(:priority2) { FactoryGirl.create :priority }
+  let(:status2) { FactoryGirl.create :status }
+  let(:workflow) do
+    FactoryGirl.create :workflow,
+                       type_id: type2.id,
+                       old_status: work_package.status,
+                       new_status: status2,
+                       role: manager_role
+  end
+  let(:version) { FactoryGirl.create :version, project: project }
+  let(:category) { FactoryGirl.create :category, project: project }
+
+  before do
+    login_as(manager)
+
+    manager
+    dev
+    priority2
+    workflow
+
+    wp_table.visit!
+    wp_table.expect_work_package_listed(work_package)
+  end
+
+  it 'allows updating and seeing the results' do
+    subject_field = wp_table.edit_field(work_package, :subject)
+    subject_field.expect_text('Foobar')
+
+    subject_field.activate!
+
+    subject_field.set_value('New subject!')
+
+    expect(UpdateWorkPackageService).to receive(:new).and_call_original
+    subject_field.save!
+    subject_field.expect_text('New subject!')
+
+    work_package.reload
+    expect(work_package.subject).to eq('New subject!')
+  end
+
+  it 'allows to subsequently edit multiple fields' do
+    subject_field = wp_table.edit_field(work_package, :subject)
+    priority_field = wp_table.edit_field(work_package, :priority)
+
+    expect(UpdateWorkPackageService).to receive(:new).and_call_original
+    subject_field.activate!
+    subject_field.set_value('Other subject!')
+
+    priority_field.activate!
+    priority_field.set_value(priority2.name)
+    priority_field.expect_inactive!
+
+    subject_field.expect_text('Other subject!')
+    priority_field.expect_text(priority2.name)
+
+    work_package.reload
+    expect(work_package.subject).to eq('Other subject!')
+    expect(work_package.priority.id).to eq(priority2.id)
+  end
+
+  it 'provides error handling' do
+    subject_field = wp_table.edit_field(work_package, :subject)
+    subject_field.expect_text('Foobar')
+
+    subject_field.activate!
+
+    subject_field.set_value('')
+
+    expect(UpdateWorkPackageService).to receive(:new).and_call_original
+    subject_field.save!
+    subject_field.expect_error
+
+    work_package.reload
+    expect(work_package.subject).to eq('Foobar')
+  end
+end

spec/support/pages/work_packages_table.rb

@@ -27,6 +27,7 @@
 #++
 
 require 'support/pages/page'
+require 'support/work_packages/inline_edit_field'
 
 module Pages
   class WorkPackagesTable < Page
@@ -59,6 +60,14 @@ module Pages
       FullWorkPackage.new(work_package)
     end
 
+    def row(work_package)
+      table_container.find("#work-package-#{work_package.id}")
+    end
+
+    def edit_field(work_package, attribute)
+      InlineEditField.new(work_package, attribute)
+    end
+
     private
 
     def path
@@ -68,9 +77,5 @@ module Pages
     def table_container
       find('#content .work-package-table--container')
     end
-
-    def row(work_package)
-      table_container.find("#work-package-#{work_package.id}")
-    end
   end
 end

spec/support/work_packages/inline_edit_field.rb

@@ -0,0 +1,85 @@
+class InlineEditField
+  include Capybara::DSL
+  include RSpec::Matchers
+
+  attr_reader :work_package, :attribute, :element, :selector
+
+  def initialize(work_package, attribute, field_type: nil)
+    @work_package = work_package
+    @attribute = attribute
+    @field_type = field_type
+
+    @selector = "#work-package-#{work_package.id} .#{attribute}"
+    @element = page.find(selector)
+  end
+
+  def expect_text(text)
+    expect(page).to have_selector(selector, text: text)
+  end
+
+  ##
+  # Activate the field and check it opened correctly
+  def activate!
+    edit_field.click
+    expect_active!
+  end
+
+  ##
+  # Set or select the given value.
+  # For fields of type select, will check for an option with that value.
+  def set_value(content)
+    if field_type == 'select'
+      input_field.find(:option, content).select_option
+    else
+      input_field.set(content)
+    end
+  end
+
+  def expect_error
+    expect(page).to have_selector("#{field_selector}.-error")
+  end
+
+  def expect_active!
+    expect(edit_field).to have_selector(field_type)
+  end
+
+  def expect_inactive!
+    expect(edit_field).to have_no_selector(field_type)
+  end
+
+  def save!
+    input_field.native.send_keys(:return)
+    reset_field
+  end
+
+  def edit_field
+    @edit_field ||= @element.find('.wp-edit-field')
+  end
+
+  def input_field
+    @input_field ||= edit_field.find(field_type)
+  end
+
+  private
+
+  def field_selector
+    "#{selector} .wp-edit-field"
+  end
+
+  ##
+  # Reset the input field e.g., after saving
+  def reset_field
+    @input_field = nil
+  end
+
+  def field_type
+    @field_type ||= begin
+      case attribute
+      when :assignee, :priority, :status
+        :select
+      else
+        :input
+      end.to_s
+    end
+  end
+end