1333875 - don't update "scheduled_by" field when merging in update of base table

Status: RESOLVED FIXED

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

Description

[[e-mail] (:bhearsum)]

While debugging bug 1333874 we discovered that the "scheduled by" field of a Scheduled Change gets updated when mergeUpdate runs. This means that if Mihai schedules a change, and then Jordan updates the Rule directly - Jordan will end up being listed as the scheduler of the change, which ultimately means he will end up in the "changed by" field when the Scheduled Change is enacted.

This is wrong. We should only update scheduled_by when someone is directly updating a scheduled change.

Comment 1

[Jaehoon Hwang]

Can I work on this bug? I'm new to contributing to an open source project.
Any guide you can give me would be appreciated.

Comment 2

[[e-mail] (:bhearsum)]

(In reply to Jaehoon Hwang from comment #1)
> Can I work on this bug? I'm new to contributing to an open source project.
> Any guide you can give me would be appreciated.

Hello, thanks for your interest! To get started, I would recommend ensuring that you're able to run the Balrog development environment and tests (instructions at https://github.com/mozilla/balrog/blob/master/README.rst).

Once you've got that working, try to reproduce this bug. It'll be a bit tricky because we need to add a second user. Try the following:
1) Go to http://localhost:8080/permissions and create a new user called "jaehoon" with "admin" permissions.
2) Schedule a Change to a Rule. Go to http://localhost:8080/rules, click on one of the "Schedule an Update" buttons, choose a time that's a few days in the future, and then click "Save Changes".
3) Go to http://localhost:8080/rules/scheduled_changes, and observe that "Scheduled By" is set to "balrogadmin"
4) Shut down the Docker containers, edit uwsgi/admin.dev.ini (change "balrogadmin" to "jaehoon"), and start the containers up again.
5) Go back to http://localhost:8080/rules and make a change to the same Rule that you scheduled a change for, eg: change the Product.
6) Finally, go back to http://localhost:8080/rules/scheduled_changes. You should see that "Scheduled By" is set to "jaehoon" - which is the problem! In a case like this, we want it to remain Scheduled By "balrogadmin".

The fix for this will most likely need to be in mergeUpdate (https://github.com/mozilla/balrog/blob/897c39144caafaa41c53bfdd95c70f71e66b4b03/auslib/db.py#L1192), and you'll want to write a test or two, similar the ones at https://github.com/mozilla/balrog/blob/897c39144caafaa41c53bfdd95c70f71e66b4b03/auslib/test/test_db.py#L1312.

I know this a lot to take in! Don't hesitate to ask questions here or in irc://irc.mozilla.org/#balrog if anything is confusing or you get stuck.

Comment 3

[Jaehoon Hwang]

Hi, I just wanted to give you an update.

I was successful in recreating the bug.

But I do have a question on step 5.

When you say change, do you mean "Update" button in http://localhost:8080/rules/scheduled_changes ?

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Jaehoon Hwang from comment #3)
> Hi, I just wanted to give you an update.
> 
> I was successful in recreating the bug.
> 
> But I do have a question on step 5.
> 
> When you say change, do you mean "Update" button in
> http://localhost:8080/rules/scheduled_changes ?

Actually, I mean the "Update" button on http://localhost:8080/rules that's associated with the Rule you created the Scheduled Changed for. Doing that will trigger the mergeUpdate function and reproduce the bug.

Comment 5

[Jaehoon Hwang]

I was able to produce the bug and I'll work on it.
Thank you for the help.

Comment 6

[Jaehoon Hwang]

I believe I have got the solution for the bug and now I am working on test cases.

Comment 7

[Jaehoon Hwang]

Created a pull request #238

Comment 8

[[github robot]]

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

https://github.com/mozilla/balrog/commit/3360d73ae2f82d528b09c4634098d14b383dba4b
Bug 1333875 - don't update "scheduled_by" field when merging in update of base table (#238). r=bhearsum,aksareen

Comment 9

[[e-mail] (:bhearsum)]

This in production now. Nice work Jaehoon!

Comment 10

[Jaehoon Hwang]

Thank you. This is a great/scary feeling.