739236 - Add balrog 'rollback' link to Balrog admin-ui for each change that is made

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Erick Dransch [:edransch]]

The balrog ui should offer an 'undo' option for each change that is made via the ui.

Comment 1

[Erick Dransch [:edransch]]

Attachment: Partial Patch

This partial patch covers mainly server code.
The modification returns the change_id for each database operation, so that it can be returned to the client side code alongside any other data that is returned.
The client side code to handle the change_id and make the rollback calls is still missing.

Comment 2

[Peter Bengtsson [:peterbe]]

:bhearsum is it enough if we simply do a delete as Undo on insertions?
The RulesAPIView.post method returns the newly created rule_id. I could use that to do `delete from rules where rule_id=:rule_id` and `delete from rules_history where rule_id=:rule_id`. Simple! 
That way it's more like it never happened and no audit trail of the undo itself.

Comment 3

[[e-mail] (:bhearsum)]

(In reply to Peter Bengtsson [:peterbe] from comment #2)
> :bhearsum is it enough if we simply do a delete as Undo on insertions?
> The RulesAPIView.post method returns the newly created rule_id. I could use
> that to do `delete from rules where rule_id=:rule_id` and `delete from
> rules_history where rule_id=:rule_id`. Simple! 
> That way it's more like it never happened and no audit trail of the undo
> itself.

We talked about this on the phone. We need to maintain the audit trail, so calling Rules.delete() (which will remove from 'rules' and add a new entry to 'rules_history') is the right thing to do. Let me know if that's different than what you understood.

Comment 4

[Peter Bengtsson [:peterbe]]

(In reply to Ben Hearsum [:bhearsum] from comment #3)
> (In reply to Peter Bengtsson [:peterbe] from comment #2)
> > :bhearsum is it enough if we simply do a delete as Undo on insertions?
> > The RulesAPIView.post method returns the newly created rule_id. I could use
> > that to do `delete from rules where rule_id=:rule_id` and `delete from
> > rules_history where rule_id=:rule_id`. Simple! 
> > That way it's more like it never happened and no audit trail of the undo
> > itself.
> 
> We talked about this on the phone. We need to maintain the audit trail, so
> calling Rules.delete() (which will remove from 'rules' and add a new entry
> to 'rules_history') is the right thing to do. Let me know if that's
> different than what you understood.

Yes, that is different. What I understood it as was that each Rule as a history view (e.g. media wiki) and for each revision (obviously not the latest current one) you can click "roll back to this" which will basically *copy* that revision as to become the latest update. 
So, no delete.

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Peter Bengtsson [:peterbe] from comment #4)
> (In reply to Ben Hearsum [:bhearsum] from comment #3)
> > (In reply to Peter Bengtsson [:peterbe] from comment #2)
> > > :bhearsum is it enough if we simply do a delete as Undo on insertions?
> > > The RulesAPIView.post method returns the newly created rule_id. I could use
> > > that to do `delete from rules where rule_id=:rule_id` and `delete from
> > > rules_history where rule_id=:rule_id`. Simple! 
> > > That way it's more like it never happened and no audit trail of the undo
> > > itself.
> > 
> > We talked about this on the phone. We need to maintain the audit trail, so
> > calling Rules.delete() (which will remove from 'rules' and add a new entry
> > to 'rules_history') is the right thing to do. Let me know if that's
> > different than what you understood.
> 
> Yes, that is different. What I understood it as was that each Rule as a
> history view (e.g. media wiki) and for each revision (obviously not the
> latest current one) you can click "roll back to this" which will basically
> *copy* that revision as to become the latest update. 
> So, no delete.

Hmmm, I'm not 100% sure I understand. Here's a concrete example of what I would expect for an update:
rule 23 has 3 different versions (1, 2, and 3). We are at version 3 and want to "rollback" to version 1. To do this, Rules.update() will be called with the state of the row from version 1. That will create version *4* in the history table at the same time, whose content which will be the same as version 1.

Comment 6

[Peter Bengtsson [:peterbe]]

Then that's exactly what I had in mind too! Communication is silly!

Comment 7

[Peter Bengtsson [:peterbe]]

Work in progress: http://cl.ly/LbJI

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Peter Bengtsson [:peterbe] from comment #7)
> Work in progress: http://cl.ly/LbJI

Looks pretty good. Any idea how to deal with the horizontal overflow?

Is the bolding of the changed fields done by comparing it against the current version, or against the version directly after it? Eg, if distVersion changed in change #3, and then changed back in change #5, would it be bold?

Comment 9

[Peter Bengtsson [:peterbe]]

I'd prefer to deal with the annoying horizontal in a different (new?) bug.

Regarding the bold changing, it's comparing row by row. Not comparing it to the "head".

Comment 10

[[e-mail] (:bhearsum)]

(In reply to Peter Bengtsson [:peterbe] from comment #9)
> I'd prefer to deal with the annoying horizontal in a different (new?) bug.

WFM.

> Regarding the bold changing, it's comparing row by row. Not comparing it to
> the "head".

Sounds fine.

Comment 11

[Peter Bengtsson [:peterbe]]

PR here: https://github.com/mozilla/balrog/pull/8

Comment 12

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/a7a3c990ccd4b341dcf266b4e3193909dee7b5a4
bug 739236 - history and rollback view, r=bhearsum