Closed Bug 97739 Opened 23 years ago Closed 23 years ago

Deleting attachment statuses should give confirmation even when JS off.

Categories

(Bugzilla :: Attachments & Requests, defect, P1)

2.15
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: CodeMachine, Assigned: justdave)

Details

Attachments

(3 files, 6 obsolete files)

Apparently there's currently a confirmation that appears when you try and delete
an attachment status that's in use.  However, it is written using JS and only
appears if you have it on.

We should augment this with non-JS code.
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
CCing myk - this is probably his baby.

Gerv
Severity: major → blocker
Actually, it appears for all deletes. Given that keyword deletion has
unconditional confirmation, attachstatus deletion probably should, too. And they
can share a template. This should fall out of the 2.18 admin CGI work.

Gerv
Severity: blocker → normal
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
editkeywords.cgi does not have unconditional deletion nor does any other CGI I'm
aware of.  I don't see how we can justify leaving this off of 2.16.
Sorry, I meant:

Actually, it appears for all deletes. Given that keyword deletion has
_un_conditional confirmation, attachstatus deletion probably should, too. And they
can share a template. This should fall out of the 2.18 admin CGI work.

My justification for leaving this is that there's an obvious sensible time to do
 it, it's a non-trivial amount of work, and it's not as if there's _no_
confirmation. We can release note it, if you like.

Gerv
It will fall out of that work, but I don't think we should ship anything in this
state.
We shipped 2.14 and 2.14.1 in "this state", didn't we? Or was attachment tracker
after that?

Anyway, it's hardly a "state". Time for another justdave ruling?

Gerv
This wasn't in either of those releases.
Attachment tracker is new code as far as releases are concerned.  We haven't had
an official "release" that contained the attachment tracker yet.  As such, I
think it's broken that you don't get a warning about deleting data if you don't
have Javascript active.

Fixing this "the right way" for all of the mentioned cases here is probably a
non-trivial amount of work, as stated, however, all we need here to satisfy this
bug is a confirmation.  There can be a hidden form value for "confirmed" that is
off by default, and the Javascript can turn it on before submitting the form if
the user okays it in the Javascript dialog (thus avoiding a repeat from the CGI
version)
Fair enough. 2.16 blocker it is.

Gerv
Severity: normal → blocker
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
Attached patch Patch v.1 (obsolete) — Splinter Review
No more js!
Attached file delete.atml (obsolete) —
template/default/attachstatus/delete.atml new template file for confirming a
delete

as soon as editkeywords is templatized, it could share this template (and move
to a more appropriate shared template directory?)
Attachment #71666 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 71666 [details]
delete.atml

>  # Contributor(s): Myk Melez <myk@mozilla.org>

Feel free to add yourself here.

>  #%]
>
>[% INCLUDE global/header 
>  title = "Delete Attachment Status"
>  style = "
>    th { text-align: right; vertical-align: top; }
>    td { text-align: left; vertical-align: top; }
>  "
>%]
>
><h2>Confirmation</h2>

Please use the standard Bugzilla layout for titling pages. Set an h1 or h2
parameter to global/header.

>Warning!  [% attachcount %] attachments use this status!

"If you delete it, they will [explode | disappear | whatever]".

><p>
>Do you really want to delete this status?
><p>
><form method="post" action="editattachstatuses.cgi">
>  <input type="hidden" name="action" value="delete">
>  <input type="hidden" name="id" value="[% id %]">
>  <input type="submit" value="Yes, delete">
></form>

You should probably make this an:
<a href="editattachstatuses.cgi?action=delete&id=[% id %]">Yes, delete</a>
link. Then it can match
the "No, don't delete" link which you will also provide - probably taking you
back to editattachstatuses.cgi.

Gerv
Attachment #71666 - Flags: review-
Comment on attachment 71665 [details] [diff] [review]
Patch v.1

>-elsif ($action eq "delete") 
>+elsif ($action eq "del") 

>-  deleteStatus(); 
>+  delStatus();

"del" and "delete" are not sensible names for these actions and functions :-)
"confirmdelete" and "delete" might be.

