Summary: Fixes T12160. Lightbox thread view should be visible if file is public.
Test Plan:
Add a file to a task, log out, click on file in task, get lightbox and no error. Expand comments, see login box.
{F2867067}
{F2867088}
{F2867098}
{F2867114}
{F2867124}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12160
Differential Revision: https://secure.phabricator.com/D17347
Summary: Fixes T12258. I think these constants are just flipped.
Test Plan: Kinda winged it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12258
Differential Revision: https://secure.phabricator.com/D17346
Summary:
Ref T12237. This adds a UI cue for users who have unverified primary addresses, since we no longer send them mail.
Also adds a new `bin/mail unverify` to unverify an address (for example, because mail is bouncing).
Test Plan:
- Unverified my address, saw setup issue.
- Verified my address, no more setup issue.
{F2861820}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12237
Differential Revision: https://secure.phabricator.com/D17344
Summary: Fixes T12253.
Test Plan:
- Before change: used "Quote Comment", saw "In null, alice wrote:" in quoted text.
- After change: used "Quote Comment", saw proper reference to the commit/page. Clicked reference, was sent to the comment properly.
{F2859093}
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T12253
Differential Revision: https://secure.phabricator.com/D17343
Summary: Ref T10390. This removes the "Copy Dashboard" feature, which was more of a crutch to assist in the complexity of building and maintaining dashboards. I think we're close enough now that removing this and adding in some simpler edit dialogs should negate any benefit to keeping this around. Also removed an un-used "Uninstall Dashboard" dialog.
Test Plan: Visit manage, edit, no longer see option to copy dashboard. grep /dashboards/ for "copy" and remove all traces. Add some panels to a dashboard I own.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17338
Summary: Fixes T4984. This is about as fancy as I want to get this pass. Adds in the list of panel titles and the author. This does give me a rough idea what's on each dashboard.
Test Plan:
Visit a list of dashboards and see various authors and panels.
{F2810876}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T4984
Differential Revision: https://secure.phabricator.com/D17340
Test Plan: attempted to create a new auth provider; observed that "enabled" ui element does not render. viewed existing auth provider and observed that "enabled" ui element still renders
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T12245
Differential Revision: https://secure.phabricator.com/D17337
Summary: Fixes T12252.
Test Plan:
I just faked this, but likely repro is:
- Call method `x.y`.
- Remove method `x.y` from the codebase.
- View log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12252
Differential Revision: https://secure.phabricator.com/D17342
Summary: Fixes T12224. This brings "Autopay" on the View controller into line with how it works on the Edit controller.
Test Plan:
- Viewed subscriptions with no autopay, valid autopay, and deleted autopay.
{F2750725}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12224
Differential Revision: https://secure.phabricator.com/D17334
Summary: Fixes T12243. That error occured due to network flakiness with some mounted filesystems so I'm not sure how best to simulate it. But you can look and see that the PhutilProxyException does indeed expect an exception as its second arg.
Test Plan: Look at method signature... look at callsite... now back at the method. Smile and nod.
Reviewers: #blessed_reviewers, yelirekim, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T12243
Differential Revision: https://secure.phabricator.com/D17335
Summary: Ref T12240. When you "Reply All" to a Phabricator mail, we make an effort not to send the response to recipients who you hit with the original message. This isn't perfect and we can't always get it right, but the old description implies it's a bigger problem than it should be in practice.
Test Plan: Read text.
Reviewers: chad, eadler
Reviewed By: chad
Maniphest Tasks: T12240
Differential Revision: https://secure.phabricator.com/D17331
Summary: Ref T10390. This mostly shuffles layout into "View" and keepts "Manage" around for Edit/Copy/History. This feels better to me overall. Also tweaked some spacing and color.
Test Plan:
New Dashboard, edit Dashboard, shuffle panels. Create new panels.
{F2684043}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17326
Summary:
Ref T12237. This tightens our delivery rules, which previously sent normal mail to unverified addresses:
- We sent general mail to unverified addresses so that you wouldn't miss anything between the time you sign up (or have an account created) and the time you verify your address. This was imagined as a slight convenience for users.
- We sent automatic reply mail to unverified addresses if they sent mail to us first, saying "we don't recognize that address". This was imagined as a convenience for users who accidentally send mail "From" the wrong address (personal vs work, for example).
I think both behaviors are probably a little better for users on the balance, but not having mail providers randomly shut us off without warning is better for me, personally -- so stop doing this stuff.
This creates a problem which we likely need to solve before the release is cut:
- On installs which do not require mail verification, mail to you will now mostly-silently be dropped if you never bothered to verify your address.
I'd like to solve this by adding some kind of per-user alert that says "We recently tried to send you some mail but you haven't verified your address.", and giving them links to verify the address and review the mail. I'll pursue this after restoring mail service to `secure.phabricator.com`.
Test Plan:
- Added a unit test.
- Unverified my address, sent mail, saw it get dropped.
- Reverified my address, sent mail, saw it go through.
- Verified that important mail (password reset, invite, confirm-this-address) either uses "Force Delivery" (skips this check) or "Raw To Addresses" (also skips this check).
- Verified that Phacility instance stuff is also covered: it uses the same invite flow.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12237
Differential Revision: https://secure.phabricator.com/D17329
Summary: Fixes T9336. Kind of a bit to back up and find the source, but works easily.
Test Plan: View feed, click on my image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9336
Differential Revision: https://secure.phabricator.com/D17322
Summary: Moves profile/project to use more standard colored boxes. Reverts dashboard border colors. Ensures better High-Contrast application more consistently across these projects. Also fix T12211.
Test Plan: Home, People, Projects in High Contrast / Standard
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12211
Differential Revision: https://secure.phabricator.com/D17321
Summary:
Fixes T12215. Two issues:
- We build this `$session` link out of `$ip`, which is (a) wrong even if `$ip` was the IP and (b) super wrong since `$ip` is a tag.
- These links don't work even if we'd built them right: searching by the //prefix// of a session identifier does nothing.
At least for now, just get rid of the links rather than trying to make this behavior work.
Test Plan:
On People > Activity logs:
- Before patch: Saw bad links with bogus targets in "session" column.
- After patch: Saw plain text in "session" column.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12215
Differential Revision: https://secure.phabricator.com/D17316
Summary: Fixes T12216. I'd like to remove this option eventually, but just narrow its scope in the config description for now.
Test Plan: Read config description.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12216
Differential Revision: https://secure.phabricator.com/D17317
Summary: Lots of little details, fix workboard bg colors, darken up global backgrounds just a hair, add more "widgety" look to dashboard panels, remove underline on anchors on mobile. Also Fixes T12210
Test Plan: Use lots of pages on mobile and desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12210
Differential Revision: https://secure.phabricator.com/D17315
Summary:
Ref T12207. Currently, to remove a panel from a dashboard, it must be a valid panel which you can see.
Instead, only require that the panel PHID actually be listed somewhere in the dashboard's internal list of panels.
This interacts with the "multiple instances of a panel" issue described in some more depth in T12207. In particular:
- Currently, you can sort of add multiple copies of a panel to a dashboard, sometimes? Maybe?
- This leads to great tragedy.
This doesn't fix up the workflow with respect to multiple copies of a panel. We still remove by panel PHID (not by column/position or internal ID) so if a dashboard has multiple copies of the same panel for some reason, I think this workflow removes one of them arbitrarily (at best) or perhaps does something worse. I'm just treating this behavior as undefined for the moment.
Test Plan:
- Removed an invalid/hidden panel from a dashboard as a user with permission to edit that dashboard.
- Tried to remove a made-up panel with a totally bogus PHID, got 404'd.
- Viewed a dashboard with a restricted panel.
- Put a hidden panel inside a tab panel, viewed it as a user who could not see it and a user who could.
Reviewers: chad
Reviewed By: chad
Subscribers: swisspol
Maniphest Tasks: T12207
Differential Revision: https://secure.phabricator.com/D17314
Summary:
Fixes T12203. If you tried to //manage// a dashboard which had a panel you can't see, we'd try to render bogus actions for it and fatal.
Instead, for the moment, survive. Presumably we'll ship a real fix for this in the next release or so, and tackle T10612 / T10145, which I think are closely related.
Test Plan: {F2570418}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12203
Differential Revision: https://secure.phabricator.com/D17311
Summary: This could hit an obscure fatal.
Test Plan:
- Create a macro.
- Upload a file, but don't give it a name.
- Before: fatal.
- After:
{F2569846}
Reviewers: chad, 20after4
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17310
Summary:
Fixes T12195. For the past few years, Recaptcha (now part of Google) has supported
a new, "no captcha" one-click user interface. This new UI is stable, doesn't
require any typing or reading words, and can even work without JavaScript (if
the administrator enables it on the Recaptcha side).
Furthermore, the new Recaptcha has a completely trivial API that can be dealt
with in a few lines of code. Thus, the external `recaptcha` php library is now
gone.
This API is a complete replacement for the old one, and does not require any
upgrade path for users or Phabricator administrators - public and secret keys
for the "new" Recaptcha UI are the exact same as the "classic" Recaptcha. Any
old Recaptcha keys for a domain will continue to work.
Note that Google is currently testing Yet Another new Captcha API, called
"Invisible reCAPTCHA", that will not require user interaction at all. In fact,
the user will not even be aware there //is even a captcha form//, as far as I
understand. However, this new API is 1) in beta, 2) requires new Recaptcha keys
(so it cannot be a drop-in replacement), and 3) requires more drastic API
changes, as form submission buttons must instead invoke JavaScript code, rather
than a token being passed along with the form submission. This would require far
more extensive changes to the controllers. Maybe when it's several years old, it
can be considered.
Signed-off-by: Austin Seipp <aseipp@pobox.com>
Test Plan:
Created a brand-new Phabricator installation, saw the new Captcha UI
on administrator sign up. Logged out, made 5 invalid login attempts, and saw the
new Captcha UI. Reworked the conditional to invert the condition, etc to test
and make sure the API responded properly.
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: epriestley, #blessed_reviewers
Subscribers: avivey, Korvin
Maniphest Tasks: T12195
Differential Revision: https://secure.phabricator.com/D17304
Summary:
Ref T12174. Ref T8033. Currently, if you can't see one panel on a dashboard, you can't see the dashboard at all. This is confusing and hard to debug.
Improve this behavior at least slightly: render the dashboard, with a big "you can't see this" panel in place of any panels you can't see. This should at least make the behavior obvious, even if it isn't the best or most comprehensive way we can handle it in all cases.
Test Plan: {F2566003}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174, T8033
Differential Revision: https://secure.phabricator.com/D17308
Summary: Ref T12174. Drag-and-drop-to-upload requires some stuff in the document. Put that stuff on all the content pages (currently: dashboards, magic home), not just the builtin home.
Test Plan:
- Dragged-and-dropped onto a Home dashbboard to upload.
- Viewed, and dragged-and-dropped onto "builtin home" to upload.
- Dragged onto "Edit Menu" for home, no upload.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17307
Summary:
Fixes T12197. I //think// this field was never recognized by Differential (it doesn't appear in D17070, but maybe that isn't the right change).
It was recognized by the ad-hoc regular expression which I replaced with a formal parser in D17262.
Allow the former parser to accept "Auditor" as an alias for "Auditors".
Test Plan: Committed a change with `Auditor: dog`, saw the audit trigger correctly in the web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12197
Differential Revision: https://secure.phabricator.com/D17306
Summary: Ref T5307, Makes these buttons a little more clear visually and verbosely. Adds white icons for blue buttons.
Test Plan: Test saving a search, viewing button changes on various form pages / uiexamples.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5307
Differential Revision: https://secure.phabricator.com/D17305
Summary:
Ref T12174.
- Home now always uses the topmost item (falling back to "magic home") and no longer supports pinning. If any personal item may be a default item, it will always be picked over any global item.
- Favorites doesn't use defaults anyway, but no longer has misleading UI suggesting it might.
Test Plan:
- Saw no pinning UI on Home/Favorites.
- Added a personal dashboard on Home, it automatically became the new default.
- Pinned stuff normally on Projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17298
Summary: Just making profiles a little nicer, adds a big picture, easier mechanism for updating photos. Also larger profile pictures... need to re-thumb?
Test Plan:
View my profile, edit my picture, view a stranger, see profile. Check mobile, tablet, desktop. Check action menu on mobile.
{F2559394}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17295
Summary: This just cleans up a method call that was missed in D15986. It's been causing fatal errors in one of our workflows.
Test Plan: Grep'd for other instances of `withIsTag` and didn't find any
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Differential Revision: https://secure.phabricator.com/D17299
Summary: Fixes T12187. Ref T12190. See T12190 for discussion of why this escaped notice.
Test Plan:
- Commented out the `error_reporting()` clause around file inclusion.
- Reproduced the error in PHP7.
- Corrected the method signature.
- Reloaded the page, no more error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12190, T12187
Differential Revision: https://secure.phabricator.com/D17297
Summary: Ref T12174. This could be a little more verbose.
Test Plan: Review Global Menu Items
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17294
Summary:
Ref T12174.
- Go back to the old mobile behavior (full-screen menu by default, click to see content).
- Hide crumbs from all Home content UIs. I left them on the edit/configure UIs since they feel a little less out-of-place there and some have multiple levels.
Test Plan:
Viewed Home on mobile, viewed `/home/` on mobile.
Also, saw no crumbs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17290
Summary: Ref T12174. Fallback behavior on this already appears to be sensible.
Test Plan:
- Hid "Magic Home".
- Viewed homepage with no dashboards on the menu.
- Saw "Magic Home" content, with no item in the menu selected, which seems reasonable.
{F2557022}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17289
Summary:
Ref T12174. Setup is:
- Allow public access.
- Don't touch the default menu.
- Visit `/` while logged out.
Currently, you see "magic home" as content, but don't actually see the menu item.
Instead, show the menu item.
Test Plan: {F2557000}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17288
Summary: Ref T12174. We were always setting a name via builtins so the tooltip was always set. Fix the calls here.
Test Plan: Add "Badges", see tooltip, give "Badges" a name of "Badges", don't see tooltip.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17284
Summary: Ref T12174. Always sets the correct type when converting to ActionList, adds a type to Divider.
Test Plan:
Add a Label, 2 applications to the personal favorites menu, see nice styles.
{F2554901}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17285
Summary:
Ref T12173.
- If we want to fetch a tag, Buildkite needs it as a "branch" (this means more like "ref to fetch").
- The API gets upset if we pass "refs/tags/...", so just pass the tag name without the prefix, which works.
- Do a better job with commits and pass a real branch to fetch.
Test Plan:
- Built a commit with Buildkite.
- Build a revision with Buildkite.
Reviewers: chad
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12173
Differential Revision: https://secure.phabricator.com/D17282
Summary: Fix copy for installing dashboard, add a revision panel, and change the default name to make it easier to find. Ref T12174
Test Plan: Go to dashboards, click New, then Simple. Visit home and install my dashboard
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17277
Summary: Ref T12174. Dashboards and "Home" currently use the page title "Configure Menu". Give them more appropriate titles instead.
Test Plan: Viewed dashboards, Home. Saw relevant page titles.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17281
Summary:
Ref T10978. I'm inching toward cleaning up our audit state. Two issues are:
- Authored commits show up in "Ready to Audit", but should not.
- Unreachable commits (like that stacked of unsquashed stuff) show up too, but we don't really care about them.
Kick authored stuff out of the "Ready to Audit" bucket and hide unreachable commits by default, with constraints for filtering. Also give them a closed/disabled/strikethru style.
Test Plan:
- Viewed audit buckets.
- Searched for reachable/unreachable commits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17279
Summary:
Ref T12174. We now require that we can figure out a valid "edit mode" (global vs custom/personal) before we hit EditEngine. Since the EditEngine routes don't have an `itemID`, they would failu to figure out the mode and just 404.
Let the engine use `id` (from EditEngine) if `itemID` (from MenuEngine) isn't present in the route.
Test Plan:
- Edited some menu items on Home / Projects.
- (I think I tested this, then broke it, originally.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17280
Summary: Ref T12174, lets you set labels as well for dividing content.
Test Plan: Add a label, review on homepage.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17278
Summary: Ref T10978. Although this script prints out some very good changes, it does not currently persist them to the database.
Test Plan: Ran `bin/audit synchronize`, saw the change appear both on the CLI and in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17276
Summary:
Ref T12174. This isn't really a "newManageItem()" since Projects have a separate manage screen.
That is, I incorrectly changed the "Manage [This Project]" item into a "Edit Menu" item, so some options (like "Archive Project") incorrectly became inaccessible.
Test Plan: Viewed a project, saw the right menu item, clicked it, could archive/etc project. Also edited the menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17275
Summary: Ref T12174. These items could fatal (`$item not defined`) if the viewer was not logged in.
Test Plan: - Viewed home as a logged-out user.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17274
Summary:
Ref T12174. This fixes more bugs than it creates, I think:
- Dashboards now show the whole menu.
- Project and home items now show selected state correctly.
- The "choose global vs personal" thing is now part of MenuEngine, and the same code builds it for Home and Favorites.
- Home now handles defaults correctly, I think.
Maybe regression/bad/still buggy?:
- Mobile home is now whatever the default thing was, not the menu?
- Title for dashboard content or other items that render their own content is incorrectly always "Configure Menu" (this was preexisting).
Test Plan:
- Created, edited, reordered, disabled, deleted and pinned personal and global items on home, favorites, and projects.
- Also checked User profiles.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17273
Summary: Ref T12173. This might need some additional work but the basics seem like they're in good shape.
Test Plan:
- Buildkite is "bring your own hardware", so you need to launch a host to test anything.
- Launched a host in AWS.
- Configured Buildkite to use that host to run builds.
- Added a Buildkite build step to a new Harbormaster build plan.
- Used `bin/harbormaster build ...` to run the plan.
- Saw buildkite execute builds and report status back to Harbormaster
{F2553076}
{F2553077}
Reviewers: chad
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12173
Differential Revision: https://secure.phabricator.com/D17270
Summary: Ref T10978. This is just a maintenance convenience script. It can fix up overall commit state after you `bin/audit delete` stuff or nuke a bunch of stuff from the database, as I did on `secure.phabricator.com`.
Test Plan: Ran `bin/audit synchronize`, and `bin/audit update-owners`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17271
Summary: Ref T11957. Needs some more polish, but I think everything here is square.
Test Plan: Add personal/global items to home, test mobile. Test workboards / colors.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: 20after4, rfreebern, Korvin
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17259
Summary: Ref T10978. This code (mostly related to the old ADD_AUDIT transaction and some to the "store English text in the database" audit reasons) is no longer reachable.
Test Plan:
Grepped for removed symbols:
- withAuditStatus
- getActionNameMap (unrelated callsites exist)
- getActionName (unrelated callsites exist)
- getActionPastTenseVerb
- addAuditReason
- getAuditReasons
- auditReasonMap
Also audited some commits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17267
Summary:
Ref T10978. This updates audits triggered by Owners to use a modern transaction. Minor changes:
- After D17264, we no longer need the "AUDIT_NOT_REQUIRED" fake-audits to record package membership. This no longer creates them.
- This previously saved English-language, untranslatable text strings about audit details onto the audit relationship. I've removed them, per discussion in D17263.
The "Audit Reasons" here are potentially a little more useful than the Herald/Explicit-By-Owner ones were, since the rules are a little more complex, but I'd still like to see evidence that we need them.
In particular, the transaction record now says "Owners added auditors: ...", just like Differential, so the source of the auditors should be clear:
{F2549087}
T11118 (roughly "add several Owners audit modes", despite the title at time of writing) might impact this too. Basically, this is simple and maybe good enough; if it's not quite good enough we can refine it.
Test Plan: Ran `bin/repository reparse --owners <commit>` saw appropriate owners audits trigger.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17266
Summary:
Ref T10978. Currently, during commit import, we write an "Audit Not Required" auditor for commits which don't require an audit.
This auditor is used to power the "Commits in this package" query in Owners.
This conflates audits and commit/package membership. I think it might even predate edges. Code needs to dance around this mess and we get the wrong result in some cases, since auditors are now editable.
Instead, write an explicit edge which just says "this commit is part of such-and-such packages". Then use that to run the query. Logical!
I'll issue guidance on this but I'm not migrating it, since it fixes itself going forward and only really affects the UI in Owners.
Test Plan:
- Ran `bin/audit update-owners` with various arguments.
- Viewed packages in web UI, saw them load the proper commits.
- Queried by packages in Diffusion explicitly.
- Clicked the "View All" link in Owners and got to the right search UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17264
Summary:
Ref T10978. Convert "Add Auditors" rules in Herald to modern modular transactions.
Here and in D17262 (and in the next change), I've removed "audit reasons". There are several reasons for this:
- They're pretty hacky.
- They store English-language (well, usually) text in the database, which can't be translated.
- I think they may not be necessary. When they were written, Herald did not apply transactions, so it was less clear when Herald was doing something. In modern code, it does, so Herald auditors are clear. The owenrs/package rules are now more clear, too. I'd like to see evidence that confusion still exists before rebuilding this feature in a modern, translatable way, since I think we may not need it at all.
Test Plan: Ran `bin/repository reparse --herald <commit>` to re-run Herald rules. Saw rules add auditors appropriately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17263
Summary:
Ref T10978. Updates how we implement "Auditors: ..." in commit messages:
- Use the same parsing code as everything else.
- (Also: parse package names.)
- Use the new transaction code.
Also, fix some UI strings.
Test Plan: Used `bin/repository reparse --herald <commit>` to re-run this code on commits with various messages (valid Auditors, invalid Auditors, no Auditors). Saw appropriate auditors added in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17262
Summary: Fixes T12166. We don't actually need this variable, so removing it.
Test Plan: Upload a new mock, edit a mock, view list of mocks.
Reviewers: epriestley, Mnkras, acs-ferreira
Reviewed By: epriestley, Mnkras, acs-ferreira
Subscribers: acs-ferreira, Korvin
Maniphest Tasks: T12166
Differential Revision: https://secure.phabricator.com/D17260
Summary:
Ref T10978. Currently, too many "This audit now <something something>" transactions are posting, because this strict `===` check is failing to detect that the audit is already in the same state.
This is because audit states are currently integers, and saving an integer to the database and then reading it back turns it into a string. This is a whole separate can of worms. For now, just weaken the comparison. I'd eventually like to use string constants here instead of integer constants.
Test Plan:
Commented on a "no audit required" commit, didn't see a double "this doesn't need audit" transaction anymore.
Also made a legit state change and did see a state transaction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17258
Summary:
Fixes T12159. This is similar to D17228, which fixed this for the main configuration operation.
Most other edit operations only test for edit capability on the MenuItem itself, which we already do correctly. However, because reordering affects all items, we test for capability on the object.
Weaken this when reordering custom items.
Test Plan: Reordered custom items in Favorites as a non-administrator.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12159
Differential Revision: https://secure.phabricator.com/D17257
Summary: Fixes T11547. I //think// this mostly gets about addressing @epriestley's comments in D16465 and stores each paste's line count in its snippet so that we can display the actual number of lines in the paste rather than '5 Lines'. Let me know if this is on the right track!
Test Plan: Open /paste and see that each paste's actual line count is reported.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T11547
Differential Revision: https://secure.phabricator.com/D17256
Summary: Ref T10978. This was introduced in D6923 in 2013 as a deprecated method (before methods were extensible) and has only ever been deprecated. It no longer works after D17250 (despite my mistaken claim there that we never had an API for actions), and has been superceded by `diffusion.commit.edit` which is a modern, fully-power method.
Test Plan: Viewed Conduit console, no longer saw method.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17254
Summary:
Ref T11957. When you click a dashboard item, it now sends you to `/<app>/item/view/123/`, which renders the proper crumbs, navigation, etc., with the dashboard as page content.
This works as you'd expect in Projects:
{F2508568}
It's sliiiightly odd in Favorites since we nuke the nav menu, but seems basically fine?
{F2508571}
Test Plan:
- Created a dashboard panel on a project.
- Clicked it, saw it render.
- Made it the default panel, viewed project default screen, saw dashboard.
- Disabled every panel I could, still saw reasonable behavior (this is silly anyway).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17255
Summary:
Fixes T2393. This allows authors to explicitly say "I think I fixed everything, please accept my commit now thank you".
Also improves behavior of "re-accept" and "re-reject" after new auditors you have authority over get added.
Test Plan:
- Kicked a commit back and forth between an author and auditor by alternately using "Request Verification" and "Raise Concern".
- Verified it showed up properly in bucketing for both users.
- Accepted, added a project, accepted again (works now; didn't before).
- Audited on behalf of projects / packages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17252
Summary:
Ref T2393. We had three copies of this code ("which packages/projects can a user accept on behalf of?"). I removed one in D17250. This consolidates the other two.
This still isn't perfect and it should probably live in a Query or something some day, but there's some weird stuff going on with the viewer in the editor context, and at least the code handles the viewer correctly now and isn't living somewhere weird and totally unrelated to auditing, and the callsites don't need to do a bunch of extra work.
This also moves towards fixing the "re-accept if you've already accepted but then a new package you have authority over was added" bug, which we fixed recently in Differential. This should be less common in Audit, but should still be fixed.
Test Plan: Viewed and audited commits with a mixture of user, package, and project auditors. Saw actions apply to the expected set of auditors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17251
Summary:
Ref T2393. This code is no longer reachable (we never had an API for auditing in Diffusion) and unused. Clean it up before implementing new states/actions.
(Note that code for displaying these transactions still needs to stick around for a bit, we'll just never apply new ones from here on out. They've been replaced with modular transactions.)
Test Plan: Grepped for usage, commentd on / audited a commit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17250
Summary: Ref T2393. This has been obsoleted by stacked actions and is no longer used.
Test Plan: Grepped for callsites, viwed commits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17249
Summary:
Ref T11114. Converting to EditEngine caused us to stop running this validation, since these fields no longer subclass this parent. Restore the validation.
Also, make sure we check the //first// line of the value, too. After the change to make "Tests: xyz" a valid title, you could write silly summaries / test plans and escape the check if the first line was bogus.
Test Plan: {F2493228}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17248
Summary: Ref T11957, just lays in some minor bug fixes. Sets correct menu, removes sidebar on edit.
Test Plan: Test /menu/ on home with Admin and Normal accounts.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17247
Summary: Ref T2393. This adds a state-change transaction hint to Audit, like we have in Differential. This is partly for consistency and partly to make it more clear what should happen next.
Test Plan: {F2477848}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2393
Differential Revision: https://secure.phabricator.com/D17243
Summary: Ref T12139. Adds sorting by shortname. Also I sorted everything else. No reason. It didn't help
Test Plan: `:star`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12139
Differential Revision: https://secure.phabricator.com/D17246
Summary: Moves the fonts around for better Windows fallback
Test Plan: Windows 10 Edge / Chrome
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17245
Summary:
This adds a more complete emoji datasource, with a typeahead and autocomplete. It works by pulling in a raw datasource from EmojiOne (I chose Unicode 8, but they have a Unicode 9 datasource as well) and transforming it for speed/need. If we build more robustness or an actual picker into the Remarkup bar, having the additional keywords, etc, might be important. When Unicode 9 support is more prevalent, we should only need to update the single file.
Tossing up as a proof of concept on engineering direction. Also I can't quite get the autocomplete to complete.
Test Plan: Test UIExamples, Autocomplete, and TypeaheadSource
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12139
Differential Revision: https://secure.phabricator.com/D17244
Summary: Ref T12139, installs 'Segoe UI Emoji' as a standard font call for color emoji on Windows devices.
Test Plan: Review Emoji on Win 10 Chrome / Edge, Mac Chrome / Safari.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12139
Differential Revision: https://secure.phabricator.com/D17241
Summary: Fixes T12142. Correct spelling of method.
Test Plan: Edit the name of a Details menu item in projects, or add a divider.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12142
Differential Revision: https://secure.phabricator.com/D17240
Summary: Ref T12136. This just yanks the band-aid off. Fundamentally these were useful well before Dashboards and advanced bucketing, but not so much any more. They also have some performance hit.
Test Plan: Add some tasks and diffs onto a new instance, see there is no count on the home menu bar.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12136
Differential Revision: https://secure.phabricator.com/D17238
Summary:
Ref T12140. The major effect of this change is that uninstalling "Home" (as we do on admin.phacility.com) no longer uninstalls the user menu (which is required to access settings or log out).
This also simplifies the code a bit, by consolidating how menus are built into MenuBarExtensions instead of some in Applications and some in Extensions.
Test Plan:
- While logged in and logged out, saw main menus in the correct order.
- Uninstalled Favorites, saw the menu vanish.
- Uninstalled Home, still had a user menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12140
Differential Revision: https://secure.phabricator.com/D17239
Summary:
See T11957#208140.
- Let Applications have a custom name, like other object items (for example, so you can call Maniphest "Tasks" if you prefer).
- Put the optional name field after the required typeahead field for these items.
- (I left "Link" in "Name, URI" order since both are required, but there's maybe an argument for swapping them?)
Test Plan:
- Created each type of item, saw "thing, name" order.
- Created an application with a cusotm name, saw custom name.
- Removed custom name, saw original name.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17236
Summary:
Ref T12128. This adds validation to menu items.
This feels a touch flimsy-ish (kind of copy/paste heavy?) but maybe it can be cleaned up a bit once some similar lightweight modular item types (build steps in Harbormaster, blueprints in Drydock) convert.
Test Plan:
- Tried to create each item with errors (no dashboard, no project, etc). Got appropriate form errors.
- Created valid items of each type.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12128
Differential Revision: https://secure.phabricator.com/D17235
Summary: Removes the often funny, but never really used but will cause us bug reports someday.... cat facts.
Test Plan: Install cat facts, run storage upgrade, see no cat facts in menu.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12126
Differential Revision: https://secure.phabricator.com/D17233
Summary: Mark required fields as required. Though in testing, none of these work.
Test Plan: Try to save a form without an app/project/dashboard and see success (not expected)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17231
Summary: Not sure this page is really providing any value, the timeline always says "edited this object" and there is a list of actions. Seems we could move actions back to the profile proper, but they feel very... engineery to me. Or we could fix the timeline stories, but my guess is they aren't useful or we would have gotten such feedback.
Test Plan: Review manage page, timeline is gone. Page is clean.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17230
Summary:
Fixes T6024. Ref T12121. Currently, we show build status in commit history tables; show audit status alongside it.
Also:
- Change the "Author/Committer" header to just "Author"; I think it's reasonably obvious what "x/y" means (if you can't guess, you can click the commit and likely figure it out) and this gives us a little more space.
- Make the audit list look more like the corresponding list in Differential, with similar formatting.
Test Plan:
- Viewed history of a repostiory, saw audit status.
- Viewed a merge commit, saw audit status in the list of merged commits.
- Viewed a commit search results list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12121, T6024
Differential Revision: https://secure.phabricator.com/D17227
Summary:
Ref T11096. Currently, editing ProfileMenuItemConfigurations always requires that you can edit the corresponding object.
This is correct for global items (for example: you can't change the global menu for a project unless you can edit the project) but not for personal items.
For personal items, only require that the user can edit the `customPHID` object. Today, this is always their own profile.
Test Plan: As a non-admin, edited personal menu items.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11096
Differential Revision: https://secure.phabricator.com/D17228
Summary:
To set this up:
- alice accepts a revision.
- Something adds a package or project she has authority over as a reviewer.
- Because alice has already accepted, she can not re-accept, but she should be able to (in order to accept on behalf of the new project or package).
Test Plan:
- Created a revision.
- Accepted as user "dog".
- Added "dog project".
- Re-accepted.
- Could not three-accept.
- Removed "dog project.
- Rejected.
- Added "dog project".
- Re-rejected.
- Could not three-reject.
Reviewers: chad, eadler
Reviewed By: chad, eadler
Differential Revision: https://secure.phabricator.com/D17226
Summary: Ref T10978. Handle loads can be batched a bit more efficiently by doing them upfront.
Test Plan: Queries dropped a bit locally, but I mostly have the same autors/auditors. I'm seeing 286 queries on my account in production, so I'll check what happens with that.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10978
Differential Revision: https://secure.phabricator.com/D17225
Summary: Builds out more UI to reinforce just who you are in this world... A perfect person.
Test Plan:
Look at myself a lot.
{F2435202}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17224
Summary: Fixes T5889. You can't write a rule like "if no other Herald rules did anything...", but you can use this rule to check for Owners or an explicit "Auditors" field doing things.
Test Plan: Using the test console, ran an "Auditors" rule against a commit with and without an auditor. Got expected pass/fail outcomes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5889
Differential Revision: https://secure.phabricator.com/D17221
Summary: Ref T5867. The `executeOne()` currently raises a policy exception if the application isn't visible to the viewer, or we fatal if the application has been uninstalled.
Test Plan:
- Viewed pages with the application uninstalled, saw working pages with no favorites menu.
- Viewed pages with the application restricted, saw working pages with no favorites menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17219
Summary: Fixes T12117. I typed or copy/pasted this constant wrong while refactoring during T10978.
Test Plan: Called `audit.query`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12117
Differential Revision: https://secure.phabricator.com/D17218
Summary:
- Attach objects when showing configuration screen
- Fix "Forms" to make more sense
- Alter EditEngine title to load correct name by loading object
Fixes T12116
Test Plan: Load up Apps/Projects/Forms on a configure menu, see proper names
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12116
Differential Revision: https://secure.phabricator.com/D17217
Summary: Ref T5867. Instead of hard-coding projects, tasks and repositories, let EditEngines say "I want a quick create item" so third-party code can also hook into the menu without upstream changes.
Test Plan: Saw same default items in menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17215
Summary: Ref T5867. I sure love Javascript.
Test Plan: Navigated between Home, Diffusion and Differential, opening the user profile menu. Saw appropraite help items.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17214
Summary: Ref T5867. Use a single query to load both personal and global items, then reorder them and add a divider if both groups have some stuff.
Test Plan: Viewed menu, edited personal and global items, viewed/edited existing project menus.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17213
Summary:
Still lots to fix here, punting up since I'm running into a few roadblocks.
TODO:
[] Sort Personal/Global correctly
[] Quicksand in Help Items correctly on page changes
Test Plan: Verify new menus work on desktop, tablet, mobile. Test logged in menus, logged out menus. Logging out via a menu, verify each link works as expected. Help menus get build when using an app like Maniphest, Differential. Check that search works, preferences still save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12107
Differential Revision: https://secure.phabricator.com/D17209
