Page MenuHomePhabricator

Render ApplicationSearch errors correctly
ClosedPublic

Authored by epriestley on Jul 7 2015, 12:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Sep 10, 8:07 PM
Unknown Object (File)
Sun, Sep 8, 7:13 PM
Unknown Object (File)
Mon, Aug 26, 2:29 PM
Unknown Object (File)
Wed, Aug 21, 8:05 AM
Unknown Object (File)
Aug 17 2024, 11:23 AM
Unknown Object (File)
Aug 17 2024, 4:56 AM
Unknown Object (File)
Aug 9 2024, 5:05 AM
Unknown Object (File)
Aug 8 2024, 2:30 AM
Subscribers

Details

Summary

Fixes T8774. When an ApplicationSearch page returned an error (e.g., using a "viewer()" query while logged out), we would try to add action links to a box without a header. Instead:

  • If a box has no header, but has show/hide actions, just create an empty header so the view renders. This is a little silly looking but does what the caller asks.
  • Always set the title on the result box, so we get a header.
Test Plan

Did an invalid (logged out, with viewer()) search. Did a valid search.

Diff Detail

Repository
rP Phabricator
Branch
searchheader
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 7177
Build 7399: [Placeholder Plan] Wait for 30 Seconds
Build 7398: arc lint + arc unit

Event Timeline

epriestley retitled this revision from to Render ApplicationSearch errors correctly.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: chad, btrahan.

Whats the silly state look like?

chad edited edge metadata.
This revision is now accepted and ready to land.Jul 7 2015, 12:58 PM

Screen Shot 2015-07-07 at 5.59.13 AM.png (1×1 px, 203 KB)

If that's too silly to exist, we can just throw instead, but it seemed like a reasonable rendering of the view as configured.

My only suggestion would be to set a default header title then like "Query", or is that not possible?

We'll always set a title on this particular box, now.

The issue is if we make this mistake in some other application (forget to set a title) -- say, maybe in Nuance some day or something -- our options are:

  1. No change, producing a hard crash, like the report in T8774.
  2. This change, rendering the box as requested but being "silly", with no title, and possibly escaping notice since it's less obvious that it's (almost certainly) wrong.
  3. Throw an exception, producing a soft crash and a clear error message ("You must set a title when adding actions.").
  4. Provide some default title, but "Query" isn't a great one because the actual box's intended title might be "Items in Nuance Queue" or something. We could use a title like "Untitled Box".

I think (2), (3) or (4) are better than (1), but don't have a strong preference.

(I'm just going to land this as-is, but yell if you want to go a different direction.)

This revision was automatically updated to reflect the committed changes.