1. 12 Sep, 2018 11 commits
    • epriestley's avatar
      Add a Phriction hint when a draft exists, and fix some draft editing bugs · b6ba75d9
      epriestley authored
      Summary:
      Depends on D19662. Ref T13077. See PHI840.
      
        - If you're looking at the published version of a document, but a draft version exists and you can edit it, add a hint/link.
        - Fix an issue where the "draft" transaction would complain when you created a document since the initial content is empty and no "draft" transaction is adding any content.
      
      Test Plan: Created new documents, viewed documents with current published versions and unpublished drafts.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13077
      
      Differential Revision: https://secure.phabricator.com/D19663
      b6ba75d9
    • epriestley's avatar
      Allow Phriction document edits to be saved as drafts · 550028a8
      epriestley authored
      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
      550028a8
    • epriestley's avatar
      Remove obsolete, nonfunctional draft auto-saving in Phriction · 152e7713
      epriestley authored
      Summary:
      Depends on D19660. Ref T5811. Ref T13077.
      
      Long ago, if you started editing a Phriction document but didn't save it, we'd save the draft in the background as part of the preview.
      
      D11169 updated the preview to use shared infrastructure and broke this function, since we never save drafts.
      
      Since this doesn't work right now, I want to add another thing called "draft", and the future of this feature should be more integrated with modern drafts and EditEngine (which fixed some bugs related to versioning), just get rid of this code for the moment.
      
      Test Plan: Edited documents. This code doesn't do anything since D11169, so no behavior changed.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13077, T5811
      
      Differential Revision: https://secure.phabricator.com/D19661
      152e7713
    • epriestley's avatar
      Support (basic) commenting on Phriction documents · e19c5559
      epriestley authored
      Summary:
      Depends on D19659. Fixes T1894. Ref T13077. See PHI840.
      
        - Add an EditEngine, although it currently supports no fields.
        - Add (basic, top-level-only) commenting (we already had the table in the database).
      
      This will probably create some issues. I'm most concerned about documents accumulating a ton of old, irrelevant comments over time which are hard to keep track of and no longer relevant. But I think this is probably a step forward in almost all cases, and a good thing on the balance.
      
      This also moves us incrementally toward putting all editing on top of EditEngine.
      
      Test Plan: {F5877347}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13077, T1894
      
      Differential Revision: https://secure.phabricator.com/D19660
      e19c5559
    • epriestley's avatar
      When a user takes actions while in a high security session, note it on the resulting transactions · f5e90a36
      epriestley authored
      Summary:
      Ref T13197. See PHI873. Record when a user has MFA'd and add a little icon to the transaction, similar to the exiting "Silent" icon.
      
      For now, this just makes this stuff more auditable. Future changes may add ways to require MFA for certain specific transactions, outside of the ones that already always require MFA (like revealing credentials).
      
      Test Plan: {F5877960}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13197
      
      Differential Revision: https://secure.phabricator.com/D19665
      f5e90a36
    • epriestley's avatar
      Enrich "diffusion.commit.search" with identity, status, and message information · 8268abcb
      epriestley authored
      Summary:
      Depends on D19657. Ref T13197. See PHI841.
      
      This enriches the results from `diffusion.commit.search` with information similar to the information returned by the "commits" attachment from `differential.diff.search`.
      
      Also include unreachable, imported, message, audit status, and repository PHID.
      
      Test Plan: Called `diffusion.commit.search` and reviewed the results, which looked sensible.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13197
      
      Differential Revision: https://secure.phabricator.com/D19658
      8268abcb
    • epriestley's avatar
      Rename "PhabricatorAuditCommitStatusConstants" to "DiffusionCommitAuditStatus"; remove "MODERN_" · c7e7b63f
      epriestley authored
      Summary:
      Depends on D19656. Ref T13197. See PHI851.
      
        - This class is now a real object, so get rid of the "Constants" part of the name.
        - Rename it for greater consistency with other modern objects.
        - Get rid of the `MODERN_` tag now that the old constants are gone.
      
      Test Plan: Bunch of `grep`, browsed around.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13197
      
      Differential Revision: https://secure.phabricator.com/D19657
      c7e7b63f
    • epriestley's avatar
      Remove legacy numeric Audit status constants · aaf22695
      epriestley authored
      Summary: Depends on D19655. Ref T13197. These no longer have callers.
      
      Test Plan: Grepped for these constants.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13197
      
      Differential Revision: https://secure.phabricator.com/D19656
      aaf22695
    • epriestley's avatar
      Migrate remaining Audit database status constants · d63281cc
      epriestley authored
      Summary: Depends on D19652. Ref T13197. See PHI851. This migrates the actual `auditStatus` on Commits, and older status transactions.
      
      Test Plan:
        - Ran migrations.
        - Spot-checked the database for sanity.
        - Ran some different queries, got unchanged results from before migration.
        - Reviewed historic audit state transactions, and accepted/raised concern on new audits. All state transactions appeared to generate properly.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      
      Maniphest Tasks: T13197
      
      Differential Revision: https://secure.phabricator.com/D19655
      d63281cc
    • epriestley's avatar
      Migrate old commit saved queries to new audit status constants · 09703938
      epriestley authored
      Summary: Depends on D19651. Ref T13197. The application now accepts either numeric or string values. However, for consistency and to reduce surprise in the future, migrate existing saved queries to use string values.
      
      Test Plan: Saved some queries on `master` with numeric constants, ran the migration, saw string constants in the database and equivalent behavior in the UI.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      
      Maniphest Tasks: T13197
      
      Differential Revision: https://secure.phabricator.com/D19652
      09703938
    • epriestley's avatar
      Fix a "withHasTransactions()" typo in AuditEditor · cc3b6d57
      epriestley authored
      Summary: See <https://discourse.phabricator-community.org/t/typo-in-phabricatorauditeditor-php/1910>. This is trivial and reproduces easily, I just missed it in testing.
      
      Test Plan:
        - Left a comment on a commit which I was the author of.
        - Before change: fatal with obvious typo.
        - After change: smooth sailing.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D19667
      cc3b6d57
  2. 11 Sep, 2018 2 commits
    • epriestley's avatar
      Give Phriction documents a normal timeline · 2379c21f
      epriestley authored
      Summary:
      Ref T13077. See PHI840. Ref T1894. I'm planning to just let you comment on Phriction documents. I think this will create a few problems (e.g., around popular documents which collect long comment threads that are eventually obsolete) but nothing should be too terribly critical (e.g., we handle it gracefully when objects have very large number of comments/transactions) and for most documents this is likely just a net improvement.
      
      "Just enable comments" is probably not the final iteration on this, but I think it's probably a step forward on the balance, not a step sideways or a slippery slope down into a dark hole or anything.
      
      Test Plan: {F5877316}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13077, T1894
      
      Differential Revision: https://secure.phabricator.com/D19659
      2379c21f
    • epriestley's avatar
      Add aural section headers for "Event Timeline", "Add Comment", and "Comment Preview" · 185e72c8
      epriestley authored
      Summary: See PHI871. Ref T13197. These sections are only divided visually and don't have textual headers. Add aural headers.
      
      Test Plan: {F5875471}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13197
      
      Differential Revision: https://secure.phabricator.com/D19654
      185e72c8
  3. 10 Sep, 2018 4 commits
  4. 07 Sep, 2018 5 commits
    • epriestley's avatar
      When authors add inlines to their own revisions/commits, mark them as "Done" by default · 2a03367a
      epriestley authored
      Summary:
      Ref T13195. Fixes T8573. When you're adding inlines to your own stuff, mark them "Done" by default. You can unmark them as "Done" if you're legitimately leaving TODOs for yourself, although I think this is unusual.
      
      (If this turns out to be less unusual than I think, we could consider an alternate rule: mark replies by the author as "Done" by default.)
      
      Test Plan: Added some inlines as an author and a non-author. Saw my author inlines marked as "Done" by default. Submitted them; unmarked and submittted them.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13195, T8573
      
      Differential Revision: https://secure.phabricator.com/D19635
      2a03367a
    • epriestley's avatar
      Allow reviewers to mark their own inlines as "Done" before they submit them · 16a6fc83
      epriestley authored
      Summary:
      Ref T13195. Ref T8573. This allows reviewers to mark their own inline comments as "Done" before they submit them.
      
      If you're leaving a non-actionable comment like "this is good", you can pre-check "Done" to give the author a hint that you don't expect any response.
      
      Test Plan: On revisions and commits, added inlines as the author and a reviewer/auditor. Marked them done/not-done before submitting. As author, marked the not-done ones done after submitting. Checked preivews, toggled done/not done states.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13195, T8573
      
      Differential Revision: https://secure.phabricator.com/D19634
      16a6fc83
    • epriestley's avatar
      Expose "isDraft" and "holdAsDraft" revision properties over Conduit · 046c1b5b
      epriestley authored
      Summary:
      Ref T13195. See PHI861. The "+ Draft" flag is not currently exposed over the API, but seems stable enough to expose.
      
      Also expose the "hold as draft" flag, normally `arc diff --draft`.
      
      Today, you can get "+ Draft" with some other state by:
      
        - abandoning a draft revision ("Abandoned + Draft"); or
        - using `arc diff --plan-changes` with an older `arc` version ("Changes Planned + Draft").
      
      Test Plan: Called `differential.revision.search` via Conduit and got sensible-looking results for revisions in various states.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13195
      
      Differential Revision: https://secure.phabricator.com/D19648
      046c1b5b
    • epriestley's avatar
      Introduce an AuditStatus object for commits and move some callsites to it · a1ce23b9
      epriestley authored
      Summary:
      Ref T13195. See PHI851. Add an object, analogous to the `DifferentialRevisionStatus` object, to handle audit status management.
      
      This will primarily make it easier to swap storage over to strings later, but also cleans things up a bit.
      
      Test Plan: Viewed audit/commit lists, saw sensible state icons. Ran `bin/audit synchronize`, got sensible output.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13195
      
      Differential Revision: https://secure.phabricator.com/D19646
      a1ce23b9
    • epriestley's avatar
      Allow underscores to appear in Harbormaster variable names · ab4d33be
      epriestley authored
      Summary:
      See PHI859. Ref T13195. The regexp for replacing variables currently does not include underscores.
      
      Include underscores.
      
      I also made a note in T13088: we should (almost certainly?) throw immediately if you try to pass a bogus variable name as a custom parameter, but this is a slightly larger change.
      
      Test Plan:
        - Made an "http request" build plan with `?x=${initiator.phid}&y={$some_variable}`.
        - Added `some_variable` as a parameter to the parameter collection.
        - Before patch: `initiator.phid` was replaced, but `some_variable` was not.
        - After patch: both variables are replaced.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      
      Maniphest Tasks: T13195
      
      Differential Revision: https://secure.phabricator.com/D19645
      ab4d33be
  5. 06 Sep, 2018 8 commits
    • epriestley's avatar
      Make a language improvement ("inlines" -> "inline comments") · e8e5dc0f
      epriestley authored
      Summary: See D19632. Agreed that this is more clear.
      
      Test Plan: Read carefully.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Differential Revision: https://secure.phabricator.com/D19644
      e8e5dc0f
    • epriestley's avatar
      Begin transitioning audits to modern (string) status constants, from legacy... · ef26b06c
      epriestley authored
      Begin transitioning audits to modern (string) status constants, from legacy (integer) status constants
      
      Summary:
      Ref T13195. See PHI851. Audits currently have older integer status constants. We've moved almost all object types away from this to string constants (which are better in basically every way, and particularly way better for exposing over the API).
      
      Commits/audits are currently accessible over the API and expose these constants via a "statuses" constraint.
      
      Prepare to move toward modern string constants by defining a new, more modern map of status details and defining the existing methods in terms of it.
      
      Test Plan: Browsed audits checking for icons/names/open-ness, saw no changes. This change should have no user-visible effects, as it just reorganizes code.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13195
      
      Differential Revision: https://secure.phabricator.com/D19642
      ef26b06c
    • epriestley's avatar
      In Conduit, let checkbox constraints self-document · 5a38b75f
      epriestley authored
      Summary:
      Ref T13195. Ref PHI851. Currently, checkbox constraints don't tell you what values are supported on the API page.
      
      You can figure this out with "Inspect Element" or by viewing the source code, but just render a nice table instead.
      
      Test Plan: {F5862969}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13195
      
      Differential Revision: https://secure.phabricator.com/D19641
      5a38b75f
    • epriestley's avatar
      Improve some documentation for "diffusion.commit.search" · a20f0674
      epriestley authored
      Summary:
      Ref T13195. See PHI851. Start by making some minor improvements here:
      
        - Clarify that the example of what constraints look like is just an example.
        - Add descriptions for parameters missing descriptions.
      
      Test Plan: Looked at API method page, saw more helpful/complete instructions.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13195
      
      Differential Revision: https://secure.phabricator.com/D19640
      a20f0674
    • epriestley's avatar
      Remove outdated "rebuild_summaries.php" script · 7967fab5
      epriestley authored
      Summary:
      Ref T13195. See PHI842. Alternative to D19638.
      
      Instead of doing all the stuff in D19638, //just// remove the `rebuild_summaries.php` script. This script is outdated, copy/pastes the rebuild logic, and doesn't understand unreachable commits.
      
      If we had some use for it it should move to `bin/repository rebuild-summary ...` or similar, but it's not clear there's any use for it. The incremental summary rebuilds seem to work fine as-is.
      
      Test Plan: Grepped for callers or documentation referencing this script, found nothing.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13195
      
      Differential Revision: https://secure.phabricator.com/D19643
      7967fab5
    • epriestley's avatar
      When applying repository operations via Drydock, provide more context on OperationType · e6ee5ee9
      epriestley authored
      Summary: Ref T13195. See PHI845. For custom OperationTypes, provide access to the Interface and Operation via getters. This is just for convenience, since passing these around everywhere can be a bit of a pain if you have a deep-ish stack of things or love using callbacks or whatever.
      
      Test Plan: Landed a revision via upstream Drydock operations.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      
      Maniphest Tasks: T13195
      
      Differential Revision: https://secure.phabricator.com/D19636
      e6ee5ee9
    • epriestley's avatar
      When a transaction adds more than 100 inline comments, include only the first 100 in email · 04139298
      epriestley authored
      Summary:
      Ref T13195. An install had a user apply a transaction which added about 1,000 inline comments. Rendering the email for this transaction took a very long time because the context section for each comment must be highlighted separately.
      
      We can make the highlighting faster (in this case, by porting the lexer to PHP) but it's also sort of silly to include more than 100 inlines in an email. These emails are likely to be truncated by outbound size rules at some point anyway. Instead, limit inlines rendered directly into email to the first 100 per transaction group.
      
      Test Plan:
      Set limit to 2, added 4 comments, viewed text and HTML emails:
      
      {F5859967}
      
      {F5859968}
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13195
      
      Differential Revision: https://secure.phabricator.com/D19632
      04139298
    • epriestley's avatar
      Update some inline comment logic to use more modern "Viewer"-oriented calls/variables · 650e7493
      epriestley authored
      Summary:
      Ref T13195. Ref T8573. The inline comment controllers currently use outdated `$user = $this->getRequest()->getUser()` calls.
      
      Instead, use `$viewer = $this->getViewer()`.
      
      This is just a small consistency update with no behavioral changes.
      
      Test Plan: Viewed and added inlines in Differential and Diffusion.
      
      Reviewers: amckinley
      
      Reviewed By: amckinley
      
      Maniphest Tasks: T13195, T8573
      
      Differential Revision: https://secure.phabricator.com/D19633
      650e7493
  6. 31 Aug, 2018 3 commits
  7. 30 Aug, 2018 3 commits
    • epriestley's avatar
      Make the Phriction "History" view more aware of drafts · 18e8d9a4
      epriestley authored
      Summary: Ref T13077. Updates the "History" view to be slightly better organized and draft-aware.
      
      Test Plan: Viewed page history in Phriction.
      
      Reviewers: amckinley
      
      Maniphest Tasks: T13077
      
      Differential Revision: https://secure.phabricator.com/D19626
      18e8d9a4
    • epriestley's avatar
      Store Phriction max version on Document, improve editing rules for editing documents with drafts · 3b1294cf
      epriestley authored
      Summary:
      Ref T13077. We need to know the maximum version of a document in several cases, so denormalize it onto the Document object.
      
      Then clean up some behaviors where we edit a document with, e.g., 7 versions but version 5 is currently published. For now, we: edit starting with version 7, save as version 8, and immediately publish the new version.
      
      Test Plan:
        - Ran migration.
        - Edited a draft page without hitting any weird version errors.
        - Checked database for sensible `maxVersion` values.
      
      Reviewers: amckinley
      
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      
      Maniphest Tasks: T13077
      
      Differential Revision: https://secure.phabricator.com/D19625
      3b1294cf
    • epriestley's avatar
      Work around an issue in MariaDB where dropping a column from a UNIQUE KEY fails · 0a77b0e5
      epriestley authored
      Summary:
      See T13193. See T13077. If we drop a column which is part of a UNIQUE KEY, MariaDB raises an error.
      
      This is probably a bad idea on our side anyway, but in this case it wasn't an obviously bad idea.
      
      To get around this:
      
        - Drop the unique key, if it exists, before dropping the column.
        - Explicitly add the new unique key afterward.
      
      Test Plan: Ran `bin/storage upgrade` locally without issue, but I'm on MySQL. Will follow up on T13193.
      
      Reviewers: amckinley
      
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      
      Differential Revision: https://secure.phabricator.com/D19624
      0a77b0e5
  8. 29 Aug, 2018 4 commits
    • epriestley's avatar
      Add "Revision test plan" as a Herald field; remove test plan from the "Revision summary" field · 75a04551
      epriestley authored
      Summary:
      See PHI844. Ref T13189.
      
      Add "Revision test plan" as an available field for Herald. This is a little niche -- and a little odd because it sticks around even if you fully disable test plans -- but probably broadly reasonable.
      
      The existing "Revision summary" field counterintuitively included the test plan. Separate this out since it's now a separate field and the behavior was weird historic nonsense. I'll note this in the changelog.
      
      Test Plan: Wrote a rule using both fields, verified they generated the expected values.
      
      Reviewers: amckinley
      
      Maniphest Tasks: T13189
      
      Differential Revision: https://secure.phabricator.com/D19623
      75a04551
    • epriestley's avatar
      Add a UI element for navigating between versions of a Phriction document · 876638e4
      epriestley authored
      Summary: Depends on D19621. Ref T13077. Fixes T4815. This adds previous/current/next/draft buttons and makes navigation between unpublished and published versions of a document more clear.
      
      Test Plan: {F5841997}
      
      Reviewers: amckinley
      
      Maniphest Tasks: T13077, T4815
      
      Differential Revision: https://secure.phabricator.com/D19622
      876638e4
    • epriestley's avatar
      Allow the published version of a Phriction document to differ from the most recent version · 34968631
      epriestley authored
      Summary:
      Depends on D19620. Ref T13077. This adds a "Publish" operation which points the current version at some historical version of the document -- not necessarily the most recent version. Newer versions become "drafts".
      
      This is still quite rough and missing a lot of hinting in the UI, I'm just making it work so I can start making the UI understand it.
      
      Test Plan: Used the "Publish" action to publish older versions of a document, saw the document revert. Many UI hints are missing and this operation is puzzling and not yet usable for normal users.
      
      Reviewers: amckinley
      
      Maniphest Tasks: T13077
      
      Differential Revision: https://secure.phabricator.com/D19621
      34968631
    • epriestley's avatar
      Remove on-object mailkeys from Phriction · 50f4adef
      epriestley authored
      Summary: Depends on D19619. Ref T13065. Ref T13077. Migrate Phriction mail keys to the new infrastructure and drop the column.
      
      Test Plan: Ran migrations, spot-checked the database.
      
      Reviewers: amckinley
      
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      
      Maniphest Tasks: T13077, T13065
      
      Differential Revision: https://secure.phabricator.com/D19620
      50f4adef