PTWin\ProjectEdit.cs: .ApplyEdit() and .Save() should appear as one transaction

so24wo posted on Sunday, January 06, 2008

This issue is addressed at least to:
PTWin\RolesEdis.cs, up to rev.1900 (current)
PTWin\WinPart.cs, up to rev.1697 (current)
.Net 2.0

Let's view the code from ProjectEdit.cs (rev.1900):

    private void RebindUI(bool saveObject, bool rebind)
    {
      // disable events
      this.projectBindingSource.RaiseListChangedEvents = false;
      this.resourcesBindingSource.RaiseListChangedEvents = false;
      try
      {
        // unbind the UI
        UnbindBindingSource(this.resourcesBindingSource, saveObject, false);
        UnbindBindingSource(this.projectBindingSource, saveObject, true);
        this.resourcesBindingSource.DataSource = this.projectBindingSource;

        // save or cancel changes
        if (saveObject)
        {
          _project.ApplyEdit();
          try
          {
            _project = _project.Save();
          }
          catch (Csla.DataPortalException ex)
          {
            MessageBox.Show(ex.BusinessException.ToString(),
              "Error saving", MessageBoxButtons.OK,
              MessageBoxIcon.Exclamation);
          }
          catch (Exception ex)
          {
            MessageBox.Show(ex.ToString(),
              "Error Saving", MessageBoxButtons.OK,
              MessageBoxIcon.Exclamation);
          }
        }
        else
          _project.CancelEdit();

      }
      finally
      {
        // rebind UI if requested
        if (rebind)
          BindUI();

        // restore events
        this.projectBindingSource.RaiseListChangedEvents = true;
        this.resourcesBindingSource.RaiseListChangedEvents = true;

        if (rebind)
        {
          // refresh the UI if rebinding
          this.projectBindingSource.ResetBindings(false);
          this.resourcesBindingSource.ResetBindings(false);
        }
      }
    }

There is one trouble with this code.

Suppose a user changes a _project object with UI and clicks the Save button.
What happens if the .Save() method will throw an exception?
After an exception messagebox will be closed a user will not be able to undo changes
because before call to .Save() we've called the .ApplyEdit() method on our object.

So these two calls - .ApplyEdit() and .Save() - should be done as one transaction.
If .Save() throws an exception then the object should revert to its undoable state
as if .ApplyEdit() was not called also.

The one solution can be an implementation of .ApplyAndSave() method.
Rocky can do it for all of us if he will find it usefull.

The my solution is to clone an object, apply changes on it and try to save it:
          Project temp = _project.Clone();
          temp.ApplyEdit();
          _project = temp.Save(); // .Save() throws an exception on errors
See the full code below.

One another trouble in ProjectEdit.cs is in OKButton_Click() event handler:

    private void OKButton_Click(object sender, EventArgs e)
    {
      using (StatusBusy busy = new StatusBusy("Saving..."))
      {
        RebindUI(true, false);
      }
      this.Close();
    }

If the .Save() method will throw an exception in RebindUI() then the ProjectEdit
control will be closed immediately after closing an exception messagebox. And a user
will not be able to undo changes again.

The solution is to catch all exception inside the RebindUI() method and then return
boolean true if saving was successful or false otherwise.

For the Close button it isn't significant but I wrote this line of code also ;)

So here are changes in ProjectEdit.cs:

    private void OKButton_Click(object sender, EventArgs e)
    {
      using (StatusBusy busy = new StatusBusy("Saving..."))
      {
        if ( RebindUI( true, false ) )
          this.Close();
      }
    }

    private void ApplyButton_Click(object sender, EventArgs e)
    {
      using (StatusBusy busy = new StatusBusy("Saving..."))
      {
        RebindUI(true, true);
      }
    }

    private void Cancel_Button_Click(object sender, EventArgs e)
    {
      RebindUI(false, true);
    }

    private void CloseButton_Click(object sender, EventArgs e)
    {
      if ( RebindUI( false, false ) )
        this.Close();
    }

    // RebindUI() returns true if the requested operation (saving or canceling
    // an object) was completed successfully. And false if any exceptions
    // were thrown.
    private bool RebindUI( bool saveObject, bool rebind )
    {
      bool operationCompleted = false;

      // disable events
      this.projectBindingSource.RaiseListChangedEvents = false;
      this.resourcesBindingSource.RaiseListChangedEvents = false;

      // save or cancel changes
      string exceptionMsgTitle = saveObject ? "Error saving":"Error canceling";
      try
      {
        // commit edits in memory
        FlushBindingSource( this.resourcesBindingSource, saveObject );
        FlushBindingSource( this.projectBindingSource, saveObject );

        if ( saveObject )
        {
          Project temp = _project.Clone();
          temp.ApplyEdit();
          _project = temp.Save(); // .Save() throws an exception on errors
          operationCompleted = true;
        }
        else
        {
          _project.CancelEdit(); // can it throw an exception?
                                 // maybe a "not enough memory" one only.
                                 // (look into UndoableBase.UndoChanges())
          operationCompleted = true;
        }
      }
      catch ( Csla.DataPortalException ex )
      {
        MessageBox.Show( ex.BusinessException.ToString(),
          exceptionMsgTitle, MessageBoxButtons.OK,
          MessageBoxIcon.Exclamation );
      }
      catch ( Exception ex )
      {
        MessageBox.Show( ex.ToString(),
          exceptionMsgTitle, MessageBoxButtons.OK,
          MessageBoxIcon.Exclamation );
      }
      finally
      {
        // rebind UI if requested and .Save() ( or Cancel() ) were successful.
        // The second check (operationCompleted==true) is required because
        // if the .Save() was not successful, then _project must stay in its
        // edit state so a user can undo this object to the previous state
        // just pressing the Cancel button once.
        if ( rebind && operationCompleted)
          BindUI(); // calls BusinessBase.BeginEdit()

        // restore events
        this.projectBindingSource.RaiseListChangedEvents = true;
        this.resourcesBindingSource.RaiseListChangedEvents = true;

        if ( rebind || !operationCompleted )
        {
          // refresh the UI if BindUI() was called
          // or if the .Save() was not successful
          // (if an exception is thrown after Save button was pressed)

          this.projectBindingSource.ResetBindings( false );
          this.resourcesBindingSource.ResetBindings( false );
        }
     
      }
      return operationCompleted;
    }

