Closed Bug 371262 Opened 17 years ago Closed 16 years ago

Adding a preview image after delete image causes wrong image to appear until edit image

Categories

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Product:
addons.mozilla.org Graveyard
Component:
Developer Pages
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: gjblee, Assigned: fligtar)

References

(
URL
)

Details

Attachments

(1 file, 1 obsolete file)

3.5 patch, v1
45.18 KB, patch
wenzel
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20061208 Firefox/2.0.0.1
Build Identifier: Remora

If I delete a preview image and then add another preview image, the deleted image will reappear.  If I then edit the caption for an image or check the "use as default preview image", the image is corrected.  This happens whether or not I entered a caption when uploading the image.

Reproducible: Always

Steps to Reproduce:
1. Go to developer panel and add an image using the "Add image" link
2. Click on the image and delete it using the "Delete Preview" button.
3. Now go and add a different image using the "Add image" link.  The old image you uploaded in step #1 should appear.
4. Now type in a new caption for the image and hit "Edit preview".  The correct image you uploaded in step #3 should appear.
Actual Results:  
In step #3, I saw the old image from step #1.

Expected Results:  
In step #3, I should have seen the image I uploaded in step #3.
After further examination, it appears that the issue is that my web browser (Firefox 2.0.0.1 for Linux) is caching the old images.  Since the images are numbered sequentially (1,2,3,...), a newly uploaded image has the same URI as the old image that was deleted and Firefox loads the cached image.  One fix could be to never reuse image numbers so there is no possibility of conflict.  Another fix is to not let the browser cache preview images.
OS: Linux → All
Hardware: PC → All
Version: unspecified → 3.0
Additionally, the netscaler will be caching the previews, which means people who aren't logged in will see the old image for "a period of time."  Not reusing image numbers sounds like a good fix, but I haven't looked at how that code works.
We've talked about putting the image's modification time in the URL we use for the images, which would then let us basically say "cache forever, kthx" and always have the right new image show up as soon as it was changed.  Sancus may recall more.

I think we should just stop using the incremental preview number system and use preview ids. That would solve this problem and be way less annoying in the code.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I agree with flig, there's no real use for the users to have sequential ids in the preview URL, so we can just as well use their database ids.

