938132 - need to be able to delete rules from admin ui

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (frontend), defect)

Description

[[e-mail] (:bhearsum)]

Right now there's no way to delete a rule through the UI. Looks like we probably need to support DELETE on the backend, too.

Comment 1

[[e-mail] (:bhearsum)]

I'm working on this.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: make rules deleteable

The frontend part of this got a bit nasty. Datatables prefers you to pass lists of values to it, but we return html blocks when loading a rule. Luckily there's a plugin that lets you add html hunks \o/. I think the frontend is already due for an overhaul :(. Eg, it would be better to return json responses, probably.

Comment 3

[Massimo Gervasini [:massimo]]

(In reply to Ben Hearsum [:bhearsum] from comment #2)
> Created attachment 8384658 [details] [diff] [review]
> make rules deleteable
> 
> The frontend part of this got a bit nasty. Datatables prefers you to pass
> lists of values to it, but we return html blocks when loading a rule.
> Luckily there's a plugin that lets you add html hunks \o/. I think the
> frontend is already due for an overhaul :(. Eg, it would be better to return
> json responses, probably.

Ben,

I have got this error trying to apply the patch to my fresh local clone (master branch):

error: auslib/admin/views/rules.py: patch does not apply
error: patch failed: auslib/test/admin/views/test_rules.py:48
error: auslib/test/admin/views/test_rules.py: patch does not apply

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Massimo Gervasini [:mgerva] from comment #3)
> (In reply to Ben Hearsum [:bhearsum] from comment #2)
> > Created attachment 8384658 [details] [diff] [review]
> > make rules deleteable
> > 
> > The frontend part of this got a bit nasty. Datatables prefers you to pass
> > lists of values to it, but we return html blocks when loading a rule.
> > Luckily there's a plugin that lets you add html hunks \o/. I think the
> > frontend is already due for an overhaul :(. Eg, it would be better to return
> > json responses, probably.
> 
> Ben,
> 
> I have got this error trying to apply the patch to my fresh local clone
> (master branch):
> 
> error: auslib/admin/views/rules.py: patch does not apply
> error: patch failed: auslib/test/admin/views/test_rules.py:48
> error: auslib/test/admin/views/test_rules.py: patch does not apply

Which rev are you at? https://github.com/mozilla/balrog/ is at 1747ff269760222ff723832818cb7b9fa899e405, and that's what my patch against....

Comment 5

[Massimo Gervasini [:massimo]]

(In reply to Ben Hearsum [:bhearsum] from comment #4)
> (In reply to Massimo Gervasini [:mgerva] from comment #3)
> > (In reply to Ben Hearsum [:bhearsum] from comment #2)
> > > Created attachment 8384658 [details] [diff] [review]
> > > make rules deleteable
> > > 
> > > The frontend part of this got a bit nasty. Datatables prefers you to pass
> > > lists of values to it, but we return html blocks when loading a rule.
> > > Luckily there's a plugin that lets you add html hunks \o/. I think the
> > > frontend is already due for an overhaul :(. Eg, it would be better to return
> > > json responses, probably.
> > 
> > Ben,
> > 
> > I have got this error trying to apply the patch to my fresh local clone
> > (master branch):
> > 
> > error: auslib/admin/views/rules.py: patch does not apply
> > error: patch failed: auslib/test/admin/views/test_rules.py:48
> > error: auslib/test/admin/views/test_rules.py: patch does not apply
> 
> Which rev are you at? https://github.com/mozilla/balrog/ is at
> 1747ff269760222ff723832818cb7b9fa899e405, and that's what my patch
> against....

wrong repository! I was trying to apply the patch against your repository!

Comment 6

[Massimo Gervasini [:massimo]]

Hi Ben,

I am testing the code locally. There's only a problem I have found: after deleting a rule, if you click on the "AUS3 Admin" link it shows the following message:

Recent changes
Loading recent changes ...

but there is nothing to show because prev_change is None. 

From logs:
auslib/admin/views/index.py line 146, in decorateChanges
    for x in prev_change
TypeError: 'NoneType' object is not iterable

Could this error be caused by my local data or do you have the same?

Another minor improvement idea: it would be nice to have a row fading out when you click on "delete" and see that something has changed in the table.
In my tests, I have deleted, by mistake, two rules instead of one. With my dataset, the last columns of the table look identical (mostly empty). My feeling was that nothing really changed after the first "delete" so I have clicked on "delete" a second time before realizing that was too late (even with the "Rule deleted!" pop up).

Comment 7

[[e-mail] (:bhearsum)]

(In reply to Massimo Gervasini [:mgerva] from comment #6)
> Hi Ben,
> 
> I am testing the code locally. There's only a problem I have found: after
> deleting a rule, if you click on the "AUS3 Admin" link it shows the
> following message:
> 
> Recent changes
> Loading recent changes ...
> 
> but there is nothing to show because prev_change is None. 
> 
> From logs:
> auslib/admin/views/index.py line 146, in decorateChanges
>     for x in prev_change
> TypeError: 'NoneType' object is not iterable
> 
> Could this error be caused by my local data or do you have the same?

This sounds like a separate bug -- where the index page is busted when there's no data. I filed bug 979294 for this. Thanks!

> Another minor improvement idea: it would be nice to have a row fading out
> when you click on "delete" and see that something has changed in the table.
> In my tests, I have deleted, by mistake, two rules instead of one. With my
> dataset, the last columns of the table look identical (mostly empty). My
> feeling was that nothing really changed after the first "delete" so I have
> clicked on "delete" a second time before realizing that was too late (even
> with the "Rule deleted!" pop up).

Unless you or Nick feel it's a blocker, I suggest we put this off until we rework this page. What do you think?

Comment 8

[Massimo Gervasini [:massimo]]

Comment on attachment 8384658 [details] [diff] [review]
make rules deleteable

For me, the animation on delete is not a blocker, I can take it as part of bug 929555

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8384658 [details] [diff] [review]
make rules deleteable

Review of attachment 8384658 [details] [diff] [review]:
-----------------------------------------------------------------

I'm mostly skipping over the jquery & templates, hoping that Massimo has that covered.

::: auslib/admin/views/rules.py
@@ +184,5 @@
> +        if not rule:
> +            return Response(status=404)
> +        # Bodies are ignored for DELETE requests, so we need to force WTForms
> +        # to look at the arguments instead.
> +        form = EditRuleForm(request.args)

What sort of info do we need to pass in the form here ? At first glance it seems like you have everything you need when rule_id is passed as an argument to the function.

@@ +195,5 @@
> +        if not form.validate():
> +            cef_event("Bad input", CEF_WARN, errors=form.errors)
> +            return Response(status=400, response=form.errors)
> +
> +        if not db.permissions.hasUrlPermission(changed_by, '/rules/:id', 'POST', urlOptions={'product': rule['product']}):

s/POST/DELETE/ ?

Comment 10

[[e-mail] (:bhearsum)]

Attachment: fix permission checking

(In reply to Nick Thomas [:nthomas] from comment #9)
> Comment on attachment 8384658 [details] [diff] [review]
> make rules deleteable
> 
> Review of attachment 8384658 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm mostly skipping over the jquery & templates, hoping that Massimo has
> that covered.
> 
> ::: auslib/admin/views/rules.py
> @@ +184,5 @@
> > +        if not rule:
> > +            return Response(status=404)
> > +        # Bodies are ignored for DELETE requests, so we need to force WTForms
> > +        # to look at the arguments instead.
> > +        form = EditRuleForm(request.args)
> 
> What sort of info do we need to pass in the form here ? At first glance it
> seems like you have everything you need when rule_id is passed as an
> argument to the function.

We get data_version from the form, but we also get the automagic CSRF token checking when we call .validate(). I'll add a comment to clear up the confusion.

> @@ +195,5 @@
> > +        if not form.validate():
> > +            cef_event("Bad input", CEF_WARN, errors=form.errors)
> > +            return Response(status=400, response=form.errors)
> > +
> > +        if not db.permissions.hasUrlPermission(changed_by, '/rules/:id', 'POST', urlOptions={'product': rule['product']}):
> 
> s/POST/DELETE/ ?

d'oh.

Comment 11

[[github robot]]

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

https://github.com/mozilla/balrog/commit/1ae7ace7e305ed02c4f4b535de4f5821ccc15f41
bug 938132: need to be able to delete rules from admin ui. r=nthomas

Comment 13

[[e-mail] (:bhearsum)]

In production now, and I used it to delete a rule that we temporarily added for bug 947965.