Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=uiExamples, ref.master=dd24c94b0741, ref.uiExamples=dd24c94b0741)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/view/phui/PHUISegmentBarSegmentView.php:58]
```
Closes T15441
Test Plan:
After applying this change, going to `/uiexample/view/PhabricatorAphrontBarUIExample/`,"Bars (PhabricatorAphrontBarUIExample)" renders with beautiful colors.
To see that page, enable prototypes with:
./bin/config set phabricator.show-prototypes true
Also try to create a Milestone on a Project, with the Config `maniphest.points` enabled, and try to create some Tasks with different points. Visit its Workboard to see the bar that still works with a useful tooltip.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15441
Differential Revision: https://we.phorge.it/D25272
Summary:
This fixes this specific exception that can happen with whatever PHP version in some pages:
Call to phutil_nonempty_string() expected null or a string, got: PhutilURI from PHUIObjectItemView.php:662
The regression was introduced since:
b56d86e48d
The problem is caused by the fact that we are trying to introduce very strict checks, and sometime we are too much strict.
In this specific case we use `phutil_nonempty_stringlike()` since it also allows objects with a `__toString()` method.
In order not to leave these cases to chance, we have added a log line, which can be removed in the future.
If you see this log line, report it in the respective Task. Thank you!
Closes T15306
Ref T15316
Test Plan:
1. {nav Dashboard > Add a Panel}: no crash
2. {nav Diffusion repo > README}: no crash
3. {nav Herald > Create}: no crash
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, aklapper, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15306, T15316
Differential Revision: https://we.phorge.it/D25157
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as
a general replacement.
In this specific case we use `phutil_nonempty_stringlike()` since we are not sure
if the variable `href` should be just a string or other objects.
In order not to leave these cases to chance, we have added a log line, which can be
removed in the future.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15303
Ref T15316
Test Plan:
Applied this change (on top of D25144, D25145, D25146, D25147, D25150,
D25142) and `/project/` rendered in web browser.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15303, T15316
Differential Revision: https://we.phorge.it/D25153
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15295
Test Plan: Applied these four changes (on top of D25144 and D25145) and `/dashboard/` finally rendered in web browser.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15295
Differential Revision: https://we.phorge.it/D25146
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15307
Test Plan:
Applied these two changes (on top `D25144`, `D25145`, `D25146`, `D25147`, `D25150`,
`D25151`, `D25152`, `D25153`, `D25154` locally applied) and `/conduit/` finally
rendered in web browser. Also the page /conduit/query/.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15307
Differential Revision: https://we.phorge.it/D25156
Summary:
Fix numerous PHP 8.1 RuntimeExceptions caused by the deprecation of strlen(null).
The strlen() was used in Phabricator to check if a generic value was a non-empty string.
For this reason, Phorge adopts phutil_nonempty_string() that checks that.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If your phutil_nonempty_string() throws an exception, just
report it to Phorge to evaluate and fix together that specific corner case.
Closes T15264
Test Plan: Phorge homepage is displayed on PHP 8.1 after applying these changes
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15264
Differential Revision: https://we.phorge.it/D25132
Summary: Ref T13682. Allow users to manually attach files which are referenced (but not attached) via the UI.
Test Plan: Reference files via `{F...}`, then attached them via the UI workflow.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21837
Summary:
Ref T9764. These "star" icons are unclear, inconsistent, and not friendly to colorblind users.
They date from a time long ago when the product didn't have icons.
Modernize them and make them more consistent with the similar statuses in Harbormaster.
Test Plan:
{F8545690}
{F8545691}
{F8545692}
Maniphest Tasks: T9764
Differential Revision: https://secure.phabricator.com/D21639
Summary: Ref T13602. Similar to subscriber and mention treatments, make it clear when a user doesn't have view permission.
Test Plan: {F8430595}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21555
Summary:
Ref T13602. When rendering a user hovercard, pass the object on which the reference appears. If the user can't see the object, make it clear on the hovecard.
Restyle the "nopermission" markup in mentions to make it more obvious what the style means: instead of grey text, use red with an explicit icon.
Test Plan: {F8430398}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21554
Summary:
Ref T13602. When a subscriber can't see an object, it's currently hard to figure it out.
Show this status clearly in the curtain UI.
Test Plan: {F8382865}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21547
Summary:
Ref T13573. Using the browser "Print" feature on pages produces "Thu, Aug 4, 12:22" timestamps which require context to interpret precisely (they don't have a year and don't have a timezone).
Instead, retain these timestamps in "screen" contexts but use "YYYY-MM-DD HH:MM:SS (UTC+X)" timestamps when printing.
Test Plan: Printed Maniphest tasks and other pages in Safari and Chrome using "?__print__=1" and "Print to PDF", saw absolute timestamps after this chagne in the printed documents.
Maniphest Tasks: T13573
Differential Revision: https://secure.phabricator.com/D21451
Summary: Ref T13552. There are currently some content overflow issues on the graph view where the menu height can exceed the content height and the frame is drawn on a sub-element. Make the frame draw around all the content.
Test Plan: Viewed commit graph history view, saw more sensible UI.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21440
Summary:
Ref T13552. Build the "commit list" elements so that the menu action items collapse under the element on mobile.
Also change the mobile breakpoint to 512px because my Safari window can't go any narrower than 508px. Future changes to responsive design will be more content-aware anyway.
Test Plan: Looked at commits in various interfaces, at desktop and mobile widths.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21430
Summary:
Ref T13552. The current layout doesn't work particularly well on desktops or devices.
We have some device/desktop table layout code, but it isn't generic. We also have property list layout code, but it isn't generic either.
Provide generic layout elements ("Fuel", from "Phabricator UI Layout" to "PHUIL"?) and narrowly specialize their display behavior. Then swap the ListItemView stuff to use it.
Test Plan:
Saw slightly better responsive behavior:
{F7637457}
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21418
Summary:
Ref T13552. In unifying the various Graph/List/Table commit views, some information was dropped -- particularly, audit status.
Restore most of it. The result isn't very pretty, but has most of the required information.
Test Plan: {F7637411}
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21417
Summary:
At some point, the highlighting behavior for the timeline broke. When you follow a link to a particular timeline story, the story should be highlighted.
Prior to this change, the `<a />` tag itself highlights, but there's no associated CSS and it's too deep in the tree to do anything useful.
(Since this change is fairly straightforward, I gave up digging for the root cause before finding it.)
Test Plan:
- Clicked a timeline story anchor, saw the story highlight.
Differential Revision: https://secure.phabricator.com/D21213
Summary: Ref T13516. Apply basic UI styling to the new UI and make some more interaction work.
Test Plan: {F7374096}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21153
Summary:
Ref T13515. Settings currently has some highly specialized code for rendering "Changes saved." messages. The "saved" state is communicated across a redirect-after-POST by adding `/saved/` to the end of the URI.
This isn't great. It needs a lot of moving pieces, including special accommodations in routing rules. It's user-visible. It has the wrong behavior if you reload the page or navigate directly to the "saved" URI.
Try this scheme, which is also pretty sketchy but seems like an upgrade on the balance:
- Set a cookie on the redirect which identifies the form we just saved.
- On page startup: if this cookie exists, save the value and clear it.
- If the current page started with a cookie identifying the form on the page, treat the page as a "saved" page.
This supports passing a small amount of state across the redirect-after-POST flow, and when you reload the page it doesn't keep the message around. Applications don't need to coordinate it, either. Seems somewhat cleaner?
Test Plan: In Firefox, Safari, and Chrome: saved settings, saw a "Saved changes" banner without any URI junk. Reloaded page, saw banner vanish properly.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21144
Summary:
Ref T13515. Adding "\" ("Open in External Editor") made this slighlty worse, but it was already pretty bad.
Long ago the keys had a special style on them, but this got changed and dropped somewhere around D16568 -- although at the time, I think they still had a grey background (see T11654).
Some later change removed this background.
Put the background back and separate the keystrokes into groups.
Test Plan: {F7370615}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21141
Summary:
Depends on D20966. Ref T13486. Curtains currently render subscribers in a plain text list, but the new ref list element is a good fit for this.
Also, improve the sorting and ordering behavior.
This makes the subscriber list take up a bit more space, but it should make it a lot easier to read at a glance.
Test Plan: Viewed object subscriber lists at varying limits and subscriber counts, saw sensible subscriber lists.
Maniphest Tasks: T13486
Differential Revision: https://secure.phabricator.com/D20967
Summary:
Ref T13486. When a curtain element like "Author" in Maniphest has a very long username, the wrapping and overflow behavior is poor: the date is obscured.
Adjust curtain elements which contain lists of references to other objects to improve wrapping behavior (put the date on a separate line) and overflow behavior (so we get a "..." when a name overflows).
Test Plan: {F7179376}
Maniphest Tasks: T13486
Differential Revision: https://secure.phabricator.com/D20966
Summary: Ref T13480. Creating a rule in Herald currently uses the older radio-button flow. Update it to the "clickable menu" flow to simplify it a little bit.
Test Plan: Created new personal, object, and global rules. Hit the object rule error conditions.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20956
Summary: Depends on D20931. Ref T13362. Move all "Console"-style interfaces to use a consistent layout based on a new "LauncherView" which just centers the content.
Test Plan: Viewed all affected interfaces.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13362
Differential Revision: https://secure.phabricator.com/D20933
Summary:
Fixes T13461. Some applications provide hints about policy strength in the header, but these hints are inconsistent and somewhat confusing. They don't make much sense for modern objects with Custom Forms, which don't have a single "default" policy.
Remove this feature since it seems to be confusing things more than illuminating them.
Test Plan:
- Viewed various objects, no longer saw colored policy hints.
- Grepped for all removed symbols.
Maniphest Tasks: T13461
Differential Revision: https://secure.phabricator.com/D20918
Summary:
Fixes T13452. We currently give users mixed signals about the interaction mode of this text: the cursor says "text" but the behavior is "grab".
Make the behavior "text" to align with the cursor. An alternate variation of this change is to remove the cursor, but this is preferable if it doesn't cause problems, since copying the task ID is at least somewhat useful.
Test Plan: In Safari, Firefox, and Chrome: selected and copied object names from workboard cards; and dragged workboard cards by other parts of their UI.
Maniphest Tasks: T13452
Differential Revision: https://secure.phabricator.com/D20898
Summary:
Ref T13289. This tightens up a couple of corner cases around locked threads.
Locking is primarily motivated by two use cases: stopping nonproductive conversations on open source installs (similar to GitHub's feature); and freezing object state for audit/record-keeping purposes.
Currently, you can edit or remove comments on a locked thread, but neither use case is well-served by allowing this. Require "CAN_INTERACT" to edit or remove a comment.
Administrators can still remove comments from a locked thread to serve "lock a flamewar, then clean it up", since "Remove Comment" on a comment you don't own is fairly unambiguously an administrative action.
Test Plan:
- On a locked task, tried to edit and remove my comments as a non-administrator. Saw appropriate disabled UI state and error dialogs (actions were disallowed).
- On a locked task, tried to remove another user's comments as an administrator. This works.
- On a normal task, edited comments normally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20551
Summary:
Depends on D20465. Ref T13277. Currently, when a commit is unpublished, we put a single line about it on the "Edit Commit" page. This is pretty much impossible to find.
Move it to the main page. This treatment is more big/bold than I'd probably like to end up, but we should probably overshoot on the explanatory text until users get used to this behavior.
Also, allow searching for only published / unpublished commits.
Test Plan: {F6395705}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20466
Summary:
Depends on D20414. Ref T13272. Several minor things here:
- Currently, you can drag panels underneath the invisible "there are no items in this column" div and the "Create Panel / Add Existing Panel" buttons. This is silly; stop it.
- Currently, when viewing a tab panel on a dashboard, you can drag the panels inside it. This is extremely silly. Make "movable" off by default and pass it through the async flow only when we actually need it.
- Make the whole "Add Tab..." virtual tab clickable to open the dropdown. This removes the rare exception/todo combo I added earlier. {key F}
- Add or remove some icons or something.
Test Plan: Moved panels around on dashboards. Tried to drag panels inside tab panels. Added tab. Things were less obviously broken.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20415
Summary:
Depends on D20410. Ref T13272. Dashboards/Panels currently use older "ngram" indexing, which is a less-powerful precursor to Ferret. Throw away the ngram index and provide a Ferret index instead. Also:
- Remove the NUX state, which links to the wrong place now and doesn't seem terribly important.
- Add project tags to the search result list.
- Make the "No Tags" tag a little less conspicious.
Test Plan:
- Indexed dashboards and panels.
- Searched for dashboards and panels via SearchEngine using Ferret "query" field.
- Searched for panels via "Add Existing Panel" datasource typeahead.
- Searched for dashboards via "Add Menu Item > Dashboard" on a ProfileMenu via typeahead.
- Viewed dashboard NUX state (no special state, but no more bad link to "/create/").
- Viewed dashboard list, saw project tags.
- Viewed dashboards with no project tags ("No Tags" is now displayed but less visible).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20411
Summary:
Ref T13272. In edit mode, tab panels now have a dropdown menu. However, this sort of overrlaps with the actual action of clicking the tab to select it.
Separate these into different click targets so that "select tab X" and "open dropdown menu for X" are different operations.
This is more work than it appears because:
- We have an "action icon" already, used when you put a dashboard on a portal/home to create an "Edit" link. It makes sense to attach dropdowns to this, but it has some hard-coded stuff.
- In applications with a "Create <thing>" in the crumbs (like Maniphest), we may use a dropdown menu if there are multiple create forms available. However, this menu renders in a weird way by reading all the properties out of an actual "View" object and building something else.
- The "list of tabs" stuff shares code with different "list of tabs" navigation used by Diffusion and Instances.
..but I think I fixed everything and didn't break anything.
Test Plan:
- Clicked "select tab" and "open dropdown menu" as separate actions.
- Viewed Diffusion, Maniphest with multiple create forms, Instances.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20396
Summary:
Depends on D20383. Ref T13272. Fixes T12363. See PHI997. This gets the edit flows for tab panels functional again. They aren't //nice//, and a lot of the workflows are fairly janky: for example, most of them end up with you on the tab panel's page, which isn't useful if you started on a dashboard page.
However, these flows were extremely janky before anyway (see T12363) and I suspect this is a net improvement even though it's a bit of a mess. I anticipate cleaning this up bit-by-bit in future diffs.
Test Plan: {F6366372}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272, T12363
Differential Revision: https://secure.phabricator.com/D20384
Summary:
Depends on D20362. Ref T13272. Currently, Dashboards have an "Install Dashboard" flow which is pretty janky and only allows you to install things to the home page.
Instead, allow users to install things to any valid target (home, favorites, portals, projects). This also provides URIs like `dashboard/install/1/home/personal/` which allow you to link users to an "install a dashboard" page; this may or may not get used.
Test Plan: Installed dashboards on home, favorites, projects, and portals.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20364
Summary:
Depends on D20353. Ref T13275. This is just some small quality-of-life fixes:
- When you add items to menus, they currently go below the "Edit Menu/Manage Menu" links by default. This isn't a very good place for them. Instead, lock "edit" items to the bottom of the menu.
- Lock profile pictures to the top of the menu. This just simplifies things a little.
- Show more iconography hints on the "edit menu items" UI.
- Add a "drag stuff to do things" hint if some stuff can be dragged.
Test Plan:
- Added new items to a Portal, they didn't go to the very bottom. Instead, they went above the "Edit/Manage" links; a sensible place for them.
- Viewed the "edit menu items" screen, saw more hints and visual richness.
- Viewed/edited Home, Projects, Portals, Favorites
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20355
Summary:
In some cases, we show a limited number of one type of object somewhere else, like "Recent Such-And-Such" or "Herald Rules Which Use This" or whatever.
We don't do a very good job of communicating that these are partial lists, or how to see all the results. Usually there's a button in the upper right, which is fine, but this could be better.
Add an explicit "more stuff" button that shows up where a pager would appear and makes it clear that (a) the list is partial; and (b) you can click the button to see everything.
Test Plan: {F6302793}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20315
Summary:
Ref T13074. Today, in normal task list views in Maniphest (not workboards), you can (sometimes) reorder tasks if the view is priority-sorted.
I suspect no one ever does this, few users know it's supported, and that it was basically rendered obsolete the day we shipped workboards.
This also means that we need to maintain a global "subpriority" for tasks, which distinguishes between different tasks at the same priority level (e.g., "High") and maintains a consistent ordering on workboards.
As we move toward making workboards more flexible (e.g., group by author / owner / custom fields), I'd like to try moving away from "subpriority" and possibly removing it entirely, in favor of "natural order", which basically means "we kind of remember where you put the card and it works a bit like a sticky note".
Currently, the "natural order" and "subpriority" systems are sort of similar but also sort of in conflict, and the "subpriority" system can't really be extended while the "natural order / column position" system can.
The only real reason to have a global "subpriority" is to support the list-view drag-and-drop.
It's possible I'm wrong about this and a bunch of users love this feature, but we can re-evaluate if we get feedback in this vein.
(This just removes UI, the actual subpriority system is still intact and still used on workboards.)
Test Plan: Viewed task lists, was no longer able to drag stuff. Grepped for affected symbols. Dragged stuff in remaining grippable lists, like "Edit Forms" in EditEngine config.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074
Differential Revision: https://secure.phabricator.com/D20263
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.
Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20154
Summary: See PHI823. These got "visual-only" but should acutally get "aural => false" to pick up "aria-hidden".
Test Plan: Viewed page source, saw both "visual-only" and "aria-hidden".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20157
Summary:
Ref T13244. See PHI1059. When you lock a task, users who can edit the task can currently override the lock by using "Edit Task" if they confirm that they want to do this.
Mark these edits with an emblem, similar to the "MFA" and "Silent" emblems, so it's clear that they may have bent the rules.
Also, make the "MFA" and "Silent" emblems more easily visible.
Test Plan:
Edited a locked task, overrode the lock, got marked for it.
{F6195005}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: aeiser
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20131
Summary:
Depends on D20026. Ref T13222. Ref T13231. The primary change here is that we'll no longer send you an SMS if you hit an MFA gate without CSRF tokens.
Then there's a lot of support for genralizing into Duo (and other push factors, potentially), I'll annotate things inline.
Test Plan: Implemented Duo, elsewhere.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231, T13222
Differential Revision: https://secure.phabricator.com/D20028
Summary:
Depends on D19896. Ref T13222. See PHI873. Add a core "Sign With MFA" transaction type which prompts you for MFA and marks your transactions as MFA'd.
This is a one-shot gate and does not keep you in MFA.
Test Plan:
- Used "Sign with MFA", got prompted for MFA, answered MFA, saw transactions apply with MFA metadata and markers.
- Tried to sign alone, got appropriate errors.
- Tried to sign no-op changes, got appropriate errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19897
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
Try to clean this up:
- Stop requiring `willRenderTimeline()` to be implemented.
- Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
- Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
- If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
Test Plan:
- Viewed audits, revisions, and mocks with inline comments.
- Used "Show Older" to page a revision back in history (this is relevant for "View Data").
- Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19918
Summary: In D19855, I removed a no-longer-necessary link around icons in some cases, but incorrectly discarded labels in other cases. Restore labels.
Test Plan: Viewed Differential revision list, saw date stamps again.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19871
Summary:
Ref T13222. Fixes T12588. See PHI683. In several cases, we present the user with a choice between multiple major options: Alamnac service types, Drydock blueprint types, Repository VCS types, Herald rule types, etc.
Today, we generally do this with radio buttons and a "Submit" button. This isn't terrible, but often it means users have to click twice (once on the radio; once on submit) when a single click would be sufficient. The radio click target can also be small.
In other cases, we have a container with a link and we'd like to link the entire container: notifications, the `/drydock/` console, etc. We'd like to just link the entire container, but this causes some problems:
- It's not legal to link block eleements like `<a><div> ... </div></a>` and some browsers actually get upset about it.
- We can `<a><span> ... </span></a>` instead, then turn the `<span>` into a block element with CSS -- and this sometimes works, but also has some drawbacks:
- It's not great to do that for screenreaders, since the readable text in the link isn't necessarily very meaningful.
- We can't have any other links inside the element (e.g., details or documentation).
- We can `<form><button> ... </button></form>` instead, but this has its own set of problems:
- You can't right-click to interact with a button in the same way you can with a link.
- Also not great for screenreaders.
Instead, try adding a `linked-container` behavior which just means "when users click this element, pretend they clicked the first link inside it".
This gives us natural HTML (real, legal HTML with actual `<a>` tags) and good screenreader behavior, but allows the effective link target to be visually larger than just the link.
If no issues crop up with this, I'd plan to eventually use this technique in more places (Repositories, Herald, Almanac, Drydock, Notifications menu, etc).
Test Plan:
{F6053035}
- Left-clicked and command-left-clicked the new JS fanciness, got sensible behaviors.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19855
Summary:
Ref T13202. In D19660, I added comments to Phriction and tweaked some CSS.
One of these tweaks was getting rid of an extra border which was rendering under the comment area. However, I took off too much and ended up removing borders from other applications.
I think we don't actually need this `setNoBorder()` stuff after all -- a later change was sufficient to stop the actual border I was trying to get rid of from rendering. So this mostly just reverts part of D19660.
This rendering still isn't perfect, but I'm fine leaving that for another day for now.
Test Plan:
- Viewed comment areas in Phriction. Saw correct number of borders (1).
- Viewed comment areas in Maniphest. Saw correct number of borders (1).
- Grepped for extraneous removed classs, no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19684
Summary:
Depends on D19661. Ref T13077. See PHI840.
When a user edits a page normally, add a "Save as Draft" button. Much of this change is around making that button render and behave properly: it needs to be an `<input type="submit" ...>` so browsers submit it and we can figure out which button the user clicked.
Then there are a few minor rules:
- If you're editing a page which is already a draft, we only give you "Save as Draft". This makes edits to update/revise a draft more natural.
- Highlight "Publish" if it's a likely action that you might want to take.
Internally, there are two types of edits. Both types create a new version with the new content. However:
- A "content" edit sets the version shown on the live page to the newly-created version.
- A "draft" edit does not update the version shown on the live page.
Test Plan: Edited a published document, edited the draft. Published documents. Reverted documents.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19662
