97739 - Deleting attachment statuses should give confirmation even when JS off.

Status: RESOLVED FIXED

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

Description

[Matthew Tuck [:CodeMachine]]

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.

Comment 1

[Gervase Markham [:gerv]]

CCing myk - this is probably his baby.

Gerv

Comment 2

[Gervase Markham [:gerv]]

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

Comment 3

[Matthew Tuck [:CodeMachine]]

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.

Comment 4

[Gervase Markham [:gerv]]

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

Comment 5

[Matthew Tuck [:CodeMachine]]

It will fall out of that work, but I don't think we should ship anything in this
state.

Comment 6

[Gervase Markham [:gerv]]

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

Comment 7

[Matthew Tuck [:CodeMachine]]

This wasn't in either of those releases.

Comment 8

[Dave Miller [:justdave]]

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)

Comment 9

[Gervase Markham [:gerv]]

Fair enough. 2.16 blocker it is.

Gerv

Comment 10

[Jeff Hedlund]

Attachment: Patch v.1

No more js!

Comment 11

[Jeff Hedlund]

Attachment: delete.atml

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?)

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 71666 [details]
delete.atml

>  # Contributor(s): Myk Melez <[e-mail]>

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

Comment 13

[Gervase Markham [:gerv]]

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

Comment 14

[Jeff Hedlund]

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 :)

Comment 15

[Jeff Hedlund]

>>+  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?

Comment 16

[Jeff Hedlund]

Attachment: Patch v.2

Everything except for the taint checking...

Comment 17

[Jeff Hedlund]

Attachment: delete.atml v.2

made necessary changes.

Comment 18

[Gervase Markham [:gerv]]

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

Comment 19

[Gervase Markham [:gerv]]

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

Comment 20

[Matthew Tuck [:CodeMachine]]

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?

Comment 21

[Jeff Hedlund]

> 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?

Comment 22

[Gervase Markham [:gerv]]

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

Comment 23

[Matthew Tuck [:CodeMachine]]

Gerv, this has nothing to do with double confirmation.  The JS confirmation
bypasses the CGI confirmation.

Comment 24

[Gervase Markham [:gerv]]

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

Comment 25

[Matthew Tuck [:CodeMachine]]

The benefit of the JS method is that it is faster, there is one less page load
required.

Comment 26

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 27

[Jeff Hedlund]

Attachment: patch v.3

Fixed up stuff

(JS still not in)

Comment 28

[Jeff Hedlund]

Attachment: delete.atml v.3

made requested changes

Comment 29

[Jeff Hedlund]

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?

Comment 30

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 31

[Jeff Hedlund]

Attachment: patch v.4

JS back in.

Comment 32

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 33

[Gervase Markham [:gerv]]

(Looks OK to me otherwise.)

Gerv

Comment 34

[Jeff Hedlund]

Attachment: patch v.5

Made the requested changes.

Almost there... I can *feel* it! :)

Comment 35

[Gervase Markham [:gerv]]

Comment on attachment 73222 [details] [diff] [review]
patch v.5

r=gerv.

Gerv

Comment 36

[Myk Melez [:myk] [@mykmelez]]

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).

Comment 37

[Myk Melez [:myk] [@mykmelez]]

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

Comment 38

[Gervase Markham [:gerv]]

>+            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

Comment 39

[Jeff Hedlund]

Attachment: patch v.6

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 40

[Gervase Markham [:gerv]]

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

Comment 41

[Myk Melez [:myk] [@mykmelez]]

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

Comment 42

[Myk Melez [:myk] [@mykmelez]]

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