The RebindUI() method calls WinPart.FlushBindingSource()
(which was introduced in http://forums.lhotka.net/forums/thread/20189.aspx )

WinPart.cs:

    protected void FlushBindingSource( BindingSource source, bool apply )
    {
      if ( apply )
        source.EndEdit();
      else
        source.CancelEdit();
    }

Also we should move binding code from the ProjectEdit constructor to the load event handler
(as noticed in http://forums.lhotka.net/forums/thread/20151.aspx ):

    public ProjectEdit(Project project)
    {
      InitializeComponent();

      // store object reference
      _project = project;

    }

    private void ProjectEdit_Load( object sender, EventArgs e )
    {
      // set up binding
      this.roleListBindingSource.DataSource = RoleList.GetList();

      BindUI();

      // check authorization
      ApplyAuthorizationRules();
    }

That's all. Nearly the same should be done with ResourceEdit.cs.

I think with these patches the ProjectTracker will be stable enough for its purpose
to be the face of .CSLA framework.

---
Regards,
Oleg

ajj3085 replied on Monday, January 07, 2008

RockfordLhotka replied on Monday, January 07, 2008

Exactly! And in 3.5 the data portal will default to cloning the object unless you use a config switch to set it to the pre-3.0 behavior (when you should have been manually cloning the object).

(which has a beneficial side-effect because it catches most remote data portal issues even with a local data portal - making moving from 2-tier to 3-tier simpler)

so24wo replied on Monday, January 07, 2008

2 Andy:

 What is the use if we clone the object after .ApplyEdit()? There will be two objects with two empty undo stacks and it will not be possible to undo any changes.

I'm talking that we should to clone the object BEFORE .ApplyEdit().

2 Rocky:

Currently I'm using CSLA 3.0.3 with CslaAutoCloneOnUpdate set to true. So the framework clones my objects automatically on every .Save(). But there is no code which clones the object in the beginning of ApplyEdit() in this version of CSLA.

What about CSLA 3.5? I'll check it also as soon as the svn becomes accessible. Is looks as it is shutdowned today :(

2 myself:

Anyway there is the bug in the finally block.

If we didn't change the projectBindingSource.DataSource then there are no needs to reset our binding sources.
The correct code is here:

      finally
      {
        if ( rebind && operationCompleted)
          BindUI();

        this.projectBindingSource.RaiseListChangedEvents = true;
        this.resourcesBindingSource.RaiseListChangedEvents = true;

        if ( rebind && operationCompleted)
        {
          // refresh the UI if BindUI() was called

          this.projectBindingSource.ResetBindings( false );
          this.resourcesBindingSource.ResetBindings( false );
        }
      }

---
Regards,
Oleg

ajj3085 replied on Monday, January 07, 2008

so24wo:

What is the use if we clone the object after .ApplyEdit()? There will be two objects with two empty undo stacks and it will not be possible to undo any changes.

I'm talking that we should to clone the object BEFORE .ApplyEdit().

I see what you're getting at; the user loses the ability to undo changes.  That only matters if for some reason your exception is correctable via user action, so as picking a new unique document number.  If you have a need for this, I suppose you can turn off auto-cloning done in the data portal and do the clone first.  I'm not sure if databinding would like it though if you re-bound the old object when the save fails.

so24wo replied on Monday, January 07, 2008

ajj3085:

I see what you're getting at; the user loses the ability to undo changes.  That only matters if for some reason your exception is correctable via user action, so as picking a new unique document number. 

Yes. One possibility when this happens is if the user saves an invalid object then ApplyEdit() doesn't throw an exception but Save() does. Of cource the user knows that the object is invalid already because the UI should shows it with the ErrorProvider control.

One another possibility is when there are new validation rules added on the server but the client application was not updated. In this case the DataPortal_Update() must check validation rules and if they are violated it must throw an exception. So Save() throws it on the client side. (Well, I wish to write one another message about the validation in the ProjectTracker's DataPortal methods.)

ajj3085:

If you have a need for this, I suppose you can turn off auto-cloning done in the data portal and do the clone first.

Why? I like it :)

ajj3085:

I'm not sure if databinding would like it though if you re-bound the old object when the save fails.

Currently I'm sure there is no any troubles with it. Of cource, if the object's DataPortal_Update() uses transactions.

---
Regards,
Oleg

ajj3085 replied on Monday, January 07, 2008

so24wo:

Yes. One possibility when this happens is if the user saves an invalid object then ApplyEdit() doesn't throw an exception but Save() does. Of cource the user knows that the object is invalid already because the UI should shows it with the ErrorProvider control.

so24wo:

One another possibility is when there are new validation rules added on the server but the client application was not updated. In this case the DataPortal_Update() must check validation rules and if they are violated it must throw an exception. So Save() throws it on the client side. (Well, I wish to write one another message about the validation in the ProjectTracker's DataPortal methods.)

That shouldn't happen, and won't work unless you take extra steps.  The normal scenario for this is remoting, and with remoting versions much match exactly for just this reason; the type may have changed between versions.  I would guess that such a setup isn't a supported scenario, unless you are using Csla to build two applications because you're crossing a trust boundry. 

so24wo:

ajj3085:

If you have a need for this, I suppose you can turn off auto-cloning done in the data portal and do the clone first.

Why? I like it :)

Well because you're doing the clone twice, needlessly.  If you're going to clone prior to ApplyEdit, there isn't a real need to clone again, and you're just incuring more overhead for no benefit.

so24wo:

ajj3085:

I'm not sure if databinding would like it though if you re-bound the old object when the save fails.

Currently I'm sure there is no any troubles with it. Of cource, if the object's DataPortal_Update() uses transactions.

so24wo replied on Tuesday, January 08, 2008

ajj3085:

Well because you're doing the clone twice, needlessly.  If you're going to clone prior to ApplyEdit, there isn't a real need to clone again, and you're just incuring more overhead for no benefit.

But we also use this autocloning in RolesEdit.cs.

So for ProjectEdit our code should look something like:

      try
      {
        ...
          Project temp = _project.Clone();
          temp.ApplyEdit();
          AutoCloneOnUpdate = false;
          _project = temp.Save(); // .Save() throws an exception on errors
          operationCompleted = true;
        ...
      }
      catches (...) { ... }
      finally
      {
          AutoCloneOnUpdate = true;
          ...
      }

Of cource this is only idea. As AutoCloneOnUpdate is a readonly property and it works for the LocalProxy only.

---
Regards,
Oleg

ajj3085 replied on Tuesday, January 08, 2008

RockfordLhotka replied on Tuesday, January 08, 2008

RockfordLhotka replied on Tuesday, January 08, 2008

RockfordLhotka replied on Monday, January 07, 2008