438355 - Empty items in flagged add-ons

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect)

Description

[Basil Hashem [:baz]]

I have two "empty" items in the flagged add-ons list, I believe these show up since either the add-on got "admin disabled" or all version files were deleted. In any case, we need to change the query on this page so that an add-on description shows up.

Comment 1

[Basil Hashem [:baz]]

Attachment: Empty Flagged Items

Comment 3

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 1

This patch needs a new, little icon, which I already committed in r23644 (so make sure to svn up before trying out the patch).

The patch fixes the superreview list by avoiding warnings, and also adds another column: The add-on name now links to the admin "edit add-on" interface, while the icon links to the version that was flagged, if applicable (i.e., if that version still exists).

Comment 4

[Jeff Balogh (:jbalogh)]

Comment on attachment 368549 [details] [diff] [review]
Patch, rev. 1

Functionally the patch is good, but there's some L10n and validation issues.

Why bgcolor instead CSS?  Do we have a <td class=even> rule anywhere?  The validator says: there is no attribute "bgcolor".

In the second <td>, will concatenating the add-on name and version string play nice for localizers?

Should "Unflag Selected Add-ons" be localized?

It's not really a new string, but should we still use ___() for 'editors_notice_none_found?

There's some trailing whitespace in the foreach loop.

Validator:
The image needs an alt.

The inputs needs to be closed.

The input under the table needs to be inside a <p> or a <div>.

Comment 5

[Fred Wenzel [:wenzel]]

Thanks! Most of these concerns were present in the old code already, but that's no need not to fix them of course :) I'll go ahead and make a new patch.

Comment 6

[Fred Wenzel [:wenzel]]

Attachment: Patch, rev. 2

This should address most your comments -- in particular, the page validates now. However, I did not internationalize it (though I used sprintf for the name/version to facilitate i18n later), because none of our admin-only interfaces is localized to date and I don't think this is currently high on the priority list.

Of course, if you feel like I should add tags on this page in an effort to eventually localize the entire page, I can do that as well.

Comment 7

[Jeff Balogh (:jbalogh)]

Comment on attachment 369400 [details] [diff] [review]
Patch, rev. 2

Looks good to me.  If we're not localizing the admin, I certainly don't want you to waste time doing that!

Comment 8

[Fred Wenzel [:wenzel]]

Done, thanks: r24013.

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

Fred: https://preview.addons.mozilla.org/en-US/admin/flagged; do you see the empty add-on there?

<td><input type="checkbox" name="data[Addon][unflag][]" value="7881"/></td>
        <td><a href="/en-US/admin/addons/status/7881" >.</a> </td>     
                <td>&nbsp;</td>                
                <td colspan="3">&nbsp;</td>

Screenshot: http://www.grabup.com/uploads/fbec2af0486b3871ef28a2bb600ebbe6.png

I guess it's not really empty in the sense that it has "." as its name -- what do I do about that; file a separate bug?

Thanks!

Comment 10

[Fred Wenzel [:wenzel]]

(In reply to comment #9)
> I guess it's not really empty in the sense that it has "." as its name -- what
> do I do about that; file a separate bug?

Considering it's "addon not found" in production, not sure if that's a bug worth filing (it may be disabled already). at any rate, it's not a problem with this patch, as the add-on's name actually *is* ".".

Does that make sense?

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #10)
> (In reply to comment #9)
> > I guess it's not really empty in the sense that it has "." as its name -- what
> > do I do about that; file a separate bug?
> 
> Considering it's "addon not found" in production, not sure if that's a bug
> worth filing (it may be disabled already). at any rate, it's not a problem with
> this patch, as the add-on's name actually *is* ".".
> 
> Does that make sense?

Yeah, it does -- I was kind of asking a rhetorical question, thanks!

Looks good on https://preview.addons.mozilla.org/en-US/admin/flagged; verified fixed (though I think there might not be enough data on preview, really; we'll reopen this bug *if* it's not fixed post production-push).