Summary:
- ipull there is wrong
- The `+` wasn't doing what I thought it was doing.
- I already forgot what that detour was doing, so I wrote it down.
Test Plan: Load Versions page, see no error log.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14686
Summary: Fixes T5788. We already have this as a pre-commit field, add it as a post-commit field too.
Test Plan: Ran this rule on a merge commit. Also ran it on a non-merge commit. Both got the correct value.
Reviewers: avivey, chad
Reviewed By: avivey, chad
Subscribers: avivey
Maniphest Tasks: T5788
Differential Revision: https://secure.phabricator.com/D14685
Summary:
Fixes T9206. This was also blocked on tokenizers being weird.
Also clean up some rendering stuff from the earlier changes.
Test Plan:
- Added an "unassign" rule by typing "None", per instructions in the placeholder text.
- Ran the rule.
- Task got unassigned.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9206
Differential Revision: https://secure.phabricator.com/D14684
Summary:
Ref T7848. This is a companion to "Change status to: ...".
(I'm pretty sure the only reason I didn't originally write these was the tokenizer bug in D14682, I just forgot about it).
This is basically a copy/paste of the "status" action.
Test Plan:
- Wrote a rule to change task priorities.
- Edited a task.
- Saw rule fire properly.
- Tokens also stick around correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7848
Differential Revision: https://secure.phabricator.com/D14683
Summary:
Fixes T7848. @jasonfsmitty discussed an issue in great detail there and in D14359, and I completely missed it. Specifically:
- If you save a "Change status to: Open" rule in Maniphest, and then edit it again, the token shows "Unknown Object (???)" instead of the correct token.
- That's because loadHandles() has no idea what to do with the value "open", since it's not a real PHID.
The way we render tokenizer tokens in Herald is quite hacky right now. Fortunately, I wrote a //slightly// better way for EditEngine yesterday or the day before. Use the slightly better way to fix the issue with D14359.
This could still be better than it is, but the badness is mostly hidden now and can be cleaned up later without impacting anything.
Test Plan: Edited a Herald rule with projects and status changes, saw proper tokens.
Reviewers: chad
Reviewed By: chad
Subscribers: jasonfsmitty
Maniphest Tasks: T7848
Differential Revision: https://secure.phabricator.com/D14682
Summary: Fixes T9496. If you have some statuses or priorities you don't need, allow users to disable them to stop the bleeding.
Test Plan:
- Set task to status X and priority Y.
- Disabled X and Y using config.
- Verified task still had old status/priority.
- Verified new task could not be created/edited into those settings.
- Verified task/priority appeared in typeahead, but were marked as disabled.
- Viewed email command docs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9496
Differential Revision: https://secure.phabricator.com/D14681
Summary:
Ref T7848. This patch is incomplete and has the following issues:
- Multiple statuses can be entered on the edit rule page (only the first one is used).
- Statuses are not rendered correctly when re-editing a rule.
Test Plan: Applied to our local phab instance and verified it works with our task workflow.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, revi, epriestley, jsmith
Maniphest Tasks: T7848
Differential Revision: https://secure.phabricator.com/D14359
Summary:
Fixes T7250. Currently, if a display group of transactions (multiple transactions by the same author in a short period of time with no intervening comments) has several transactions of similar strength (e.g., several status change transactions) we can end up displaying them in reverse chronological order, which is confusing.
Instead, make sure transactions of the same type/strength are always in logical order.
Test Plan:
- Merged a task into another task, then reopened the merged task.
- Before patch: merge/reopen showed in wrong order.
{F1014954}
- After patch: merge/reopen show in correct order.
{F1014955}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7250
Differential Revision: https://secure.phabricator.com/D14680
Summary: For consistency, plus I ignore these. Ref T9897
Test Plan: Change to notify, log into notchad, subscribe, change back, see notification instead of email.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9909, T9897
Differential Revision: https://secure.phabricator.com/D14676
Summary: This just pretties up some config like `maniphest.custom-field-definitions` which I noticed was kind of hard to read while chasing down other stuff.
Test Plan: {F1014940}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14679
Summary: Ref T9132. See T9908#147038.
Test Plan:
- As user A, created a new task.
- As user B, started typing a comment on it (with no prior activity).
- Users A and B must be different.
Before patch: preview/draft don't work, trace in error log (see above).
After patch: preview/draft work.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14678
Summary:
Ref T9132. Fixes T4580. Thhat might actually have been fixed a while ago or something since it describes a buggy/bad interaction which doesn't reproduce for me at HEAD.
This saves and restores all the stacked actions (subscribers, projects, etc) so that you don't lose anything if you close a window by accident.
Test Plan:
Added a bunch of actions in various states, reloaded the page, draft stuck around.
Submitted form, actions didn't stick around anymore.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4580, T9132
Differential Revision: https://secure.phabricator.com/D14675
Summary: Ref T9132. I can't actually get rid of the EditController yet since a few weird things still use it, but I think I can swap this button out without breaking anything. This will let us do "New Feature Request" / "New Bug" / "Advanced Task Creation" on secure and start playing with this stuff sooner.
Test Plan: Clicked "Create Task", got sent to new form.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14673
Summary:
Ref T9132. We currently have an old preview/draft behavior and a new actions behavior.
Let the actions behavior do drafts/previews too, so we can eventually throw away the old thing.
This is pretty much just copying the old behavior into the new one, but with a few tweaks. The major change is that we submit all the stacked actions behavior now, so the preview reflects everything the change will do (and, soon, we can save it in the draft in a consistent way).
Also includes one hack-fix that I'll clean up at some point.
Test Plan: Added a bunch of stacked actions and observed meaningful previews.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14672
Summary:
Ref T9132. Open to discussion here since it's mostly product stuff, but here's my gut on this:
- Change Maniphest behavior to stop assigning tasks if they're unassigned when closed. I think this behavior often doesn't make much sense. We'll probably separately track "who closed this" in T4434 eventually.
- Only add the actor as a subscriber if they comment, like in other applications. Previously, we added them as a subscriber for other types of changes (like priority and status changes). This is more consistent, but open to retaining the old behavior or some compromise between the two.
- Retain the "when changing owner, subscribe the old owner" behavior.
Test Plan:
- Added a comment, got CC'd.
- Changed owners, saw old owner get CC'd.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14670
Summary:
Ref T9132. This is kind of a mess because the tokenizer rewrite left rendering tokenizers in Javascript a little rough. This causes bugs like icons not showing up on tokens in the "Policy" dialog, which there's a task for somewhere I think.
I think I've fixed it enough that the beahavior is now correct (i.e., icons show up properly), but some of the code is a bit iffy. I'll eventually clean this up properly, but it's fairly well contained for now.
Test Plan:
- Reassigned a task.
- Put a task up for grabs.
- No reassign on closed tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14669
Summary: Ref T9132. Supports selects in stacked actions and adds "Change Status" + "Change Priority".
Test Plan: Changed status and priority from stacked actions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14667
Summary:
Ref T9132. Only allow a task to have a single owner in the UI.
In Conduit, make this field appear and behave as "phid" instead of "list<phid>".
Test Plan: Edited a task with new fancy form, got limited to one owner. Assigned/unassigned. Used Conduit to assign/unassign.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14666
Summary: Ref T9132. This makes the "Quote" action on comments work properly in these applications.
Test Plan: Quoted text in each application.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14665
Summary: Ref T9132. Shhh this never happened shhhhhhh.
Test Plan: Selected multiple actions, saw them add at the bottom.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14664
Summary:
Ref T9132. Like D14659, I'll hold this until after the cut.
This swaps commenting in Maniphest over to EditEngine / stackable actions. New code doesn't have parity yet, although none of the things we're missing should technically be //strictly mandatory//. There's a list inline. I'll restore these in the next diffs.
Briefly -- comments, subscribers and projects work. Status, owners and priority do not yet.
Test Plan:
- Made comments and added subscribers and projects.
- Read through the old code to look for missing features and tried to document them all.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14663
Summary:
Ref T9132. I'm going hold this until after the release cut since it isn't going to land completely smoothly, but I think I can prep it today/tomorrow and hopefully get it close enough to working to put in HEAD on Saturday after the push.
This adds the basics: new EditEngine, new EditController, and new `maniphest.edit` API endpoint.
I put the new stuff at `editpro/` for now until it works a little better.
Some notes on stuff this is dropping/changing/not-working-yet:
- Preview for the description. I'd rather solve this by putting a "Preview" button on every Remarkup area if we want to retain it. Particularly, it does not generalize to adding custom remarkup fields in its current form. See also T3967.
- Per-field policies are no longer enforced. They were never truly enforced anyway (for example, any user who can edit a task has always been able to edit every field via Conduit or email actions or Herald, where Herald supports things), and only really served as a hint to users. I think we can obsolete this by having installs hide/lock these fields instead. This is a desirable outcome for me, since I don't like retaining these policies and the idea of truly enforcing them properly is worrisome. These were originally added for Uber as an onboarding sort of thing. I'll prepare users for this in greater detail in the documentation.
- Couple of minor bugs with ordering / defaults / only-one-owner-allowed in this diff that I'll clean up in future diffs before this stuff lands.
- I don't have a concrete plan on "Create Similar Task" / "Clone" yet (do you have thoughts? Is this worth trying to do in every application?). I'll probably just mostly mimic the current behavior.
Test Plan: I'll vet this more thoroughly in followups, just banged around some tasks for now and created/edited via the API.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14659
Summary: Provides a real 2x avatar and offers new built in images for profile pictures.
Test Plan: reload profile, see sharper image, pick eevee, see eevee
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14668
Summary: Use Profile Image, remove skin, show domain info better, add New Post button. Ref T9897
Test Plan: Test new buttons, see new images.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14671
Summary: Updates some internal builtin image defaults for 2x, and adds some new ones for profiles
Test Plan: View my profile, still works
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14661
Summary: Make this function re-usable in other views. Ref T9897
Test Plan: View a blog, see the same information
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14658
Summary: Color nodata as nodata, fix picture redirect, give hints when items aren't set on blogs. Ref T9897
Test Plan: Tested each of these items.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9897
Differential Revision: https://secure.phabricator.com/D14657
Summary: I spent way to long to arrive at this solution. Ref T9360
Test Plan: Publish a new post, see time, unpublish post, see draft. Start a new post, wait 10 minutes, publish, see "now".
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9360
Differential Revision: https://secure.phabricator.com/D14656
Summary: Ref T9132. This still has a lot of rough edges but the basics seem to work OK.
Test Plan: {F1012627}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14653
Summary:
Ref T9132. Fixes T5031. This approximately implements the plan described in T5031#67988:
When we recieve a preview request, don't write a draft if the form is from a version of the object before the last update the viewer made.
This should fix the race-related (?) zombie draft comments that sometimes show up.
I just added a new object for this stuff to make it easier to do stacked actions (or whatever we end up with) a little later, since I needed to do some schema adjustments anyway.
Test Plan:
- Typed some text.
- Reloaded page.
- Draft stayed there.
- Tried real hard to get it to ghost by submitting stuff in multiple windows and typing a lot and couldn't, although I didn't bother specifically narrowing down the race condition.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5031, T9132
Differential Revision: https://secure.phabricator.com/D14640
Summary:
Ref T9132. When I've touched `PhabricatorApplication` I keep hitting this bad `pht()` junk.
The warning is correct, these strings are not extactable and can not be translated.
Fix it so they can be extracted and translated.
Broadly, in all cases we want to render one of these:
> 95 Things (for fewer than some limit)
> 99+ Things (when we hit the limit)
Test Plan: Looked at homepage status counts, moused over them, saw reasonable strings. Grepped for removed method.
Reviewers: chad
Reviewed By: chad
Subscribers: joshuaspence
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14638
Summary:
Ref T9132. This just replaces the "Add Comment" form in Paste with a generic flow in EditEngine.
No actual field-awareness or action stacking or anything quite yet, but that will come in a bit. This mildly regresses drafts (which don't seem like a big deal for Pastes). I'll hook those up again in the next diff, but I want to build them in a better way that will work with multiple actions in a generic way, and solve T5031.
Big practical advantage here is that applications don't need copy/pasted preview controllers.
Test Plan:
- Saw previews.
- Added comments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14637
Summary: Seeing if this is the correct path, then will apply in Pholio, Ponder.
Test Plan: epriestley
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, Korvin
Maniphest Tasks: T9825
Differential Revision: https://secure.phabricator.com/D14646
Summary: This adds a separate Publish/Unpublish step aside from Preview in Phame Posts. This allows easier access to publishing without previewing, though I left publish in tact on the preview page. Also cleaned up some minor transaction issues with mail.
Test Plan: New Post, Publish Post, Preview Post. Check mail logs. Get mail upon publish.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14642
Summary:
Ref T9132. Give most standard custom fields reasonable Conduit support so you can use the new `application.x` endpoints to set them.
Major missing field type is dates, again.
Test Plan: Used Conduit to set various custom fields on a package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14635
Summary:
Ref T9132. This allows you to prefill custom fields with `?custom.x.y=value`, for most types of custom fields.
Dates (which are substantially more complicated) aren't supported. I'll just do those once the dust settles. Other types should work, I think.
Test Plan:
- Verified custom fields appear on "HTTP Parameters" help UI.
- Used `?x=y` to prefill custom fields on edit form.
- Performed various normal edits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14634
Summary:
Fixes T9874.
- Stop using the phrase "restart your webserver". Instead, say "restart Phabricator".
- Write a document explaining that "Restart Phabricator" means to restart all of the server processes, depending on how your configuration is set up, and approximately how to do that.
- Link to this document.
- In places where we are not specifically giving instructions and the user isn't expected to do anything, be intentionally vague so as to avoid being misleading.
Test Plan:
- Read document.
- Hit "exetnsion" and "PHP config" setup checks, got "restart Phabricator" with documentation links in both cases.
Reviewers: chad
Maniphest Tasks: T9874
Differential Revision: https://secure.phabricator.com/D14636
Summary:
Ref T9132. This isn't perfect, but doesn't break any existing functionality. This stuff works:
- Editing values.
- Reordering fields.
- All builtin field tyepes.
This stuff may not work yet:
- Assigning custom field defaults.
- Some conduit stuff.
- Fully custom fields?
- Locking/hiding fields? Didn't actually test this one.
I'll keep chipping away at that stuff. In some cases, it may be easier to convert all the CustomField apps first, although Differential might be a fair bit of work.
Test Plan:
Created a bunch of custom fields of every avialable type and edited them.
{F1008789}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14617
Summary:
Ref T9132. Paste is in fairly good shape so Owners is up next. Reasoning:
- One install wants API access for it.
- It's a simple application for getting CustomFields working with EditEngine.
This only does the EditEngine part, so CustomFields are no longer editable until I make that work. That will be up next, and I'll hold this until that's ready.
Test Plan:
- Created and edited packages via web UI.
- Created and edited package editing forms via web UI.
- Created and edited packages via Conduit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14598
Summary:
Fixes T9881. This one had Quicksand spelled as "QuickSand" (with capital "S") so it probably didn't get hit by `grep`.
Didn't need to do any special magic with the footer, as far as I can tell.
Test Plan: Loaded project board view, seemed to work OK (no footer, nav works, title works, mobile menu sane).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9881
Differential Revision: https://secure.phabricator.com/D14626
Summary: Fixes T9715. Adds a MySQL-based lock to ensure that schema migrations are not applied on multiple hosts simultaneously.
Test Plan: Ran `./bin/storage upgrade` concurrently. One invocation was successful whilst the other hit a `PhutilLockException`.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9715
Differential Revision: https://secure.phabricator.com/D14463
Summary: Removes all calls to addExtraQuicksandConfig Ref T9690
Test Plan: grep for addExtraQuicksandConfig, view a Pholio Page with and without chatbar, edit a pholio mock, save mock.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D14622
