From 314e7266c3e5b071cb6e916d28c21cb143d6e013 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Nov 2017 12:06:24 -0800 Subject: [PATCH 1/6] Restore "Summary" and "Test Plan" to initial mail for non-draft configurations Summary: See PHI210. Ref T2543. Currently, we don't set this flag if you have prototypes off and don't get any of the new draft stuff, so the mail drops some of the details it is supposed to have. Test Plan: Disabled prototypes, created a revision, saw summary / test plan in the initial mail. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18771 --- .../differential/editor/DifferentialTransactionEditor.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 26c440e866..7cea29359f 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1612,6 +1612,13 @@ final class DifferentialTransactionEditor $xactions[] = $xaction; } + } else { + // If this revision is being created into some state other than "Draft", + // this is the first broadcast and should include sections like "SUMMARY" + // and "TEST PLAN". + if ($this->getIsNewObject()) { + $this->firstBroadcast = true; + } } return $xactions; From 95a5b41b0ba2751bccdec0725b84561ba7000d09 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Nov 2017 12:17:31 -0800 Subject: [PATCH 2/6] Allow "arc diff --draft" to create revisions as drafts more forcefully Summary: Depends on D18771. See PHI206. Currently, `arc diff --draft` only holds revisions in draft mode: it doesn't put them into draft mode if the install isn't configured to use draft mode. Instead, make it a bit more forceful so that `arc diff --draft` can create into draft mode explicitly even if protoypes are off. This aligns with expection a little more clearly. Test Plan: Ran `arc diff --draft` with prototypes off, got a revision held in draft mode. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18772 --- .../xaction/DifferentialRevisionHoldDraftTransaction.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/applications/differential/xaction/DifferentialRevisionHoldDraftTransaction.php b/src/applications/differential/xaction/DifferentialRevisionHoldDraftTransaction.php index 5bc257ab62..2562f18209 100644 --- a/src/applications/differential/xaction/DifferentialRevisionHoldDraftTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionHoldDraftTransaction.php @@ -16,6 +16,15 @@ final class DifferentialRevisionHoldDraftTransaction public function applyInternalEffects($object, $value) { $object->setHoldAsDraft($value); + + // If draft isn't the default state but we're creating a new revision + // and holding it as a draft, put it in draft mode. See PHI206. + // TODO: This can probably be removed once Draft is the universal default. + if ($this->isNewObject()) { + if ($object->isNeedsReview()) { + $object->setModernRevisionStatus(DifferentialRevisionStatus::DRAFT); + } + } } public function getTitle() { From a1f12b4ac7af59216b4a0feb1321542486b8be64 Mon Sep 17 00:00:00 2001 From: Mukunda Modell Date: Tue, 14 Nov 2017 17:03:16 -0600 Subject: [PATCH 3/6] Specify a null behavior for the callsign sort column. Summary: Fixes paging on the Diffusion Repository List. PhabricatorRepositoryQuery needs to specify a behavior for `null` on the OderableColumns definition for the `callsign` column. See https://phabricator.wikimedia.org/T180457 Test Plan: 1. On an instance with more than 100 repositories * some of which are missing a callsign 2. Attempt to sort by callsign. 3. See the sorted results Previously: 3. Exception: "Column "0" has null value, but does not specify a null behavior." Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D18773 --- src/applications/repository/query/PhabricatorRepositoryQuery.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index ffa9fa1b59..035693ff75 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php @@ -425,6 +425,7 @@ final class PhabricatorRepositoryQuery 'type' => 'string', 'unique' => true, 'reverse' => true, + 'null' => 'tail', ), 'name' => array( 'table' => 'r', From 3700bcb638def188008839683feecd7b68729173 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 15 Nov 2017 09:21:52 -0800 Subject: [PATCH 4/6] Warn and prevent 1-up/2-up switch in Differential if the user is editing an inline Summary: See PHI180. Currently, if you begin creating or editing an inline and then swap display modes (for example, with "View Unified"), your edit is lost. Persisting the editor state is complicated and this is very rare, so just prevent the action and warn the user instead. Also make the warning persist for a little longer since a few of the messages, including this one, take a couple seconds to read now. Test Plan: - Edited a comment, tried to swap display modes, got a warning. - Swapped display modes normally with no comment being edited. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18774 --- resources/celerity/map.php | 14 +++++++------- .../view/DifferentialChangesetListView.php | 3 +++ .../js/application/diff/DiffChangesetList.js | 19 ++++++++++++++++++- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 30c04ffc92..78ae8bd461 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => '4c79d74f', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', - 'differential.pkg.js' => 'ae6460e0', + 'differential.pkg.js' => '500a75c5', 'diffusion.pkg.css' => 'a2d17c7d', 'diffusion.pkg.js' => '6134c5a1', 'favicon.ico' => '30672e08', @@ -396,7 +396,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', 'rsrc/js/application/diff/DiffChangeset.js' => '99abf4cd', - 'rsrc/js/application/diff/DiffChangesetList.js' => '8f1cd52c', + 'rsrc/js/application/diff/DiffChangesetList.js' => '3b77efdd', 'rsrc/js/application/diff/DiffInline.js' => 'e83d28f3', 'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832', 'rsrc/js/application/differential/behavior-comment-preview.js' => '51c5ad07', @@ -775,7 +775,7 @@ return array( 'phabricator-darkmessage' => 'c48cccdd', 'phabricator-dashboard-css' => 'fe5b1869', 'phabricator-diff-changeset' => '99abf4cd', - 'phabricator-diff-changeset-list' => '8f1cd52c', + 'phabricator-diff-changeset-list' => '3b77efdd', 'phabricator-diff-inline' => 'e83d28f3', 'phabricator-drag-and-drop-file-upload' => '58dea2fa', 'phabricator-draggable-list' => 'bea6e7f4', @@ -1137,6 +1137,10 @@ return array( 'javelin-dom', 'javelin-magical-init', ), + '3b77efdd' => array( + 'javelin-install', + 'phuix-button-view', + ), '3cb0b2fc' => array( 'javelin-behavior', 'javelin-dom', @@ -1610,10 +1614,6 @@ return array( '8e1baf68' => array( 'phui-button-css', ), - '8f1cd52c' => array( - 'javelin-install', - 'phuix-button-view', - ), '8f29b364' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 307d424b0e..59f73b5465 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -301,6 +301,9 @@ final class DifferentialChangesetListView extends AphrontView { 'Hide or show all inline comments.' => pht('Hide or show all inline comments.'), + + 'Finish editing inline comments before changing display modes.' => + pht('Finish editing inline comments before changing display modes.'), ), )); diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 3f47f1ed35..ec0270ac12 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -465,7 +465,7 @@ JX.install('DiffChangesetList', { new JX.Notification() .setContent(message) .alterClassName('jx-notification-alert', true) - .setDuration(1000) + .setDuration(3000) .show(); }, @@ -691,6 +691,7 @@ JX.install('DiffChangesetList', { 'div', 'differential-changeset'); + var changeset_list = this; var changeset = this.getChangesetForNode(node); var menu = new JX.PHUIXDropdownMenu(button); @@ -738,6 +739,22 @@ JX.install('DiffChangesetList', { var up_item = new JX.PHUIXActionView() .setHandler(function(e) { if (changeset.isLoaded()) { + + // Don't let the user swap display modes if a comment is being + // edited, since they might lose their work. See PHI180. + var inlines = changeset.getInlines(); + for (var ii = 0; ii < inlines.length; ii++) { + if (inlines[ii].isEditing()) { + changeset_list._warnUser( + pht( + 'Finish editing inline comments before changing display ' + + 'modes.')); + e.prevent(); + menu.close(); + return; + } + } + var renderer = changeset.getRenderer(); if (renderer == '1up') { renderer = '2up'; From bea45e90d3240ac575f70d2ab46aef3d9c9b8634 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Wed, 15 Nov 2017 11:28:47 -0800 Subject: [PATCH 5/6] Add yaml files to differential.whitespace-matters Summary: Whitespace has semantic meaning for yaml files, so we shouldn't suppress whitespace-only lines of diff by default. Test Plan: Edited local config to include yaml files, saw expected whitespace changes. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D18775 --- .../differential/config/PhabricatorDifferentialConfigOptions.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php index 7ff886664f..0d00207b43 100644 --- a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php +++ b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php @@ -86,6 +86,7 @@ EOHELP array( '/\.py$/', '/\.l?hs$/', + '/\.ya?ml$/', )) ->setDescription( pht( From d2cff6a2cf01396f6337edfadd1f7df7cce1277d Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Thu, 16 Nov 2017 00:43:35 -0800 Subject: [PATCH 6/6] Transcode the HTML part of incoming email into UTF-8 as well Summary: D1093 did this for just the text/plain part of incoming email. Most text/html parts choose to either use entity encoding //or// are already UTF-8, thus obviating the need to transcode the HTML part. However, this is not always the case, and leads to dropped messages, by way of: ``` EXCEPTION: (Exception) Failed to JSON encode value (#5: Malformed UTF-8 characters, possibly incorrectly encoded): Dictionary value at key "html" is not valid UTF8, and cannot be JSON encoded: [snip HTML part of message content]``` Generalize the charset transcoding to not apply to just the text/plain part, but both text/plain and text/html parts. Test Plan: Fed in a Windows-1252-encoded text/html part with 0x92 bytes in it; verified that $content only contained valid UTF-8 after this change. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Differential Revision: https://secure.phabricator.com/D18776 --- scripts/mail/mail_handler.php | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/scripts/mail/mail_handler.php b/scripts/mail/mail_handler.php index 2ff23adb0f..b76b3910df 100755 --- a/scripts/mail/mail_handler.php +++ b/scripts/mail/mail_handler.php @@ -35,16 +35,19 @@ $args->parse( $parser = new MimeMailParser(); $parser->setText(file_get_contents('php://stdin')); -$text_body = $parser->getMessageBody('text'); - -$text_body_headers = $parser->getMessageBodyHeaders('text'); -$content_type = idx($text_body_headers, 'content-type'); -if ( - !phutil_is_utf8($text_body) && - (preg_match('/charset="(.*?)"/', $content_type, $matches) || - preg_match('/charset=(\S+)/', $content_type, $matches)) -) { - $text_body = phutil_utf8_convert($text_body, 'UTF-8', $matches[1]); +$content = array(); +foreach (array('text', 'html') as $part) { + $part_body = $parser->getMessageBody($part); + $part_headers = $parser->getMessageBodyHeaders($part); + $content_type = idx($part_headers, 'content-type'); + if ( + !phutil_is_utf8($part_body) && + (preg_match('/charset="(.*?)"/', $content_type, $matches) || + preg_match('/charset=(\S+)/', $content_type, $matches)) + ) { + $part_body = phutil_utf8_convert($part_body, 'UTF-8', $matches[1]); + } + $content[$part] = $part_body; } $headers = $parser->getHeaders(); @@ -57,10 +60,7 @@ if ($args->getArg('process-duplicates')) { $received = new PhabricatorMetaMTAReceivedMail(); $received->setHeaders($headers); -$received->setBodies(array( - 'text' => $text_body, - 'html' => $parser->getMessageBody('html'), -)); +$received->setBodies($content); $attachments = array(); foreach ($parser->getAttachments() as $attachment) {