>+sub delStatus {
>+  #check if we need confirmation to delete:

Nit - one space after #.

>+  SendSQL("SELECT COUNT(attach_id) FROM attachstatuses
>+           WHERE statusid = $::FORM{'id'}");

<cough> Did this pass taint checks? I'm pretty sure this won't work.

>+  my $attaches = FetchSQLData();

$attachcount.

Otherwise, looking good.

Gerv
Attachment #71665 - Flags: review-
Gerv, I will make the changes. 

The reason I used "del" and "delete", the title of the page, and a button to
confirm delete- I was mimicking editproducts.cgi (though that will change when
templitization is done).  I agree with your points, and noticed them before, but
I was trying to be consistent with the exisiting code 

Not a very good excuse, I realize :)
>>+  SendSQL("SELECT COUNT(attach_id) FROM attachstatuses
>>+           WHERE statusid = $::FORM{'id'}");

> <cough> Did this pass taint checks? I'm pretty sure this won't work.

I don't follow this one..  What are the taint checks?  Do I detaint_natural()
the id?
Attached patch Patch v.2 (obsolete) — Splinter Review
Everything except for the taint checking...
Attachment #71665 - Attachment is obsolete: true
Attached file delete.atml v.2 (obsolete) —
made necessary changes.
Attachment #71666 - Attachment is obsolete: true
Comment on attachment 71773 [details] [diff] [review]
Patch v.2

>+  } else {

Nit: uncuddled elses.

}
else {

> sub deleteStatus
> {
>+
>   # Delete an attachment status flag from the database.

Don't add this space :-)

Fix that and r=gerv.

Gerv
Attachment #71773 - Flags: review+
Comment on attachment 71774 [details]
delete.atml v.2

>[% INCLUDE global/header 
>  title = "Confirm Delete Attachment Status"
>%]

Confirm Delete of Attachment Status [% status %]?

>
><p>
>Warning!  [% attachcount %] attachments use this status.  If you delete the status, those attachments will lose this status.

How about:
"[% attachcount %] attachments have the status '[% status %]'.	If you delete
it, those attachments will lose this status."

><p>
>Do you really want to delete this status?
><p>
><a href="editattachstatuses.cgi?action=delete&id=[% id %]">Yes, delete</a>
><p>
><a href="editattachstatuses.cgi">No, don't delete</a>

Aligned horizontally rather than vertically? This also isn't valid HTML - a
load of </p> tags are missing.

Also, if you could follow current indenting practice (see any other template)
that would be good.

Gerv
Attachment #71774 - Flags: review-
The entire attachstatus directory will likely be revamped and moved to admin as
a part of bug #97706 and there is already a confirmdelete template I'm going to
use on CUST_RES_BRANCH at template/default/admin/common/confirmdelete.atml,
although it may be more generalised than we want here.

Why are we removing the JS confirmation?  What's wrong with both?



> Why are we removing the JS confirmation?  What's wrong with both?

The reason I took it JS out completely was to get this bug working for the 2.16
release.  Reading over this bug's comments, it sounds like things will change
for this in the 2.18 release anyway, so I was avoiding work that might be
overwritten in the "short" term anyway.

But if it's necessary to have both for 2.16, I can add it back in somehow... so
we would want to use JS if the client supports it, or go to the confirm delete
page otherwise?
Users will not want to confirm twice. So, we have two methods - one which works
some of the time, and one which works (or can work) all of the time. In this
case, it makes sense to just have the second.

Gerv
Gerv, this has nothing to do with double confirmation.  The JS confirmation
bypasses the CGI confirmation.
Yes, I understand that. I'm not saying that there _will_ be double confirmation. 

1) We have two possible confirmation methods - JS and template
2) The JS method works only some of the time (JS on)
3) The template method works, or can be made to work, all of the time
4) It is good to only have one bit of code to do a particular thing
5) Given 2), 3) and 4), we should turn on the template method all the time and 
remove the JS method.

Gerv
The benefit of the JS method is that it is faster, there is one less page load
required.
The JS version of this feature should not be removed for the sake of having a
single implementation.  Making Bugzilla work better is more important than
having a single implementation (as is maintaining compatibility with
non-JS/JS-off browsers) unless dual implementations would be exceptionally
burdensome, which is clearly not the case here.

Implementing the JS feature on top of the CGI work here is as simple as
redirecting the user to a different URL if they click the link and answer "OK"
to the JavaScript confirmation dialog.
Attached patch patch v.3 (obsolete) — Splinter Review
Fixed up stuff

(JS still not in)
Attachment #71773 - Attachment is obsolete: true
Attached file delete.atml v.3
made requested changes
Attachment #71774 - Attachment is obsolete: true
Reason I have not included JS yet:  I'm not sure of a clean way to do it.  The
way it did the check before was a simple onclick confirm() no matter if there
were statuses that used the attachement or not.

The template stuff gives more information:  How many times the status is used,
the name of the status being deleted, etc. 

I agree that the JS is much quicker and nice for those who have it, but wouldn't
it be nice to give the same information as the template?
The solution is to add a count of attachments using the status to the page
displaying attachment statuses, i.e.:

SELECT id, name, description, sortkey, product, count(statusid) 
FROM attachstatusdefs, attachstatuses 
WHERE attachstatusdefs.id=attachstatuses.statusid 
GROUP BY id;

Then, if you want the JavaScript alert to contain this information, pass it as
parameters to the confirmDelete function.

Keywords: patch, review
Attached patch patch v.4 (obsolete) — Splinter Review
JS back in.
Attachment #72537 - Attachment is obsolete: true
Comment on attachment 72914 [details] [diff] [review]
patch v.4

>Index: editattachstatuses.cgi

>@@ -174,14 +179,17 @@
> 
>   # Retrieve a list of attachment status flags and create an array of hashes
>   # in which each hash contains the data for one flag.
>-  SendSQL("SELECT id, name, description, sortkey, product 
>-           FROM attachstatusdefs ORDER BY sortkey");
>+  SendSQL("SELECT id, name, description, sortkey, product, count(statusid)
>+           FROM attachstatusdefs LEFT JOIN attachstatuses 
>+                ON attachstatusdefs.id=attachstatuses.statusid
>+           GROUP BY sortkey");

This query should grouped by id rather than sortkey (which fails to show
all attachment statuses).


>Index: template/default/attachstatus/list.atml

>+        <a href="editattachstatuses.cgi?action=edit&id=[% statusdef.id %]">
>+            Edit
>+        </a>
>+        <a href="editattachstatuses.cgi?action=confirmdelete&id=[% statusdef.id %]" 
>+         onclick="return confirmDelete([% statusdef.attachcount %],
>+                                       '[% statusdef.name %]',
>+                                       [% statusdef.id %]);">
>+            Delete
>+        </a>

Filter statusdef.name through the JS filter to escape any quotation marks
in it ([% statusdef.name FILTER js %]).  Also, these two links run into
each other when displayed so that they look like a single link.  They need
to be separated in a way that makes it obvious they are two separate links.
Attachment #72914 - Flags: review-
(Looks OK to me otherwise.)

Gerv
Attached patch patch v.5Splinter Review
Made the requested changes.

Almost there... I can *feel* it! :)
Attachment #72914 - Attachment is obsolete: true
Comment on attachment 73222 [details] [diff] [review]
patch v.5

r=gerv.

Gerv
Attachment #73222 - Flags: review+
Comment on attachment 73222 [details] [diff] [review]
patch v.5

>Index: template/default/attachstatus/list.atml

>@@ -44,8 +44,16 @@

>+        <a href="editattachstatuses.cgi?action=edit&id=[% statusdef.id %]">
>+            Edit
>+        </a>
>+        &nbsp;|&nbsp;
>+        <a href="editattachstatuses.cgi?action=confirmdelete&id=[% statusdef.id %]" 
>+         onclick="return confirmDelete([% statusdef.attachcount %],
>+                                       '[% statusdef.name FILTER js %]',
>+                                       [% statusdef.id %]);">
>+            Delete
>+        </a>

All good except for the remaining cosmetic issue that the Edit and Delete links 
have an extra space appended to them in some browsers (Netscape 4.x, Mozilla).
Attachment #73222 - Flags: review-
Comment on attachment 73222 [details] [diff] [review]
patch v.5

Changing my mind.  Minor cosmetic issues shouldn't prevent a patch like this
from going in; they should just be fixed in a separate patch afterwards.  r=myk
Attachment #73222 - Flags: review- → review+
>+            Delete
>+        </a>

Before you check this in, just turn those two lines into:

>+            Delete</a>

It's that easy :-)

(BTW, this "feature" of NS 4.x annoys me.)

Gerv

Attached patch patch v.6Splinter Review
Only difference between 5 and 6 is the </a> tag on the same line as the link
text to clean up the extra space in some browsers.
Comment on attachment 73684 [details] [diff] [review]
patch v.6

If that's true, then please check it in :-) Or, if you can't, I'll do it this
evening (hopefully) unless someone beats me to it.

Gerv
Attachment #73684 - Flags: review+
Checking in editattachstatuses.cgi;
/cvsroot/mozilla/webtools/bugzilla/editattachstatuses.cgi,v  <-- 
editattachstatuses.cgi
new revision: 1.4; previous revision: 1.3
done
Checking in template/default/attachstatus/list.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/attachstatus/list.atml,v 
<--  list.atml
new revision: 1.3; previous revision: 1.2
done
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/attachstatus/delete.atml,v
done
Checking in template/default/attachstatus/delete.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/attachstatus/delete.atml,v 
<--  delete.atml
initial revision: 1.1
done
Component: Administration → attachment and request management
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: