Closed
Bug 938132
Opened 11 years ago
Closed 11 years ago
need to be able to delete rules from admin ui
Categories
(Release Engineering Graveyard :: Applications: Balrog (frontend), defect)
Release Engineering Graveyard
Applications: Balrog (frontend)
x86_64
Linux
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bhearsum, Assigned: bhearsum)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
nthomas
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
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 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
(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•11 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....
Comment 5•11 years ago
|
||
(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•11 years ago
|
||
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•11 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 8•11 years ago
|
||
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 9•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
Attachment #8386783 -
Flags: review?(nthomas) → review+
Comment 11•11 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•11 years ago
|
Attachment #8386783 -
Flags: checked-in+
Assignee | ||
Comment 13•11 years ago
|
||
In production now, and I used it to delete a rule that we temporarily added for bug 947965.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Release Engineering → Release Engineering Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•