Use temporary negative IDs for rows created in a draft Changeset

app/Http/Controllers/TableController.php

@@ -213,7 +213,7 @@ class TableController extends Controller
         $rowsToInsert = null;
         $rowNumerator = null;
         try {
-            $rowsToInsert = Changeset::csvToRowsArray($columns, $dataCsvLines, true)->all();
+            $rowsToInsert = (new Changeset)->csvToRowsArray($columns, $dataCsvLines, true, false)->all();
         } catch (\Exception $e) {
             return $this->backWithErrors(['data' => $e->getMessage()]);
         }

app/Http/Controllers/TableEditController.php

@@ -334,6 +334,23 @@ class TableEditController extends Controller
      */
     public function draftSubmit(Request $request, User $user, string $table)
     {
+        /** @var Table $tableModel */
+        $tableModel = $user->tables()->where('name', $table)->first();
+        if ($tableModel === null) abort(404, "No such table.");
+
+        $input = $this->validate($request, [
+            'note' => 'required|string'
+        ]);
+
+        $changeset = $this->getChangeset($tableModel);
+        $changeset->note = trim($input->note);
+        if (empty($changeset->note)) throw new SimpleValidationException('note', 'Note is required.');
+
+        if (!$changeset->getRowChangeCounts()->any && !$changeset->getColumnChangeCounts()->any) {
+            flash()->error("There are no changes to submit.");
+            return back();
+        }
+
         //
     }
 }

app/Models/Proposal.php

@@ -79,7 +79,7 @@ class Proposal extends BaseModel
             throw new NotApplicableException('No changes to propose.');
         }
 
-        if ($changeset->note == null) {
+        if (empty($changeset->note)) {
             throw new NotApplicableException('Proposal note must be filled.');
         }
 
@@ -91,6 +91,9 @@ class Proposal extends BaseModel
             throw new NotApplicableException('Revision not assigned to Changeset');
         }
 
