Summary:
Ref T13513.
- Firefox represents multiple selected rows as a discontinuous range. Accommodate this.
- Unified diffs don't have a "copy" marker. Do something sort-of-reasonable for them.
Test Plan:
- Selected multiple lines of content in Firefox, got an option to add a comment.
- Selected content in unified mode, got an option to add a comment.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21249
Summary: Ref T13513. Support direct text selection for inlines. This is currently just an alternate way to get to the same place as using line numbers, but can preserve offset/range information in the future.
Test Plan:
- Selected some text, hit "c", clicked "New Inline Comment", got sensible comments on both sides of a diff in Safari, Chrome, and (with limitations) Firefox.
- Caveats: no unified support, doesn't work across lines in Firefox.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21248
Summary: Ref T11401. Ref T13513. This paves the way for more comment actions, particularly an edit-after-submit action.
Test Plan: Took all actions from menus, via mouse and via keyboard (where applicable).
Maniphest Tasks: T13513, T11401
Differential Revision: https://secure.phabricator.com/D21244
Summary: See <https://github.com/phacility/phabricator/pull/854>. In some situations, `line-break: anywhere` produces better behavior than `word-break: break-all`. It never appears to produce worse behavior.
Test Plan:
- Break behavior changes if a line contains "<span />" elements caused by syntax highlighting. This CSS adjustment only appears to apply to text with internal "<span />" elements.
- This specifically impacts certain internal breakpoints adjacent to punctuation, so the test case is highly specific. Generic test cases with latin word characters do not evidence any behavioral changes.
- This change appears to have no impact on Safari, which uses the better behavior in all cases.
- Before Patch: In Firefox and Chrome, this specific change breaks awkwardly. There is more room for text to fit on the broken line:
Firefox
{F7480567}
Chrome
{F7480568}
- After Patch: Firefox and Chrome break the line better. Here's Firefox:
{F7480569}
- Additional context:
Safari Behavior (Unchanged)
{F7480570}
Chrome with no highlighting (desirable behavior). Firefox does the same thing.
{F7480571}
Also tested other cases, which seem never-worse in any browser.
{F7480574}
Differential Revision: https://secure.phabricator.com/D21247
Summary:
Ref T13513. Currently, "View as Document Type..." lists every available engine.
This is hard to get completely right because we can't always rebuild the document ref accurately in the endpoint, but try harder to fake something reasonable.
Test Plan: Used "View as Document Type..." on Jupyter notebooks, was given "Jupyter" and "Source" as options.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21241
Summary:
Ref T13513. As part of inline metadata, save the document engine the change is being rendered with.
This will allow other parts of the UI to detect that an inline was created on a Jupyter notebook but is being rendered on raw source, or whatever else.
The immediate goal is to fix nonsensical inline snippet rendering in email on Jupyter notebooks.
Test Plan:
- Created inlines and replies on normal soure code, saw no document engine annotated in the database.
- Created inlines and replies on a Jupyter notebook rendered in Jupyter mode, saw "jupyter" annotations in the database.
- Swapped document engines between Jupyter and Source, etc.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21240
Summary: Ref T13513. Currently, if you're editing a comment, "delete" doesn't put the comment into the correct state. This action is normally only reachable from comment previews, since an editing inline has no "delete" button.
Test Plan:
- Started editing an inline, clicked "Delete", got a deletion.
- Created an inline, typed text,
- Deleted a normal comment via preview.
- Deleted a normal comment via the on-inline action.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21238
Summary:
Ref T13513. Currently, clicking "View" from the inline comment preview (below the "add comment" area at the bottom of the page) only works if the inline isn't being edited.
Update this behavior so it works on inlines in either "Viewing" or "Editing" states.
Test Plan:
- Clicked "View" on a normal inline, got jumped/selected.
- Clicked "View" on an editing inline, got jumped/selected.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21237
Summary:
Ref T13513. Currently:
- If you click the "Show Changeset" button, your state change doesn't actually get saved on the server.
- It's hard to select a changeset path name for copy/paste because the "highlight the header" code tends to eat the event.
Instead: persist the former event; make the actual path text not be part of the highlight hitbox.
Test Plan:
- Clicked "Show Changeset", reloaded, saw changeset visibility persisted.
- Selected changeset path text without issues.
- Clicked non-text header area to select/deselect changesets.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21236
Summary:
Ref T13513. Currently, inline storage objects ("TransactionComment") can't directly generate a runtime object ("InlineComment").
Allow this transformation to be performed in a genric way so clunky code which does it per-object-type can be removed, lifted, or simplified.
Simplify an especially gross callsite in preview code.
Test Plan: Previewed inline comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21226
Summary: See D21213. If there's no matching element, `findAbove()` throws. Handle these cases correctly.
Test Plan: Visited `#toc` on a revision, no longer saw a JS error.
Differential Revision: https://secure.phabricator.com/D21222
Summary:
Ref T13513. Overloading "original text" to get "edit-on-load" comments into the right state has some undesirable side effects.
Instead, provide the text when the editor opens. This fixes a cancel interaction.
Test Plan:
- Create an inline, type text, don't save.
- Reload page.
- Cancel.
- Before: cancelled into empty state.
- After: cancelled into deleted+undo state.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21219
Summary:
Ref T13513. This is a bit clumsy, but the cleanest way to implement "isEditing" inlines today is to send them down as normal inlines and then simulate clicking "edit" on them.
When we do, don't focus the resulting editor: focusing it makes the page scroll around and highlight things in essentially random order as the editors load in.
Test Plan: Reloaded a page with some open editors, wasn't scrolled to them.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21217
Summary:
Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.
This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.
Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21216
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
Summary: Ref T13513. This slightly expands the existing-but-hacky "warning" workflow to cover both "mentions on draft" and "submitting inlines being edited".
Test Plan:
- Submitted changes to a revision with mentions on a draft, inlines being edited, both, and neither.
- Got sensible warnings in the cases where warnings were appropriate.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21191
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.
This also serves to delete empty comments.
Test Plan:
- Clicked a line number to create a new comment. Then:
- Clicked "Cancel". Reloaded page, saw no more comment.
- Typed text, saved. Reloaded page, saw non-editing draft. Clicked "Edit", reloaded page, saw editing draft. Clicked "Cancel", reloaded page, saw non-editing draft.
- Typed text, saved. Clicked "Edit", deleted text, saved. Reloaded page, saw no more comment.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21187
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
Summary: Ref T13513. This plans for "currently editing", character range comments, code suggestions, document engine tracking. And absolutely nothing else.
Test Plan:
- Ran `bin/storage upgrade -f`, got a clean upgrade.
- Created and submitted some inline comments; nothing exploded.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21184
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 T13520. Generally, make the table of contents look and more like the paths panel:
- Show a hierarchy, with compression for single-sibling children.
- Use the same icons, instead of "M D" and "(img)" stuff.
- Use EditDistanceMatrix to do a piece-by-piece diff of paths changes.
- Show path changes within the path list.
I'm not entirely sold on this, but it was complicated to write and I've never heard the term "sunk cost fallacy". I think this is mostly a net improvement, but may need some adjustments and followup.
Test Plan: Viewed various changes in Differential and Diffusion, saw a more usable table of contents.
Maniphest Tasks: T13520
Differential Revision: https://secure.phabricator.com/D21183
Summary: Ref T13516. This isn't terribly clean, but get the page footer into the bottom of the content page on FormationView pages so it doesn't overlap into the side panel.
Test Plan: With and without a footer, viewed normal and FormationView pages. Saw footers in appropriate places at appropriate times.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21166
Summary: Ref T13516. Differential got some new UI elements and behaviors, so update static resource package definitions.
Test Plan:
- Saw JS requests drop from 17 to 4.
- Saw CSS requests drop from 9 to 3.
(These won't quite match production since some JS/CSS is for DarkConsole.)
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21163
Summary: Ref T13516. Hide this UI on devices without the screen width to reasonably support it.
Test Plan: Viewed a revision at various window widths, saw the elements vanish at device widths and reappear at desktop widths.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21162
Summary:
Ref T13516.
- Add an "Add Comment" navigation anchor.
- Make selection state more clear.
- Make hidden state tidier and more clear.
- Hide "View Options" in the hidden state to dodge all the weird behaviors it implies.
- Click to select/deselect changesets.
- When you open the view dropdown menu, then press "h", close the dropdown menu.
Test Plan: Fiddled with all these behaviors.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21161
Summary:
Ref T13516. Minor improvements here:
- Show key commands in the "View Options" dropdown.
- Organize it slightly better.
- Improve disabled item behaviors a little bit.
- Add a "Browse Directory" action.
- Rename "...in Diffusion" to "...in Repository".
- Make "d", "D", and "h" use the same targeting rules as "\".
- When you hide a file with the "h" menu item, select it.
Test Plan: Poked at the menu a lot, ran into less questionable behavior.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21160
Summary:
Ref T13455. Make "hidden" a changeset property similar to other changeset properties.
We don't need to render this on the server, so we make a request (to update the setting) and just discard the response.
Test Plan: {F7375468}
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21158
Summary: Ref T13516. Mark low-importance changes (generated code, deleted files) and owned-with-authority changes in the filetree.
Test Plan: {F7375327}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21157
Summary:
Ref T13516. Deletes all old filetree / flex / active / collapse nav code in favor of the new code.
Restores the inline tips in the path tree.
Test Plan: {F7374175}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21154
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 T13516. Generate a tree structure based on the page changesets. Still missing styles and a whole lot of behavior.
Test Plan: {F7373967}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21152
Summary: Ref T13516. This glues "FormationView" to "ChangesetList". The actual tree is not functional in any meaningful way yet.
Test Plan: {F7373838}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21151
Summary:
Ref T13516. Currently, the "File Tree" element is a semi-dynamic side panel that's implemented as a special mode of a side nav panel.
This implementation is fairly clunky, and arose from organic growth out of the side nav. As such, it has some weird behaviors, doesn't have builtin support for show/hide, and can't generalize easily.
Introduce a "FormationView" which supports loading a page up with piles of side panels in various modes.
Test Plan: No callers and no user-visible impact.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21150
Summary:
Ref T13515. This restores the "Open in Editor" behavior to Diffusion, and makes "\" work there.
The URI pattern is now sent as a structured template to the client, so the code will work properly if a file path contains "%l".
Test Plan:
- Clicked "Open in Editor" and pressed "\" in Diffusion when viewing a file.
- Clicked a line, hit "\", got the file opened to that line.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21149
Summary:
Ref T13515. Currently, "Open in Editor" only works with a file-level selection.
- If we have a change-level or inline-level selection, open the parent changeset.
- If we have no selection, but the banner is showing something, open the fine shown in the banner.
Test Plan: With files, inlines, changes, and no selection, pressed "\". Saw files pop open in my external editor.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21148
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:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.
Some complexity here arises from these concerns:
- In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
- In all other cases, we render the changeset with AJAX.
So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.
Then, some bookkeeping issues:
- At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
- Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".
Test Plan:
- Viewed changes in Differential, Diffusion, and in standalone mode.
- Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
- Started typing a comment, cancelled it, hit the undo UI.
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21138
Summary:
Ref T13455. Add container-level storage for persistent view state, and persist "Highlight As..." inside it.
The storage generates a "PhabricatorChangesetViewState" configuration object as an output.
When preferences are expressed on a diff and that diff is later attached to a revision, we attempt to copy the preferences.
The internal storage tracks per-changeset settings, but currently always uses "last update wins" to apply the settings in the UI.
Test Plan:
- Viewed revisions, changed highlighting, reloaded. Saw highlighting stick in revision view and standalone view.
- Viewed commits, changed highlighting, reloaded. Saw highlighting stick.
- Created a diff, changed highlighting, turned it into a revision, saw highlighting persist.
Subscribers: jmeador, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21137
Summary: Ref T13515. See PHI1661. If a file is selected, add a keystroke to click the "Open in External Editor" link.
Test Plan: In Safari, Chrome, and Firefox: used "J" to select a file, then "\" to open it in an external editor. (In Safari and Chrome, this prompts.)
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21135
Summary:
Fixes T13508. The "Notification" and "Messages" icons in the menu bar have a CSS transition animation on hover.
In Chrome, when this element moves up 2px, you can get a flicker in and out of the hover state if the user's cursor is at the very bottom of the element, since the bounding box for the element is rapidly sliding in and out of the area under the cursor.
To fix this: as we move the element up, also make it taller.
Test Plan: In Safari, Chrome, and Firefox: put my cursor at the very bottom of the element, no longer saw any animation flickering.
Maniphest Tasks: T13508
Differential Revision: https://secure.phabricator.com/D21133
Summary:
Fixes T13510. This migration currently fails because it tries to affect the "paste" database, but when it runs this database will be named "pastebin".
Since the cost of fixing it in place or moving it past the rename migration both seem relatively high (and the cost of throwing it away is plausibly zero) just throw it for now.
Test Plan: Looked at file, saw no more code that can execute.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13510
Differential Revision: https://secure.phabricator.com/D21132
Summary:
Ref PHI1292. Enable fulltext searchs in paste. Maybe this should only index a snippet instead of the entire content?
Also updates table names in `PhabricatorPasteQuery`.
Test Plan: Created some pastes, indexed them, searched for them.
Reviewers: amckinley
Subscribers: codeblock, Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20650
Summary:
See <https://discourse.phabricator-community.org/t/storage-upgrade-error/3748>.
It is broadly unsafe for migrations to use "save()". If the object gains new fields later, the query will include "SET newField = X", which will fail against the old schema which is in the process of being upgraded.
Instead, migrations must issue raw SQL against the schema as it is expected to exist at the time the migration executes.
Migrations have followed this rule for a long time, but this ~6 year old migration was overlooked. Update it to issue a raw query to perform the policy update.
Test Plan: This is somewhat flimsy since rebuilding a genuine reproduction case is messy, but used "bin/storage --apply ..." to at least get the new query to execute against modern Phabricator without issues.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D21124
Summary:
Fixes T5028. Older versions of Git (apparently, from before 2010) did not provide a way to extract the raw body of a commit message from "git log", so we approximate it with "subject" and "wrapped body".
In newer versions of Git, the raw body can be extracted exactly.
Adjust how we extract messages based on the version of Git, and try to be more faithful to edge cases: particularly, be more careful to extract the correct number of trailing newlines.
Test Plan:
- Added "var_dump()" + "die(1)" later in this method, then pushed various commit messages. Used "&& false" to force execution down the old path (either path should work in modern Git).
- Observed more faithful extraction of messages, including a more faithful extraction of the number of trailing newlines. Extraction is fully faithful if we can go down the "%B" path, which we should be able to in nearly all modern cases.
- Not all messages extract faithfully or consistently across the old and new versions, but the old extraction is destructive so this is likely about as close as we can realistically ever get.
Maniphest Tasks: T5028
Differential Revision: https://secure.phabricator.com/D21027
Summary:
Depends on D21018. Ref T13493. Ref T6703. The "ExternalAccount" table has a unique key on `<accountType, accountDomain, accountID>` but this no longer matches our model of reality and changes in this sequence end writes to `accountID`.
Remove this key.
Then, remove all readers of `accountType` and `accountDomain` (and all nontrivial writers) because none of these callsites are well-aligned with plans in T6703.
This change has no user-facing impact today: all the rules about linking/unlinking/etc remain unchanged, because other rules currently prevent creation of more than one provider with a given "accountType".
Test Plan:
- Linked an OAuth1 account (JIRA).
- Linked an OAuth2 account (Asana).
- Used `bin/auth refresh` to cycle OAuth tokens.
- Grepped for affected symbols.
- Published an Asana update.
- Published a JIRA link.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13493, T6703
Differential Revision: https://secure.phabricator.com/D21019
Summary: Depends on D21016. Ref T13493. This copies existing external account "accountID" values into the "ExternalAccountIdentifier" table, preparing for an authority switch.
Test Plan: Ran migration several times, looked at the data that came out of it, saw sensible results. Logged out / in with external accounts.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21017
Summary:
Depends on D21010. Ref T13493. External accounts may have multiple different unique identifiers, most often when v1 of the API makes a questionable choice (and provies a mutable, non-unique, or PII identifier) and v2 of the API uses an immutable, unique, random identifier.
Allow Phabricator to store multiple identifiers per external account.
Test Plan: Storage only, see followup changes.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21011
Summary: Ref T13395. Moves a small amount of remaining "libphutil/" code into "phabricator/" and stops us from loading "libphutil/".
Test Plan: Browsed around; there are likely remaining issues.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20981
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 T13482. Although this style makes physical sense by relationship to a written checklist, it seems to do more harm than good in practice.
Test Plan: Wrote a checklist with a checked-off item in remarkup, saw no more line-through.
Maniphest Tasks: T13482
Differential Revision: https://secure.phabricator.com/D20954
Summary: Fixes T13476. Policy tags in object headers and "Visible To" controls in some dialog contexts may stack and wrap oddly. Improve spacing so they don't overlap visually when wrapping.
Test Plan: Viewed affected interfaces in narrow and wide windows.
Maniphest Tasks: T13476
Differential Revision: https://secure.phabricator.com/D20944
Summary:
Ref T13444. To interact meaningfully with "DestructionEngine", objects need a PHID. The "UserEmail" object currently does not have one (or a real "Query").
Provide basic PHID support so "DestructionEngine" can interact with the object more powerfully.
Test Plan:
- Ran migrations, checked data in database, saw sensible PHIDs assigned.
- Added a new email address to my account, saw it get a PHID.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13444
Differential Revision: https://secure.phabricator.com/D20913
Summary:
Ref T13444. You can currently explicitly unassign an identity (useful if the matching algorithm is misfiring). However, this populates the "currentEffectiveUserPHID" with the "unassigned()" token, which mostly makes things more difficult.
When an identity is explicitly unassigned, convert that into an explicit `null` in the effective user PHID.
Then, realign "assigned" / "effective" language a bit. Previously, `withAssigneePHIDs(...)` actualy queried effective users, which was misleading. Finally, bulk up the list view a little bit to make testing slightly easier.
Test Plan:
- Unassigned an identity, ran migration, saw `currentEffectiveUserPHID` become `NULL` for the identity.
- Unassigned a fresh identity, saw NULL.
- Queried for various identities under the modified constraints.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13444
Differential Revision: https://secure.phabricator.com/D20908
Summary:
Ref T13444. Currently, identities for a particular email address are queried with "LIKE" against a binary column, which makes the query case-sensitive.
- Extract the email address into a separate "sort255" column.
- Add a key for it.
- Make the query a standard "IN (%Ls)" query.
- Deal with weird cases where an email address is 10000 bytes long or full of binary junk.
Test Plan:
- Ran migration, inspected database for general sanity.
- Ran query script in T13444, saw it return the same hits for "git@" and "GIT@".
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13444
Differential Revision: https://secure.phabricator.com/D20907
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 T13440. Give the table more obvious visual structure and get rid of the largely useless header columns.
Test Plan: Viewed table, saw a slightly cleaner result.
Maniphest Tasks: T13440
Differential Revision: https://secure.phabricator.com/D20885
Summary: Depends on D20883. Ref T13440. In most cases, all changes belong to the same repository, which makes the "Repository" column redundant and visually noisy. Show repository information in a section header.
Test Plan: {F6989932}
Maniphest Tasks: T13440
Differential Revision: https://secure.phabricator.com/D20884
Summary:
Fixes T13437. This URI construction was just missing URI encoding.
Also, trim the symbol because my test case ended up catching "#define\n" as symbol text.
Test Plan:
- Configured a repository to have PHP symbols.
- Touched a ".php" file with "#define" in it.
- Diffed the change.
- Command-clicked "#define" in the UI, in Safari/MacOS, to jump to the definition.
- Before: taken to a nonsense page where "#define" became an anchor.
- After: taken to symbol search for "#define".
Maniphest Tasks: T13437
Differential Revision: https://secure.phabricator.com/D20876
Summary:
Fixes T13435. If you move Phabricator or copy data from one environment to another, the repository URI index currently still references the old URI, since it writes the URI as a plain string. This may make "arc which" and similar workflows have difficulty identifying repositories.
Instead, store the "phabricator.base-uri" domain and the "diffusion.ssh-host" domain as tokens, so lookups continue to work correctly even after these values change.
Test Plan:
- Added unit tests to cover the normalization.
- Ran migration, ran daemons, inspected `repository_uriindex` table, saw a mixture of sensible tokens (for local domains) and static domains (like "github.com").
- Ran this thing:
```
$ echo '{"remoteURIs": ["ssh://git@local.phacility.com/diffusion/P"]}' | ./bin/conduit call --method repository.query --trace --input -
Reading input from stdin...
>>> [2] (+0) <conduit> repository.query()
>>> [3] (+3) <connect> local_repository
<<< [3] (+3) <connect> 555 us
>>> [4] (+5) <query> SELECT `r`.* FROM `repository` `r` LEFT JOIN `local_repository`.`repository_uriindex` uri ON r.phid = uri.repositoryPHID WHERE (uri.repositoryURI IN ('<base-uri>/diffusion/P')) GROUP BY `r`.phid ORDER BY `r`.`id` DESC LIMIT 101
<<< [4] (+5) <query> 596 us
<<< [2] (+6) <conduit> 6,108 us
{
"result": [
{
"id": "1",
"name": "Phabricator",
"phid": "PHID-REPO-2psrynlauicce7d3q7g2",
"callsign": "P",
"monogram": "rP",
"vcs": "git",
"uri": "http://local.phacility.com/source/phabricator/",
"remoteURI": "https://github.com/phacility/phabricator.git",
"description": "asdf",
"isActive": true,
"isHosted": false,
"isImporting": false,
"encoding": "UTF-8",
"staging": {
"supported": true,
"prefix": "phabricator",
"uri": null
}
}
]
}
```
Note the `WHERE` clause in the query normalizes the URI into "<base-uri>", and the lookup succeeds.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13435
Differential Revision: https://secure.phabricator.com/D20872
Summary:
Fixes T13433. Currently, "Login Screen Instructions" in "Auth" are shown only on the main login screen. If you enter a bad password or bad LDAP credential set and move to the flow-specific login failure screen (for example, "invalid password"), the instructions vanish.
Instead, persist them. There are reasonable cases where this is highly useful and the cases which spring to mind where this is possibly misleading are fairly easy to fix by making the instructions more specific.
Test Plan:
- Configured login instructions in "Auth".
- Viewed main login screen, saw instructions.
- Entered a bad username/password and a bad LDAP credential set, got kicked to workflow sub-pages and still saw instructions (previously: no instructions).
- Grepped for other callers to `buildProviderPageResponse()` to look for anything weird, came up empty.
Maniphest Tasks: T13433
Differential Revision: https://secure.phabricator.com/D20863
Summary: Ref T13425. When we render a diff between two source lines, highlight intraline changes.
Test Plan: Viewed a Jupyter notebook with a code diff in it, saw the changed subsequences in the line highlighted.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20851
Summary:
Depends on D20843. Ref T13425. Add very basic support for "Show Hidden Context", in the form of folding it behind an unclickable shield. This isn't ideal, but should be better than nothing.
Prepare for "intraline" diffs on content blocks.
Fix newline handling in Markdown sections in Jupyter notebooks.
Remove the word "visibile" from the codebase.
Test Plan: {F6898192}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20844
Summary:
Ref T13425. Currently, code inputs and all outputs are grouped into a single block. This is fine for display notebooks but not great for diffing notebooks.
Instead, split source code input into individual lines with one line per block, and each output into its own block.
This allows you to leave actual line-by-line inlines on source, and comment on outputs individually.
Test Plan: {F6888583}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20840
Summary:
Depends on D20835. Ref T13425. Ref T13414. When a document has a list of content blocks, we may not be able to diff it directly, but we can hash each block and then diff the hashes (internally "diff" also does approximately the same thing).
We could do this ourselves with slightly fewer layers of indirection, but: diff already exists; we already use it; we already have a bunch of abstractions on top of it; and it's likely much faster on large inputs than the best we can do in PHP.
Test Plan: {F6888169}
Maniphest Tasks: T13425, T13414
Differential Revision: https://secure.phabricator.com/D20836
Summary:
Depends on D20834. Ref T13425. After the change from "th" to "td" for accessibility, the algorithm picks which cells it should highlight slightly improperly (it picks too many cells since it can no longer find the line numbers).
Ideally, it would probably highlight //only// the source content, but there isn't an easy way to do this right now. Settle for an incremental improvement for the moment.
Test Plan: Hovered over line numbers, saw a more accurate highlight area.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20835
Summary:
Depends on D20832. Ref T13425. Emit Jupyter notebooks as diffable blocks with block keys.
No diffing or proper inlines yet.
Test Plan: {F6888058}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20833
Summary:
Depends on D20831. Ref T13425. As an escape hatch to get out of future DocumentEngine rendering behavior, provide a "View As.." option.
Now I can break DocumentEngine real bad and no one can complain.
Test Plan: Used "View As" to swap document engines for image files.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20832
Summary:
Depends on D20830. Ref T13425. Have the image engine elect into block rendering, then emit blocks.
This is rough (the blocks aren't actually diffed yet) but image diffs were already pretty rough so this is approximately a net improvement.
Test Plan: Viewed image diffs, saw nothing worse than before.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20831
Summary:
Fixes T13284. See that task for substantial discussion. There are currently two cases where we'll skip over commits which we should publish:
- if a branch is not permanent, then later made permanent; or
- in some cases, the first time we examine branches in a repository.
In both cases, this error is one-shot and things work correctly going forward. The root cause is conflation between the states "this ref currently permanent" and "this ref was permanent the last time we updated refs".
Separate these pieces of state and cover all these cases. Also introduce a "--rebuild" flag to fix the state of bad commits.
Test Plan:
See T13284 for the three major cases:
- initial import;
- push changes to a nonpermanent branch, update, then make it permanent;
- push chanegs to a nonpermanent branch, update, push more changes, then make it permanent.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13284
Differential Revision: https://secure.phabricator.com/D20829
Summary:
Ref T13410. We currently generate some less-than-ideal anchors in remarkup, but it's hard to change the algorithm without breaking stuff.
To mitigate this, allow `#xyz` to match any target on the page which begins with `xyz`. This means we can make anchors longer with no damage, and savvy users are free to shorten anchors to produce more presentation-friendly links.
Test Plan: Browsed to `#header-th`, was scrolled to `#header-three`, etc.
Maniphest Tasks: T13410
Differential Revision: https://secure.phabricator.com/D20820
Summary: Ref T13279. Fix some tabular stuff, draw areas better, make the "compose()" API more consistent, unfatal the demo chart, unfatal the project burndown, make the project chart do something roughly physical.
Test Plan: Looked at charts, saw fewer obvious horrors.
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20817
Summary: Ref T13279. We currently draw a point on the chart for each datapoint, but this leads to many overlapping circles. Instead, aggregate the raw points into display points ("events") at the end.
Test Plan: Viewed a stacked area chart with many points, saw a more palatable number of drawn dots.
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20814
Summary:
Ref T13413. In Chrome 77, workboard cards with titles that must break in the middle of words cause the browser to completely lock up.
Work around the major known instance of this by overriding the "break-word" behavior. This gives us worse rendering for tasks with very long "words" in their titles (they are truncated instead of broken) but fixes the freezing.
Once Chrome is fixed, this can be reverted.
Test Plan:
- Created a task named "MMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMM" on a workboard.
- Loaded the board in Chrome 77.
- Before: entire page locks up.
- After: smooth sailing, except the "MMMMMM..." is truncated.
Maniphest Tasks: T13413
Differential Revision: https://secure.phabricator.com/D20812
Summary:
Fixes T7961. Currently, we present Herald users with actions like "Require legalpad signatures" and "Run build plans" even if Legalpad and Harbormaster are not installed.
Instead, allow fields and actions to be made "unavailable", which means that we won't present them as options when adding to new or existing rules.
If you edit a rule which already uses one of these fields or actions, it isn't affected.
Test Plan:
- Created a rule with a legalpad action, uninstalled legalpad, edited the rule. Action remained untouched.
- Created a new rule, wasn't offered the legalpad action.
- Reinstalled the application, saw the action again.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T7961
Differential Revision: https://secure.phabricator.com/D20808
Summary: Ref T13411. When users click a link to explain a capability (like the policy header on many objects, or the link next to specific capabilities in "Applications", "Diffusion", etc), inline the full ruleset for the custom policy into the dialog if the object has a custom policy.
Test Plan: {F6856365}
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20805
Summary:
Fixes T13408. Currently, when a package (or other object) appears in a field (rather than an action), it is not indexed.
Instead: index fields too, not just actions.
Test Plan:
- Wrote a rule like "[ Affected packages include ] ...".
- Updated the search index.
- Saw rule appear on "Affected By Herald Rules" on the package detail page.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13408
Differential Revision: https://secure.phabricator.com/D20795
Summary:
Depends on D20732. Ref T13366. This generally makes the "Merchant" UI look and work like the "Payment Account" UI.
This is mostly simpler since the permissions have largely been sorted out already and there's less going on here and less weirdness around view/edit policies.
Test Plan: Browsed all Merchant functions as a merchant member and non-member.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20733
Summary:
Depends on D20720. Ref T13366.
- Use modern policies and policy interfaces.
- Use new merchant authority cache.
- Add (some) transactions.
- Move MFA from pre-upgrade-gate to post-one-shot-check.
- Simplify the autopay workflow.
- Use the "reloading arrows" icon for subscriptions more consistently.
Test Plan: As a merchant-authority and account-authority, viewed, edited, and changed autopay for subscriptions.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20721
Summary:
Depends on D20718. Ref T13366. Ref T13367.
- Phortune payment methods currently do not use transactions; update them.
- Give them a proper view page with a transaction log.
- Add an "Add Payment Method" button which always works.
- Show which subscriptions a payment method is associated with.
- Get rid of the "Active" status indicator since we now treat "disabled" as "removed", to align with user expectation/intent.
- Swap out of some of the super weird div-form-button UI into the new "big, clickable" UI for choice dialogs among a small number of options on a single dimension.
Test Plan:
- As a mechant-authority and account-authority, created payment methods from carts, subscriptions, and accounts. Edited and viewed payment methods.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13367, T13366
Differential Revision: https://secure.phabricator.com/D20719
Summary:
Depends on D20713. Ref T13366. When a payment account establishes a relationship with a merchant by creating a cart or subscription, create an edge to give the merchant access to view the payment account.
Also, migrate all existing subscriptions and carts to write these edges.
This aims at straightening out Phortune permissions, which are currently a bit wonky on a couple of dimensions. See T13366 for detailed discussion.
Test Plan:
- Created and edited carts/subscriptions, saw edges write.
- Ran migrations, saw edges write.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20715
Summary: Depends on D20697. Ref T8389. Add support for adding "billing@enterprise.com" and similar to Phortune accounts.
Test Plan: Added and edited email addresses for a payment account.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T8389
Differential Revision: https://secure.phabricator.com/D20713
Summary:
Fixes T8830. Fixes T13364.
- The inability to destroy objects from the web UI is intentional. Make this clear in the messaging, which is somewhat out of date and partly reflects an earlier era when things could be destroyed.
- `bin/remove destroy` can't rewind time. Document expectations around the "put the cat back in the bag" use case.
Test Plan: Read documentation, clicked through both workflows.
Maniphest Tasks: T13364, T8830
Differential Revision: https://secure.phabricator.com/D20694
Summary:
Fixes T13363. Currently, these are genuine links which we intercept events for.
Make them pseudolinks instead. Possible alternative approaches are:
- Keep them as genuine links, but mark them as non-navigation links for Quicksand. (But: yuck, weird special case.)
- Keep them as genuine links, and have the dialog handler `JX.Stratcom.pass()` to see if anything handles the event. (But: the "pass()" pattern generally feels bad.)
"Tableaus" or whatever comes out of T10469 some day will probably break everything anyway?
Test Plan:
- Opened the "Edit Related Tasks... > Edit Subtasks" dialog.
- Clicked task title links (not the "open in new window" icon, and not the "Select" button).
- Before: Dialog (sometimes) closed abruptly.
- After: Task is consistently selected as part of the attachment set.
Maniphest Tasks: T13363
Differential Revision: https://secure.phabricator.com/D20693
Summary:
Ref T4900. When a card is edited, we currently emit an update notification for all the projects the task is tagged with. This isn't quite the right set:
- We want to emit notifications for projects the task //was previously// tagged with, so it can be removed from boards it should no longer be part of.
- We want to emit notifications for ancestors of projects the task is or was tagged with, so parent project boards can be updated.
- However, we don't need to emit notifications for projects that don't actually have workboards.
Adjust the notification set to align better to these rules.
Test Plan:
- Removal of Parent Project: Edited a task on board "A > B", removing the "B" project tag. Saw board A update in another window.
- Normal Update: Edited a task title on board X, saw board X update in another window.
- Used `bin/aphlict debug` to inspect the notification set, saw generally sensible-seeming data going over the wire.
Reviewers: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20680
Summary:
See D20650. Long ago, this got added as "pastebin", but that's the name of another product/company, not a generic term for paste storage.
Rename the database to `phabricator_paste`.
(An alternate version of this patch would rename `phabricator_search` to `phabricator_bing`, `phabricator_countdown` to `phabricator_spacex`, `phabricator_pholio` to `phabricator_adobe_photoshop`, etc.)
Test Plan:
- Grepped for `pastebin`, now only found references in old patches.
- Applied patches.
- Browsed around Paste in the UI without encountering issues.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20661
Summary:
Depends on D20654. Ref T4900. When a task is edited, emit a "workboards" event for all boards it appears on (in a future change, this should also include all boards it //previously// appeared on, and all parents of both sets of boards -- but I'm just getting things working for now).
When we receive a "workboards" event, check if the visible board should be updated.
Aphlict has a complicated intra-window leader/follower election system which could let us process this update event exactly once no matter how many windows a user has open with the same workboard. I'm not trying to do any of this since it seems fairly rare. It makes sense for events like "you have new notifications" where we don't want to generate 100 Ajax calls if the user has 100 windows open, but very few users seem likely to have 100 copies of the same workboard open.
Test Plan:
- Ran `bin/aphlict debug`.
- Opened workboard A in two windows, X and Y.
- Edited and moved tasks in window X.
- Saw "workboards" messages in the Aphlict log.
- Saw window Y update in nearly-real-time (locally, this is fast enough that it feels instantaneous).
Then:
- Stopped the Aphlcit server.
- Edited a task.
- Started the Aphlict server.
- Saw window Y update after a few moments (i.e., update in response to a reconnect).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20656
Summary:
Depends on D20653. Ref T4900. Pass ordering details to the reload endpoint so it can give the client accurate ordering/header information in the response.
The removed comment mentions this, but here's why this is a difficult mess:
- In window A, view a board with "Group by: Owner" and no tasks owned by "Alice". Since "Alice" owns no tasks, this means the columns do not have an "Assigned to: Alice" header!
- In window B, edit task T and assign it to Alice.
- In window A, press "R".
Window A now not only needs to update to properly reflect the state of task T, it actually needs to draw a new "Assigned to: Alice" header in every column.
Fortunately, the "group by" code anticipates this being a big mess, is fairly careful about handling it, and the client can handle this state change and the actual code change here isn't too involved. This is just causing a lot of not-very-obvious indirect effects in the pipeline to handle these situations that need complex redraws.
Test Plan:
- After making various normal edits/creates/moves in window A, pressed "R" in window B. Saw ordering reflected correctly after sync.
- Went through the whole "Group by: Owner" + assign to unrepresented owner flow above. After pressing "R", saw "Assigned to: Alice" appear on the board.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20654
Summary:
Depends on D20652. Ref T4900. When the user presses "R", send a list of cards currently visible on the client and their version numbers.
On the server:
- Compare the client verisons to the server versions so we can skip updates for objects which have not changed. (For now, the client version is always "1" and the server version is always "2", so this doesn't do anything meaningful, and every card is always updated.)
- Compare the client visible set to the server visible set and "remove" any cards which have been removed from the board.
I believe this means that "R" always puts the board into the right state (except for some issues with client orderings not being fully handled yet). It's not tremendously efficient, but we can make versioning better (using the largest object transaction ID) to improve that and loading the page in the first place doesn't take all that long so even sending down the full visible set shouldn't be a huge problem.
Test Plan:
- In window A, removed a card from a board.
- In window B, pressed "R" and saw the removal reflected on the client.
- (Also added cards, edited cards, etc., and didn't catch anything exploding.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20653
Summary:
Depends on D20639. Ref T4900. Currently, "BoardResponseEngine" has a `setObjectPHID()` method. This is called after edit operations to mean "we just edited object X, so we know it needs to be updated".
Move toward `setUpdatePHIDs(...)` in all cases, with `setUpdatePHIDs(array(the-object-we-just-edited))` as a special case of that. After this change, callers pass:
- An optional list of PHIDs they know need to be updated on the client. Today, this is always be a card we just edited (on edit/move flows), or a sort of made-up list of PHIDs for the moment (when you press "R"). In the future, the "R" endpoint will do a better job of figuring out a more realistic update set.
- An optional list of PHIDs currently visible on the client. This is used to update ordering details and mark cards for removal. This is currently passed by edit/move, but not by pressing "R" (it will be in the future).
- An optional list of objects. The "R" workflow has to load these anyway, so we can save a couple queries by letting callers pass them. For now, the edit/move flows still rely on the engine to figure out what it needs to load.
This does very little to actually change client behavior, it mostly just paves the way for the next update to the "R" workflow to make it handle add/remove cases properly.
Test Plan:
- Edited and moved cards on a workboard.
- Pressed "R" to reload a workboard.
Neither of these operations seem any worse off than they were before. They still don't fully work:
- When you edit a card and delete the current workboard project from it, it remains visible. This is also the behavior on `master`. This is sort of intentional since we don't necessarily want to make these cards suddenly disappear? Ideally, we would probably have some kind of "tombstone" state where the card can still be edited but can't be dragged, and the next explicit user interaction would clean up old tombstones. This interaction is very rare and I don't think it's particularly important to specialize.
- When a card is removed from the board, "R" can't currently figure out that it should be removed from the client. This is because the client does not yet pass a "visiblePHIDs" state. It will in an upcoming change.
- The "R" flow always sends a full set of card updates, and can not yet detect that some cards have not changed.
- There's a TODO, but some ordering stuff isn't handled yet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20652
Summary:
Depends on D20638. Ref T4900. This is an incremental step toward proper workboard updates.
Currently, the client can mostly update its view because we do updates when you edit or move a card, and the client and server know how to send lists of card updates, so a lot of the work is already done.
However, the code assumes we're only updating/redrawing one card at a time. Make the client accept and process multiple card updates.
In future changes, I'll add versioning (so we only update cards that have actually changed), fix the "TODO" around ordering, and move toward actual Aphlict-based real-time updates.
Test Plan:
- Opened the same workboard in two windows.
- Edited cards in one window, pressed "R" (capital letter, with no modifier keys) to reload the second window.
- Saw edits and moves reflected accurately after sync, except for some special cases of header/order interaction (see "TODO").
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20639
Summary:
Ref T13302. The "Close/Cancel" button is currently running two copies of the "dismiss dialog" code, since it's techncally a link with a valid HREF attribute.
An alternate formulation of this is perhaps `if (JX.Stratcom.pass()) { return; }` ("let other handlers react to this event; if something kills it, stop processing"), but `pass()` is inherently someone spooky/fragile so try to get away without it.
Test Plan: Opened the Javascript console, clicked "Edit Task" on a workboard, clicked "Close" on the dialog. Before: event was double-handled leading to a JS error in the console. After: dialog closes uneventfully.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13302
Differential Revision: https://secure.phabricator.com/D20640
Summary: Depends on D20637. Ref T4900. This is some ancient dead code that nothing uses.
Test Plan: Grepped for `updateCard()` to verify it's private. Searched for "options" and "dirtyColumn" and got no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20638
