Rewrite the comment dialog using a model/view split, with unit tests.

Review Request #3704 — Created Dec. 27, 2012 and submitted

Information

Review Board
release-1.7.x

Reviewers

Rewrite the comment dialog using a model/view split, with unit tests.

This splits up the old commentDlg singleton and replaces it with a new
CommentEditor model and CommentDialogView view.

CommentEditor keeps track of the state of a pending comment edit. This
incudes what actions (save, delete, cancel) are allowed, the text of the
comment, the actual Comment backing it, opened issue state, and the
dirty state. It also provides the actions you can make, such as saving
the contents of the editor back to the comment and to the server, or
deleting it.

CommentDialogView replaces the old commentDlg view code, but leaves all
the state handling and operations up to the associated CommentEditor. It
also splits out the list of existing published comments into a separate
sub-view.

Unit tests are included to test the majority of this new functionality.
It should help catch any major regressions going forward.

There are improvements I want to make to this as we develop our
architecture. One is to rewrite how the existing published comments list
works. Right now, it's very unclean, since it's based on how Comments
currently work and how we handle serialization/deserialization. Later,
it should get a real backing and manage its own state better.

It also doesn't prevent editing on draft review requests, but that's not
a regression, and will be covered either in a revision to this, or in
another change.
Tested various scenarios with diff, screenshot and file attachment
comments. This included simulating that I couldn't edit comments
(currently only due to not being logged in), creating new comments,
editing draft comments, and creating comments in regions with
existing published comments.

Created a bunch of new unit tests to test various state (which helped
catch a bunch of bugs while writing this). They all pass in Chrome
and Firefox.
JA
david
  1. Nothing jumps out at me, but I think this will require a lot of testing. What branch did you want to push this to?
    1. master is fine for now. We'll keep release-1.7.x in a stable state from here on out. I would like to try to get this stuff into 1.7.x releases sooner than later, though.
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (101a39c)
Loading...