+        // Assign unique row IDs to new rows (they use temporary negative IDs in a draft)
+        $changeset->renumberRows();
+
         return new Proposal([
             // relations
             'table_id' => $changeset->table->getKey(),

app/Tables/BaseNumerator.php

@@ -3,7 +3,9 @@
 
 namespace App\Tables;
 
-
+/**
+ * Sequential ID assigner
+ */
 abstract class BaseNumerator
 {
     /** @var int */
@@ -39,10 +41,18 @@ abstract class BaseNumerator
             throw new \OutOfBoundsException("Column numerator has run out of allocated GCID slots");
 
         $key = $this->getKey($this->next);
-        $this->next++;
+        $this->advance();
         return $key;
     }
 
+    /**
+     * Advance to the next ID
+     */
+    protected function advance()
+    {
+        $this->next++;
+    }
+
     /**
      * Convert numeric index to a key
      *

app/Tables/Changeset.php

@@ -104,6 +104,14 @@ class Changeset
      */
     public $removedColumns = [];
 
+    /**
+     * Draft row numerator state holder; numbers grow to negative,
+     * and are replaced with real unique row IDs when the proposal is submitted.
+     *
+     * @var int
+     */
+    public $nextRowID = -1;
+
     /**
      * Generator iterating all properties, used internally for serialization to array
      *
@@ -160,22 +168,6 @@ class Changeset
         return $object;
     }
 
-    /**
-     * Check if there is any change in this changeset
-     *
-     * @return bool - any found
-     */
-    public function hasAnyChanges()
-    {
-        foreach ($this->walkProps() as $prop) {
-            if (!empty($this->$prop)) {
-                return true;
-            }
-        }
-
-        return false;
-    }
-
     /**
      * Decorate / transform a single row for the editor view
      *
@@ -504,6 +496,9 @@ class Changeset
             }
         }
 
+        // try creating a column with the new data
+        new Column(array_merge($col->toArray(), $updateObj));
+
         if ($this->isNewColumn($id)) {
             $this->newColumns[$id] = array_merge($this->newColumns[$id], $updateObj);
         }
@@ -569,7 +564,7 @@ class Changeset
      *                               so it can be inserted directly into DB
      * @return Collection
      */
-    public static function csvToRowsArray($columns, $csvArray, $forTableInsert)
+    public function csvToRowsArray($columns, $csvArray, $forTableInsert, $useDraftIds)
     {
         /** @var Collection $rows */
         $rows = collect($csvArray)->map(function ($row) use ($columns, $forTableInsert) {
@@ -597,9 +592,11 @@ class Changeset
             }
         })->filter();
 
-        // TODO we could use some temporary IDs and replace them with
+        if ($useDraftIds) {
-        // TODO   proper unique IDs when submitting the proposal
+            $rowNumerator = new DraftRowNumerator($this, count($csvArray));
+        } else {
             $rowNumerator = new RowNumerator(count($csvArray));
+        }
 
         if ($forTableInsert) {
             return $rows->map(function ($row) use (&$rowNumerator) {
@@ -628,9 +625,7 @@ class Changeset
             $template[$column->id] = $column->cast(null);
         }
 
-        // TODO we could use some temporary IDs and replace them with
+        $numerator = new DraftRowNumerator($this, $count);
-        // TODO   proper unique IDs when submitting the proposal
-        $numerator = new RowNumerator($count);
 
         foreach ($numerator->generate() as $_id) {
             $row = $template;
@@ -649,7 +644,7 @@ class Changeset
         /** @var Column[] $columns */
         $columns = array_values($this->fetchAndTransformColumns());
 
-        $rows = self::csvToRowsArray($columns, $csvArray, false)
+        $rows = self::csvToRowsArray($columns, $csvArray, false, true)
             ->keyBy('_id');
 
         // using '+' to avoid renumbering
@@ -803,4 +798,77 @@ class Changeset
     {
         $this->rowUpdates = [];
     }
+
+    /**
+     * Get row change counts (used for the view)
+     *
+     * @return object
+     */
+    public function getRowChangeCounts()
+    {
+        $numChangedRows = count($this->rowUpdates);
+        $numNewRows = count($this->newRows);
+        $numRemovedRows = count($this->removedRows);
+
+        return (object) [
+            'changed' => $numChangedRows,
+            'new' => $numNewRows,
+            'removed' => $numRemovedRows,
+            'any' => $numChangedRows || $numNewRows || $numRemovedRows,
+        ];
+    }
+
+    /**
+     * Get column change counts (used for the view)
+     *
+     * @return object
+     */
+    public function getColumnChangeCounts()
+    {
+        $numChangedColumns = count($this->columnUpdates);
+        $numNewColumns = count($this->newColumns);
+        $numRemovedColumns = count($this->removedColumns);
+        $reordered = count($this->columnOrder) != 0;
+
+        return (object) [
+            'changed' => $numChangedColumns,
+            'new' => $numNewColumns,
+            'removed' => $numRemovedColumns,
+            'reordered' => $reordered,
+            'any' => $reordered || $numChangedColumns || $numNewColumns || $numRemovedColumns,
+        ];
+    }
+
+    /**
+     * Check if there is any change in this changeset
+     *
+     * @return bool - any found
+     */
+    public function hasAnyChanges()
+    {
+        $colChanges = $this->getColumnChangeCounts();
+        $rowChanges = $this->getRowChangeCounts();
+
+        return $colChanges->any || $rowChanges->any;
+    }
+
+    /**
+     * Replace temporary negative row IDs with real unique row IDs
+     */
+    public function renumberRows()
+    {
+        $rows = [];
+        $numerator = new RowNumerator(count($this->newRows));
+        foreach ($this->newRows as $row) {
+            if ($row['_id'] < 0) {
+                $id = $numerator->next();
+                $row['_id'] = $id;
+                $rows[$id] = $row;
+            } else {
+                // keep ID
+                $rows[$row['_id']] = $row;
+            }
+        }
+        $this->newRows = $rows;
+    }
 }

app/Tables/Column.php

@@ -125,27 +125,27 @@ class Column implements JsonSerializable, Arrayable
      *
      * @param object|array $obj
      */
-    public function __construct($obj, $allowEmptyName=false)
+    public function __construct($obj)
     {
         $b = objBag($obj);
 
-        if (!$allowEmptyName && (!$b->has('name') || $b->name == '')) {
+        if (!$b->has('name') || $b->name == '') {
-            throw new NotApplicableException('Missing name in column');
+            throw new SimpleValidationException('name', "Missing column name.");
         }
 
-        if (!$b->has('type')) {
+        if (!$b->has('type') || !in_array($b->type, self::colTypes)) {
-            throw new NotApplicableException('Missing type in column');
+            throw new SimpleValidationException('type', "Missing or invalid column type.");
         }
 
-        if ($b->name[0] == '_') { // would cause problems with selects (see: _id / GRID)
+        if ($b->name[0] == '_' || !preg_match(IDENTIFIER_PATTERN, $b->name)) {
-            throw new NotApplicableException("Column name can't start with underscore.");
+            throw new SimpleValidationException('name', "Column name can contain only letters, digits, and underscore, and must start with a letter.");
         }
 
-        if (!in_array($b->type, self::colTypes)) {
+        if (!$b->has('id') || $b->id[0] == '_') {
-            throw new NotApplicableException("\"$b->type\" is not a valid column type.");
+            throw new SimpleValidationException('id', "Invalid or missing column ID");
         }
 
-        $this->id = $b->get('id', null);
+        $this->id = $b->id;
         $this->name = $b->name;
         $this->type = $b->type;
         $this->title = $b->title ?: $b->name;

app/Tables/DraftRowNumerator.php

@@ -0,0 +1,22 @@
+<?php
+
+
+namespace App\Tables;
+
+/**
+ * Utility for allocating & assigning temporary row IDs
+ * for rows in a changeset
+ */
+class DraftRowNumerator extends BaseNumerator
+{
+    /**
+     * Create a numerator for the given number of rows.
+     *
+     * @param int $capacity - how many
+     */
+    public function __construct(Changeset $changeset, $capacity)
+    {
+        parent::__construct([$changeset->nextRowID - $capacity + 1, $changeset->nextRowID]);
+        $changeset->nextRowID -= $capacity;
+    }
+}

app/Tables/RowNumerator.php

@@ -5,6 +5,9 @@ namespace App\Tables;
 
 use App\Models\Row;
 
+/**
+ * Utility for allocating & assigning globally unique row IDs
+ */
 class RowNumerator extends BaseNumerator
 {
     /**

porklib/helpers.php

@@ -16,7 +16,7 @@ use MightyPork\Utils\Utils;
 
 #region Defines
 
-define('IDENTIFIER_PATTERN', '/^[a-z][a-z0-9_]*$/i');
+define('IDENTIFIER_PATTERN', '/^[a-z_][a-z0-9_]*$/i');
 define('USERNAME_PATTERN', '/^[a-z0-9_-]+$/i');
 
 #endregion

resources/assets/js/components/ColumnEditor.vue

@@ -61,20 +61,26 @@ Complex animated column editor for the table edit page
               <!-- Editable cells -->
               <td :style="tdWidthStyle('name')">
                 <input v-model="col.name"
-                       class="form-control"
+                       :class="['form-control', { 'is-invalid': col._errors && col._errors['name'] }]"
+                       :title="(col._errors && col._errors['name']) ? col._errors['name'][0] : null"
                        type="text">
               </td>
 
               <td :style="tdWidthStyle('type')">
                 <select v-model="col.type"
-                        class="form-control custom-select">
+                        :title="(col._errors && col._errors['type']) ? col._errors['type'][0] : null"
+                        :class="[
+                          'form-control',
+                          'custom-select',
+                          { 'is-invalid': col._errors && col._errors['type'] }]">
                   <option v-for="t in colTypes" :value="t">{{t}}</option>
                 </select>
               </td>
 
               <td :style="tdWidthStyle('title')">
                 <input v-model="col.title"
-                       class="form-control"
+                       :title="(col._errors && col._errors['title']) ? col._errors['title'][0] : null"
+                       :class="['form-control', { 'is-invalid': col._errors && col._errors['title'] }]"
                        type="text">
               </td>
             </template>
@@ -262,6 +268,7 @@ export default {
       }, (resp) => {
         this.$set(this.columns, n, resp.data)
       }, (er) => {
+        console.log("Col save error: ", er)
         if (!_.isUndefined(er.errors)) {
           this.$set(this.columns[n], '_errors', er.errors)
         }

resources/views/table/propose/manage-columns.blade.php

@@ -18,6 +18,7 @@
         name: 'columns',
         route: {!! toJSON($table->draftUpdateRoute) !!},
         xColumns: {!! toJSON($columns) !!},
+        orderChanged: @json(!empty($changeset->columnOrder))
       })
     });
   </script>

resources/views/table/propose/review.blade.php

@@ -3,20 +3,10 @@
   /** @var \App\Tables\Changeset $changeset */
   /** @var \App\Models\Table $table */
 
-  $numChangedRows = count($changeset->rowUpdates);
+  $rowChanges = $changeset->getRowChangeCounts();
-  $numNewRows = count($changeset->newRows);
+  $columnChanges = $changeset->getColumnChangeCounts();
-  $numRemovedRows = count($changeset->removedRows);
 
-  $anyRowChanges = $numChangedRows || $numNewRows || $numRemovedRows;
+  $anyChanges = ($rowChanges->any || $columnChanges->any) && strlen(trim($changeset->note)) > 0;
-
-  $numChangedColumns = count($changeset->columnUpdates);
-  $numNewColumns = count($changeset->newColumns);
-  $numRemovedColumns = count($changeset->removedColumns);
-  $colsReordered = !empty($changeset->columnOrder);
-
-  $anyColChanges = $numChangedColumns || $numNewColumns || $numRemovedColumns || $colsReordered;
-
-  $anyChanges = ($anyRowChanges || $anyColChanges) && strlen(trim($changeset->note)) > 0;
 @endphp
 
 @extends('table.propose.layout')
@@ -35,27 +25,27 @@
           Rows
         </div>
         <div class="col-md-7">
-          @if($anyRowChanges)
+          @if($rowChanges->any)
 
-            @if($numChangedRows)
+            @if($rowChanges->changed)
               <div class="text-info">
-                <b>{{ $numChangedRows }}</b> changed
+                <b>{{ $rowChanges->changed }}</b> changed
                 <a href="{{$table->draftUpdateRoute}}?action=reset.row-update" class="ml-2"
                    data-confirm="Revert all row changes?">Reset</a>
               </div>
             @endif
 
-            @if($numNewRows)
+            @if($rowChanges->new)
              <div class="text-success">
-               <b>{{ $numNewRows }}</b> new
+               <b>{{ $rowChanges->new }}</b> new
                <a href="{{$table->draftUpdateRoute}}?action=reset.row-add" class="ml-2"
                   data-confirm="Discard all added rows?">Reset</a>
              </div>
             @endif
 
-            @if($numRemovedRows)
+            @if($rowChanges->removed)
              <div class="text-danger">
-               <b>{{ $numRemovedRows }}</b> removed
+               <b>{{ $rowChanges->removed }}</b> removed
                <a href="{{$table->draftUpdateRoute}}?action=reset.row-remove" class="ml-2"
                   data-confirm="Undo row removals?">Reset</a>
              </div>
@@ -72,33 +62,33 @@
           Columns
         </div>
         <div class="col-md-7">
-          @if($anyColChanges)
+          @if($columnChanges->any)
 
-            @if($numChangedColumns)
+            @if($columnChanges->changed)
               <div class="text-info">
-                <b>{{ $numChangedColumns }}</b> changed
+                <b>{{ $columnChanges->changed }}</b> changed
                 <a href="{{$table->draftUpdateRoute}}?action=reset.col-update" class="ml-2"
                    data-confirm="Revert all column changes?">Reset</a>
               </div>
             @endif
 
-            @if($numNewColumns)
+            @if($columnChanges->new)
               <div class="text-success">
-                <b>{{ $numNewColumns }}</b> new
+                <b>{{ $columnChanges->new }}</b> new
                 <a href="{{$table->draftUpdateRoute}}?action=reset.col-add" class="ml-2"
                    data-confirm="Discard all added columns?">Reset</a>
               </div>
             @endif
 
-            @if($numRemovedColumns)
+            @if($columnChanges->removed)
               <div class="text-danger">
-                <b>{{ $numRemovedColumns }}</b> removed
+                <b>{{ $columnChanges->removed }}</b> removed
                 <a href="{{$table->draftUpdateRoute}}?action=reset.col-remove" class="ml-2"
                    data-confirm="Undo column removals?">Reset</a>
               </div>
             @endif
 
-            @if($colsReordered)
+            @if($columnChanges->reordered)
               <div class="text-info">
                 Order changed
                 <a href="{{$table->draftUpdateRoute}}?action=reset.col-order" class="ml-2"
@@ -131,7 +121,7 @@
             @endif
           </button>
 
-          @if($anyRowChanges || $anyColChanges)
+          @if($rowChanges->any || $columnChanges->any)
             <span class="text-muted ml-3" id="empty-note-prompt">Write a summary to submit your changes.</span>
           @else
             <span class="text-danger ml-3">No changes to submit.</span>
@@ -148,7 +138,7 @@
     ready(function () {
       app.DraftNotePage({
         route: @json($table->draftUpdateRoute),
-        anyChanges: @json($anyRowChanges || $anyColChanges)
+        anyChanges: @json($rowChanges->any || $columnChanges->any)
       })
     })
   </script>