422691 - Attachment gets added twice after hitting "Back" and "Refresh"

Status: RESOLVED FIXED

Description

[wescottnimbus7]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: 

1.Submitted a bug report.
2. Went back into the bug and clicked on the "Add Attachment" button.
3. Added the attachment and gave it a description. Then I added some optional
comments on the last field below.
4. clicked the Submit button.
5. Then I clicked on the "Details" link next to the attachment.
6. Then I clicked the Browser's "Back" button. (I receive an error at this point).
7. Then I click on the Browser's "Refresh" button.
Results: The attachment was added again.

Reproducible: Always

Steps to Reproduce:
1.Submitted a bug report.
2. Went back into the bug and clicked on the "Add Attachment" button.
3. Added the attachment and gave it a description. Then I added some optional
comments on the last field below.
4. clicked the Submit button.
5. Then I clicked on the "Details" link next to the attachment.
6. Then I clicked the Browser's "Back" button. (I receive an error at this point).
7. Then I click on the Browser's "Refresh" button.
Results: The attachment was added again.
Actual Results:  
The attachment gets added twice

Expected Results:  
When I hit the back button it should return me to the bug report without another attachment being added. Any Refresh action should not cause the attachment to be re-added to the bug report.

Comment 1

[Frédéric Buclin]

This is because adding an attachment immediately brings you to show_bug.cgi. And if you click "Details" or view the attachment and then want to come back into the bug, attachment.cgi is called again to resubmit the attachment, which is a problem. Maybe do we need an intermediate page which immediately redirects the user to show_bug.cgi to prevent this.

Comment 2

[David Lawrence [:dkl]]

Created attachment 325637 [details] [diff] [review]
Patch to cancel duplicate attachments when reloading form (v1)

Created a patch to throw an intermediate message screen when someone hits refresh/reload after previous submitting an attachment. The screen will allow them to go to the page for entering a new attachment, to the previously submitted attachment, or back to the bug itself.

I basically took parts of the same code that protects against duplicate bug form submissions and reworked it for attachments.

Please take a look and review.
Thanks
Dave

Comment 3

[Max Kanat-Alexander]

Comment on attachment 325637 [details] [diff] [review]
Patch to cancel duplicate attachments when reloading form (v1)

Attachment stuff is LpSolit's.

Comment 4

[Frédéric Buclin]

Simply clicking the Back button doesn't resubmit the attachment (I thought it did), so that's not a big enough issue to be considered as a blocker.

Comment 5

[Frédéric Buclin]

Comment on attachment 325637 [details] [diff] [review]
Patch to cancel duplicate attachments when reloading form (v1)

This looks good to me. Just a few things to fix.


>Index: attachment.cgi

>+    # Detect if the user already used the same form to submit a bug

s/bug/attachment/.


>+        unless ($creator_id 
>+            && ($creator_id == $user->id) 
>+                && ($old_attach_id =~ "^createattachment:")) {

Nit: put { on its own line as the test is several lines long.



>Index: template/en/default/attachment/cancel-create-dupe.html.tmpl

>+  # bugid: integer. ID of the bug previously used to create a bug.

This must be updated. We are talking about attachments now. Also, you forgot to describe attachid.


>+  # allow_override: boolean int. Is 1 if the user may submit the bug again.

This parameter is no longer relevant for attachments.


>+        You already used the form to file attachment 
>+        <a href="[% urlbase FILTER html %]attachment.cgi?id=[% attachid FILTER url_quote %]&action=edit">[% attachid FILTER url_quote %]</a>.

Nit: enclose "attachment" in the link (as we do for bugs). This makes the link longer and easier to click.


>+  If you would like to submit a new attachment, click here to 
>+  <a href="[% urlbase FILTER html %]attachment.cgi?bugid=[% bugid FILTER url_quote %]&action=enter">add a new attachment</a>.
>+<p>
>+<p>
>+  Return to <a href="[% urlbase FILTER html %]show_bug.cgi?id=[% bugid FILTER url_quote %]">[% terms.bug %]&nbsp;[% bugid FILTER html %]</a>.

Nit: couldn't we mix both sentences in a single one, without repeating "add a new attachment"? For instance, you can either <a>add a new attachment</a> or <a>go back to bug XXX</a>.

Comment 6

[David Lawrence [:dkl]]

Created attachment 326948 [details] [diff] [review]
Patch to cancel duplicate attachments when reloading form (v2)

Thanks for the review. Here is an updated patch with your suggested changes.

Please review.
Thanks
Dave

Comment 7

[Frédéric Buclin]

Comment on attachment 326948 [details] [diff] [review]
Patch to cancel duplicate attachments when reloading form (v2)

>Index: template/en/default/attachment/cancel-create-dupe.html.tmpl

>+  # Contributor(s): Olav Vitters <[e-mail]>

Nit: you should add your name to the list.

>+  create a new attachment</a> or <a href="[% urlbase FILTER html %]show_bug.cgi?id=[% bugid FILTER url_quote %]">
>+  go back to [% terms.bug %]&nbps;[% bugid FILTER html %]</a>.

Two things:

1) Remove &nbps;, first because there is a typo (should be &nbsp;), and secondly because it's useless. Write [%+ bugid FILTER html %] to force a whitespace before the bug ID.

2) Instead of 1), you could write [% "go back to $terms.bug $bugid" FILTER bug_link($bugid) FILTER none %] instead of the whole URL.


My preference is to implement 2). Could you attach a new patch with 2) implemented?

Comment 8

[Frédéric Buclin]

Holding approval till the patch is updated.

Comment 9

[David Lawrence [:dkl]]

Created attachment 327155 [details] [diff] [review]
Patch to cancel duplicate attachments when reloading form (v3)

New patch addressing final suggestions by LpSolit. 

I updated the back to bug link to use bug_link(bugid) instead. Eventually I suppose we should also have a attach_link(attachid) similar utility function but that is a different bug. Also added my name to the contributor list.

Please approve
Dave

Comment 10

[Frédéric Buclin]

Comment on attachment 327155 [details] [diff] [review]
Patch to cancel duplicate attachments when reloading form (v3)

r=LpSolit

Comment 11

[David Lawrence [:dkl]]

3.2:
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.144.2.1; previous revision: 1.144
done
Checking in template/en/default/attachment/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.35.2.1; previous revision: 1.35
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/Attic/cancel-create-dupe.html.tmpl,v
done
Checking in template/en/default/attachment/cancel-create-dupe.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/Attic/cancel-create-dupe.html.tmpl,v  <--  cancel-create-dupe.html.tmpl
new revision: 1.1.2.1; previous revision: 1.1
done

tip:
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.145; previous revision: 1.144
done
Checking in template/en/default/attachment/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.36; previous revision: 1.35
done
Checking in template/en/default/attachment/cancel-create-dupe.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/cancel-create-dupe.html.tmpl,v  <--  cancel-create-dupe.html.tmpl
new revision: 1.2; previous revision: 1.1
done