fix: undoing and redoing deleting procedures (#6722)

* fix: undoing and redoing deleting procedure defs

* chore: added tests for undoing and redoing procedure def deletes

* fix: dragging blocks from flyout

* chore: PR comments

* chore: fixup from rebase

blocks/procedures.js

@@ -308,6 +308,7 @@ const procedureDefGetDefMixin = function() {
      * disposed.
      */
     destroy: function() {
+      if (this.isInsertionMarker()) return;
       this.workspace.getProcedureMap().delete(this.getProcedureModel().getId());
     },
   };
@@ -598,10 +599,12 @@ const procedureDefMutator = {
    *     parameters and statements.
    */
   saveExtraState: function() {
-    const params = this.getProcedureModel().getParameters();
-    if (!params.length && this.hasStatements_) return null;
-
     const state = Object.create(null);
+    state['procedureId'] = this.getProcedureModel().getId();
+
+    const params = this.getProcedureModel().getParameters();
+    if (!params.length && this.hasStatements_) return state;
+
     if (params.length) {
       state['params'] = params.map((p) => {
         return {
@@ -625,6 +628,16 @@ const procedureDefMutator = {
    *     statements.
    */
   loadExtraState: function(state) {
+    const map = this.workspace.getProcedureMap();
+    const procedureId = state['procedureId'];
+    if (procedureId && procedureId != this.model_.getId() &&
+        map.has(procedureId)) {
+      if (map.has(this.model_.getId())) {
+        map.delete(this.model_.getId());
+      }
+      this.model_ = map.get(procedureId);
+    }
+
     if (state['params']) {
       for (let i = 0; i < state['params'].length; i++) {
         const {name, id, paramId} = state['params'][i];

core/procedures.ts

@@ -193,7 +193,9 @@ export function rename(this: Field, name: string): string {
   name = name.trim();
 
   const legalName = findLegalName(name, block);
-  if (isProcedureBlock(block)) block.getProcedureModel().setName(legalName);
+  if (isProcedureBlock(block) && !block.isInsertionMarker()) {
+    block.getProcedureModel().setName(legalName);
+  }
   const oldName = this.getValue();
   if (oldName !== name && oldName !== legalName) {
     // Rename any callers.

tests/mocha/blocks/procedures_test.js

@@ -1338,6 +1338,35 @@ suite('Procedures', function() {
           chai.assert.isTrue(
               callBlock2.disposed, 'Expected the second caller to be disposed');
         });
+
+    test('undoing and redoing a procedure delete will still associate ' +
+        'procedure and caller with the same model',
+        function() {
+          const defBlock = createProcDefBlock(this.workspace);
+          createProcCallBlock(this.workspace);
+          // TODO: Apparently we need to call checkAndDelete to handle event
+          //   grouping, this seems like possibly a bug.
+          const oldModel = defBlock.getProcedureModel();
+          defBlock.checkAndDelete();
+          this.clock.runAll();
+
+          this.workspace.undo();
+          this.clock.runAll();
+
+          const newDefBlock =
+              this.workspace.getBlocksByType('procedures_defnoreturn')[0];
+          const newCallBlock =
+              this.workspace.getBlocksByType('procedures_callnoreturn')[0];
+
+          chai.assert.equal(
+              newDefBlock.getProcedureModel(),
+              newCallBlock.getProcedureModel(),
+              'Expected both new blocks to be associated with the same model');
+          chai.assert.equal(
+            oldModel.getId(),
+            newDefBlock.getProcedureModel().getId(),
+            'Expected the new model to have the same ID as the old model');
+        });
   });
 
   suite('caller blocks creating new def blocks', function() {