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

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Attachments & Requests
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: wescottnimbus7, Assigned: dkl)

Tracking

3.1.3
Bugzilla 3.2
Bug Flags:
approval +
approval3.2 +
blocking3.2 -

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
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.
Status: UNCONFIRMED → NEW
Component: Creating/Changing Bugs → Attachments & Requests
Depends on: 398428
Ever confirmed: true
Flags: blocking3.2+
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 3.2
Version: unspecified → 3.1.3
(Assignee)

Comment 2

10 years ago
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
Assignee: create-and-change → dkl
Status: NEW → ASSIGNED
Attachment #325637 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #325637 - Flags: review? → review?(mkanat)

Comment 3

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

Attachment stuff is LpSolit's.
Attachment #325637 - Flags: review?(mkanat) → review?(LpSolit)

Comment 4

10 years ago
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.
Flags: blocking3.2+ → blocking3.2-

Comment 5

10 years ago
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>.
Attachment #325637 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 6

10 years ago
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
Attachment #325637 - Attachment is obsolete: true
Attachment #326948 - Flags: review?(LpSolit)

Comment 7

10 years ago
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 <olav@bkor.dhs.org>

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?
Attachment #326948 - Flags: review?(LpSolit) → review+

Comment 8

10 years ago
Holding approval till the patch is updated.
Flags: approval?
(Assignee)

Comment 9

10 years ago
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
Attachment #326948 - Attachment is obsolete: true
Attachment #327155 - Flags: review?(LpSolit)

Comment 10

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

r=LpSolit
Attachment #327155 - Flags: review?(LpSolit) → review+

Updated

10 years ago
Flags: approval?
Flags: approval3.2+
Flags: approval+
(Assignee)

Comment 11

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.