The default bug view has changed. See this FAQ.

need to be able to delete rules from admin ui

RESOLVED FIXED

Status

Release Engineering
Balrog: Frontend
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bhearsum, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
I'm working on this.
Assignee: nobody → bhearsum
(Assignee)

Comment 2

3 years ago
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.
Attachment #8384658 - Flags: review?(nthomas)
Attachment #8384658 - Flags: review?(mgervasini)
(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
(Assignee)

Comment 4

3 years ago
(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....
(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!
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).
(Assignee)

Comment 7

3 years ago
(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 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
Attachment #8384658 - Flags: review?(mgervasini) → review+
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/ ?
(Assignee)

Comment 10

3 years ago
Created attachment 8386783 [details] [diff] [review]
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.
Attachment #8384658 - Attachment is obsolete: true
Attachment #8384658 - Flags: review?(nthomas)
Attachment #8386783 - Flags: review?(nthomas)

Updated

3 years ago
Attachment #8386783 - Flags: review?(nthomas) → review+

Comment 11

3 years ago
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
(Assignee)

Updated

3 years ago
Attachment #8386783 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 823158
(Assignee)

Updated

3 years ago
Depends on: 983135
(Assignee)

Comment 13

3 years ago
In production now, and I used it to delete a rule that we temporarily added for bug 947965.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.