The fix probably doesn't even need to much code changes, because most (all?) preview image URLs are generated by the image component.
I agree that we should probably change this to just use preview ids, but I'll check to see if there's any reason not to do that before I fix this.
Assignee: nobody → sancus
This is probably a related bug: I had similar wrong-image issues with my extension Grab and Drag. I changed the description of the image to match the one being shown and the image being shown changed! (so that the description is still wrong). Changing the description of a preview image to match a description already used seems to display whatever previous image had that description.
Blocks: 374817
Agree that we should use preview IDs.  My original design for this was clearly busted, sorry about that. :(

Increasing severity; not using preview ids is blocking several other bugs - some because of the actual problem and some because I don't want to make changes that will just have to be re-done when we do switch to ids.
Severity: normal → major
Blocks: 376396
Comment on attachment 262547 [details] [diff] [review]
patch to add support for returning images by preview id

r=fligtar

Looks good.

The use of $addon in the controller actions as both an add-on id and a preview id is kinda confusing though. Might want to rename it or document it.
Attachment #262547 - Flags: review? → review+
Yeah, I'll add a comment about that in the code - once we've purged the use of the old urls from all the pages on the site though, we can just remove the dual use completely anyway.
I see this behavior for the most part, although I can't get the right image to appear, even when I edit the preview (perhaps because of the NetScaler caching?).  But I also see other wonky behavior, like two "No caption" lines in the "Previews" section of the entry for the addon in the Developer Control Panel.

And a second preview I uploaded shows some image (looks like a background image for some UI element in the site) that isn't what I uploaded at all.  Perhaps this is a separate bug, but maybe it'll get fixed by the patch in this bug?
I see there's already a patch submitted, so I'm probably not adding anything useful, but here was my experience with this bug today: 

 * Uploading a new screenshot caused an old (weeks old) screenshot to be repeated instead. 
 * Deleting previews would cause the wrong one to be deleted
 * Uploading previews would cause the captions to be attached to the wrong 
 * Couldn't get the default preview image to change. 

All very frustrating. In the end the workaround I discovered was to upload new preview images without captions, then edit the captions afterwards. So I'm mainly posting to let others know about that who find this bug themselves.
I ran into the same issues as Seth.  I spent a good hour trying to second guess which caption would go with what image.  When I finally got everything to match up, I exited the dev cp.  Came back a little later to check the add-ons page.  Everything majorly fubared.  Multiple duplicate images not matching captions and preview images bringing up completely different images when clicked.

Totally unusable as is. 
(In reply to comment #15)
>In the end the workaround I discovered was to upload new
>preview images without captions, then edit the captions afterwards.
This doesn't work for me.

This wouldn't have been a problem if the preview page had been correct when it suggested that the preview is limited to 700x525; it is in fact 200x100.
(In reply to comment #17)
> >In the end the workaround I discovered was to upload new
> >preview images without captions, then edit the captions afterwards.
>This doesn't work for me.
Nor me. I have a preview that just won't go away - is there a workaround for these problems? I have a sneaking suspicion that the problem is just with caching. Maybe if I just upload and ignore the result, everything will be ok once everything gets recached?

>This wouldn't have been a problem if the preview page had been correct when it
>suggested that the preview is limited to 700x525; it is in fact 200x100.
I have a preview >200x100. I think that limit is just for the "default" image

Confirmed that after a few hours everything sorts itself out.

So until the preview ID patch is applied, the following seems to work:

* Starting from the bottom, delete images as required. Delete according to the caption - if the wrong image is displayed, just ignore it.
* Add new images, including captions. Never mind if they don't actually appear.
* Go for a drink.

Devs: if you don't have the time to fix this soon, please at least put a link to this bug on the submit page in the meantime.
Yes, some sort of message to the people uploading the images to let them know that although it may look broken during the process it'll sort itself out in a few minutes would be a good stopgap measure.
Target Milestone: --- → 3.4
Assignee: sancus → nobody
Depends on: 425315
Blocks: 431010
Target Milestone: 3.4 → 3.5
This is still happening and it's horribly painful. Any chance of moving it into the 3.4 milestone where the bug it's dependent on is?
Attached patch 3.5 patch, v1 — — Splinter Review 
This patch changes the default preview URL methods to use the preview ID instead of an incremental number. It's a complete revamp of the preview display system, and adds the last modified time to preview and icon URLs so changes will show up instantly in the Dev CP and much quicker on the public pages.

I removed the $screenshot parameter of the add-on display view because, as far as I know, we don't use it, and it would require a method all to itself to function again.

Old URLs that use the /addon_id/preview_num format will still work, but it's no longer possible to generate those URLs from AMO -- they will all use the new /p/preview_id/timestamp or /t/preview_id/timestamp URLs.
Assignee: nobody → fligtar
Attachment #262547 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #324542 - Flags: review?(fwenzel)
I should mention this patch is against AMO 3.5 as that's where it will be checked in, although it will probably work for trunk too as I very recently merged.
Comment on attachment 324542 [details] [diff] [review]
3.5 patch, v1

Ah yes, that seems to work quite nicely. Good job.

("Serves 150-200 million" is a nice serving size too. How many calories?)
Attachment #324542 - Flags: review?(fwenzel) → review+
(In reply to comment #24)
> I should mention this patch is against AMO 3.5 as that's where it will be
> checked in, although it will probably work for trunk too as I very recently
> merged.
> 

Yeah the patch works on trunk without problems.
Thanks!

Checked into 3.5 branch, r15257.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Verified FIXED; while testing, filing, and verifying bug 449476 and bug 450811, this has been verified on http://remora-3.5.php5stage.mozilla.com/en-US/developers/previews/7522/, etc.
Status: RESOLVED → VERIFIED
I still see this behavior for my extension (7026). I'm note sure whether this is fixed now, because I heard developer pages are 3.5.*. If it's not pushed, please ass keyword "push-needed". If it is pushed, there's maybe still something wrong.
(In reply to comment #29)
> please ass keyword
Sowwy... typo. Pleas add keyword.


I'm still seeing this as broken today as well.  The first delete I did updated, correctly, but subsequent deletes did not.  After I did the deletes I added some previews and they all display the images of the old deleted items.

3.5 was supposedly released last week so it doesn't look like this was fixed.
(In reply to comment #31)
> 3.5 was supposedly released last week so it doesn't look like this was fixed.

It's not. 3.5 is code complete since last week, and you can test out its behavior on http://preview.addons.mozilla.org if you like. But it's not in production yet, sorry. (Justin may be able to tell you more, when he gets back online after the whole "Gustav" thing.)
Then please add the keyword "push-needed" to clear all confusion about this...
I was told that was the normal way things work for amo bugs/fixes/releases.
(In reply to comment #33)
> Then please add the keyword "push-needed" to clear all confusion about this...

Yes, sorry.
Keywords: push-needed
Fligtar -- is this scheduled to get pushed this Thursday?
yes
Keywords: push-needed
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: