1333095 - reset signoffs when a scheduled change is updated

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), defect, P1)

Description

[[e-mail] (:bhearsum)]

When a person signs off on a Scheduled Change, they do so having viewed it in its current form. If that Scheduled Change changes in any way, any Signoffs done must become invalid - otherwise we could have something gain full sign, then get changed before being enacted -- which effectively means things without signoff could go live.

Comment 1

[A.S.]

The only way to edit/update a scheduled change is POST a request on this URI /scheduled_changes/<table>/<sc_id> , Now I'm considering two scenarios : 

1. request body parameters are all same values as the ones already persisting in DB, shouldn't reset signoffs as nothing has changed.

2. Changes in values for some parameters, hence reset signoffs.

Considering an example, here's the schema for releases_scheduled_changes and releases_scheduled_changes_signoffs. http://imgur.com/a/tKV8t
I got slightly confused on my first look at the code,  but if I understood this correctly, resetting signoffs means removing all sc_ids that match in releases_scheduled_changes_signoffs for a particular scheduled change ?

Comment 2

[[e-mail] (:bhearsum)]

(In reply to Ashish Sareen from comment #1)
> I got slightly confused on my first look at the code,  but if I understood
> this correctly, resetting signoffs means removing all sc_ids that match in
> releases_scheduled_changes_signoffs for a particular scheduled change ?

That's exactly right.

> The only way to edit/update a scheduled change is POST a request on this URI
> /scheduled_changes/<table>/<sc_id> , Now I'm considering two scenarios : 
> 
> 1. request body parameters are all same values as the ones already
> persisting in DB, shouldn't reset signoffs as nothing has changed.
> 
> 2. Changes in values for some parameters, hence reset signoffs.
> 
> Considering an example, here's the schema for releases_scheduled_changes and
> releases_scheduled_changes_signoffs. http://imgur.com/a/tKV8t

Resetting signoffs shouldn't be something that the UI is responsible for. Consider that things other than the UI could update Scheduled Changes (by using the API directly) - we need to make sure that signoffs get reset regardless of how they get updated.

I think the more likely place for this logic to live is in ScheduledChangesTable.update (https://github.com/mozilla/balrog/blob/897c39144caafaa41c53bfdd95c70f71e66b4b03/auslib/db.py#L1084) -- it should have all the necessary information to do the revocations.

Comment 3

[A.S.]

Update: Adding line '''self.sc_table.signoffs.delete(where={"sc_id": sc_id},changed_by=changed_by, transaction=transaction)''' at (https://github.com/mozilla/balrog/blob/master/auslib/admin/views/scheduled_changes.py#L98) should delete all the signoffs present for the given sc_id for the particular *_scheduled_changes table. 

However, the current AUSTable delete method inherited and used by SignOffs tables expects only 1 row to be matched and deleted as a criteria for success. So the method _prepareForDelete defined at (https://github.com/mozilla/balrog/blob/master/auslib/db.py#L428) throws an error when multiple rows or no row is returned. So two approaches: 

1. I can write a private method to be invoked at (https://github.com/mozilla/balrog/blob/master/auslib/admin/views/scheduled_changes.py#L98) to reset signoffs.
OR
2. Modify the delete method inherted fro AUSTable in Signoffs class(This will require some analysis and discussions with the stakeholders).

What should be done ?

Comment 4

[A.S.]

For posterity , here's the error log generated after adding the delete method statementfor signoffs:
http://pastebin.com/mTzGs6HG

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Ashish Sareen from comment #3)
> Update: Adding line '''self.sc_table.signoffs.delete(where={"sc_id":
> sc_id},changed_by=changed_by, transaction=transaction)''' at
> (https://github.com/mozilla/balrog/blob/master/auslib/admin/views/
> scheduled_changes.py#L98) should delete all the signoffs present for the
> given sc_id for the particular *_scheduled_changes table. 
> 
> However, the current AUSTable delete method inherited and used by SignOffs
> tables expects only 1 row to be matched and deleted as a criteria for
> success. So the method _prepareForDelete defined at
> (https://github.com/mozilla/balrog/blob/master/auslib/db.py#L428) throws an
> error when multiple rows or no row is returned. So two approaches: 

Ack. I forgot that AUSTable.delete enforced this. I don't think it's a good idea to try to change that as part of this bug...

> 1. I can write a private method to be invoked at
> (https://github.com/mozilla/balrog/blob/master/auslib/admin/views/
> scheduled_changes.py#L98) to reset signoffs.
>
> 2. Modify the delete method inherted fro AUSTable in Signoffs class(This
> will require some analysis and discussions with the stakeholders).

Currently, the only things that actually modify the database (that is, call the real sqlalchemy update/insert/delete methods) are in AUSTable. This is convenient, because it lets us reason about certain types of problems more easily, so I don't think creating a private method in another class would be a good idea.

As an alternative, could you select from the signoffs table, and then delete each row by using sc_id+username in the where clause?

Comment 6

[A.S.]

(In reply to Ben Hearsum (:bhearsum) from comment #5)
> (In reply to Ashish Sareen from comment #3)
> > Update: Adding line '''self.sc_table.signoffs.delete(where={"sc_id":
> > sc_id},changed_by=changed_by, transaction=transaction)''' at
> > (https://github.com/mozilla/balrog/blob/master/auslib/admin/views/
> > scheduled_changes.py#L98) should delete all the signoffs present for the
> > given sc_id for the particular *_scheduled_changes table. 
> > 
> > However, the current AUSTable delete method inherited and used by SignOffs
> > tables expects only 1 row to be matched and deleted as a criteria for
> > success. So the method _prepareForDelete defined at
> > (https://github.com/mozilla/balrog/blob/master/auslib/db.py#L428) throws an
> > error when multiple rows or no row is returned. So two approaches: 
> 
> Ack. I forgot that AUSTable.delete enforced this. I don't think it's a good
> idea to try to change that as part of this bug...
> 
> > 1. I can write a private method to be invoked at
> > (https://github.com/mozilla/balrog/blob/master/auslib/admin/views/
> > scheduled_changes.py#L98) to reset signoffs.
> >
> > 2. Modify the delete method inherted fro AUSTable in Signoffs class(This
> > will require some analysis and discussions with the stakeholders).
> 
> Currently, the only things that actually modify the database (that is, call
> the real sqlalchemy update/insert/delete methods) are in AUSTable. This is
> convenient, because it lets us reason about certain types of problems more
> easily, so I don't think creating a private method in another class would be
> a good idea.
Ah I see. Good thing I consulted first.

> 
> As an alternative, could you select from the signoffs table, and then delete
> each row by using sc_id+username in the where clause?
A little redundant but easily doable as an alternative for now.

Comment 7

[[github robot]]

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

https://github.com/mozilla/balrog/commit/c1ed45406f082351ae273b317eb47d19bb14b1c8
Bug 1333095 - reset signoffs when a scheduled change is updated (#229). r=bhearsum

Comment 8

[[e-mail] (:bhearsum)]

In production, thank you Ashish!