Summary:
Ref T11476. This is a bit hacky, but makes `Application` extend `LiskDAO` so we can apply transactions to it with an `Editor` class.
Also fixes schema stuff so builds should produce a clean bill of health again.
This might only get you slightly further, yell if you run into more trouble.
Test Plan:
- Ran `bin/storage upgrade -f` and got no warnings.
- Browsed around, nothing exploded?
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11476
Differential Revision: https://secure.phabricator.com/D17738
Summary: Part of the groundwork for T11476.
Test Plan: ran `./bin/storage upgrade` and observed expected DB tables
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11476
Differential Revision: https://secure.phabricator.com/D17736
Summary: Sets notification and sound preferences in a single array()
Test Plan: Change email preference, save, set sound preference, save. Email preference still OK.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17735
Summary: Moves participants over to ModularTransactions, simplified a lot of the code. Fixes T12550
Test Plan:
Create a new room with just myself and myself + fake accounts.
Remove a person.
Remove myself.
Edit a room, topic.
Type some messages.
???
Profit
Reviewers: chad
Reviewed By: chad
Subscribers: Korvin
Maniphest Tasks: T12550
Differential Revision: https://secure.phabricator.com/D17685
Summary:
Pathway to D17685. We no longer have "behindTransactionPHID", so we no longer need the latest transaction.
This allows some code to be removed.
Test Plan:
- Grepped for callsites to `markUpToDate()` and variables used in the calls.
- Sent messages in a couple threads, viewed them, saw unread counts go away.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17733
Summary:
Pathway to D17685. This fixes an issue idenified in D17731: if any caller ever queried for more than one participant, some results could get thrown away by re-keying the results on thread PHID: two different participants can be members of the same thread!
This also fixes an issue from D17683, where a `needParticipantCache()` callsite was overlooked.
Test Plan:
- Viewed Conpherence dropdown.
- Sent messages, saw unread count / thread order still work properly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17732
Summary:
Pathway to D17685. This column is (mostly) a denormalization of `dateModified` on the thread.
Just use a JOIN instead.
This isn't //exactly// the same: we'll bump threads to the top now for non-message changes (e.g., a topic or title change). That seems fine, but we could put a `lastMessageDate` on Thread later if we want to refine it.
Also got rid of a lot of other unused stuff. There's a big garbage TODO here, I'll fix that in the next change.
Test Plan:
- Grepped for `dateTouched`.
- Grepped for `participantCursor`.
- Grepped for `ConpherenceParticipantQuery::LIMIT`.
- Looked for callsites to `setOrder()`, found none.
- Added a message to an older thread, saw it bump up to the top.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17731
Summary:
Pathway to D17685. This column is a very complicated cache of: is participant.messageCount equal to thread.messageCount?
We can just ask this question with a JOIN instead and simplify things dramatically.
Test Plan:
- Ran migration.
- Browsed around.
- Sent a message, saw unread count go up.
- Read the message, saw unread count go down.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17730
Summary: Pathway to D17685. Nothing reads this field and it has no use or value.
Test Plan:
- Ran migration.
- Grepped for `behindTransactionPHID`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17729
Summary: A few more mp3s to choose from for Conpherence.
Test Plan: Test each sound in a new room.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17734
Summary: Ref T7567. This adds some constants (for adding new sounds), global setting for turning on and off sound (setting) and per thread preference for sound choice. Also specc'd out Mentions, if added.
Test Plan: I tested all the preference wiring, but need to set up notifications locally to verify if this works. Feel free to test.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: amckinley, Korvin
Maniphest Tasks: T7567
Differential Revision: https://secure.phabricator.com/D17726
Summary:
Fixes T12596. A query for a token (like "having") which stems to a stopword (like "have") currently survives filtering. Stem it first so it gets caught.
Also, for InnoDB, a custom stopword table can be configured. If it is, read that instead of the default stopword list (I configured it locally, but the default list is reasonable so we never formally recommended installs configure it).
Test Plan:
Queried for words that stem to stopwords, saw them filtered:
{F4915843}
Queried for the original problem query and saw "having" caught with "have" in the stopword list:
{F4915844}
Fiddled with local InnoDB stopword table config and saw the stopword list get loaded correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12596
Differential Revision: https://secure.phabricator.com/D17728
Summary:
Ref T7567. In T8266 I fixed a bunch of obscure "Range" issues, but only for file downloads -- not for Celerity.
Extend all that stuff to Celerity, which is fortunately much easier.
I believe this will fix Conpherence sounds in Safari.
Test Plan:
- Wrote out an HTTP request in a text file with `Range: bytes=0-1` and similar, piped it to localhost with `cat request.txt | nc localhost 80`, saw server return appropriate range responses consistent with file behavior after T8266, which all seems to work.
- Also did that for files to try to make sure I wasn't breaking anything.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T7567
Differential Revision: https://secure.phabricator.com/D17724
Summary: Fixes T12587. Adds a new `PhabricatorFileDeleteTransaction` that enqueues `File` delete tasks.
Test Plan:
- hack `PhabricatorFileQuery` to ignore isDeleted state
- stop daemons
- upload a file, delete it from the UI
- check that the DB has updated isDeleted = 1
- check timeline rendering in `File` detail view
- start daemons
- confirm rows are deleted from DB
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, thoughtpolice
Maniphest Tasks: T12587
Differential Revision: https://secure.phabricator.com/D17723
Summary:
Ref T12566. Ref T12563. This fixes three bugs with Aphlict replay stuff:
First, Conphernece would try to repaint the UI even if no thread was open. Only repaint when a thread is open.
Second, although we deduplicate JX.Leader messages, we didn't deduplicate actual notification messages. If you browsed the leader window, then it re-elected itelf as a leader and replayed history, it could rebroadcast notifications and other windows could show doubles. Deduplicate notifications to prevent this.
Third, we always replayed the last 60 seconds of history. When you browsed the leader window, whichever window became the new leader (possibly the one you just browsed) could replay messages from before it had opened, leading to duplicate messages. Particularly, after receiving a message and then browsing you could see that message again. Instead, only replay history as far back as when the window first opened.
Test Plan:
- Clicked "Repaint" with a thread open, saw a repaint. Clicked "Repaint" with Conpherence open but no thread, no repaint and no 404 request to `/update/null/`.
- In browser A, opened three windows. In browser B, sent a notification. In browser A, browsed the leader window away twice in a row. Observed that the window which never became a leader doesn't duplicate notifications.
- In browser A, opened three windows. In browser B, sent a notification. In browser A, browsed the leader window away over and over again. Observed that replay requests issued with appropriate history windows.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12566, T12563
Differential Revision: https://secure.phabricator.com/D17722
Summary:
Deletion is a possibly time-intensive process, especially with large
files that are backed by high-latency, chunked storage (such as
S3). Even ~200mb objects take minutes to delete, which makes for an
unhappy experience. Fixes T10828.
Test Plan:
Delete a large file, and stare in awe of the swiftness with
which I am redirected to the main file application.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: thoughtpolice, Korvin
Maniphest Tasks: T10828
Differential Revision: https://secure.phabricator.com/D15743
Summary: Fixes T12579. Unclear why the user ran this command.
Test Plan: Ran with `--id cat`. Ran with `--id 123`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12579
Differential Revision: https://secure.phabricator.com/D17719
Summary: Ensures that newly-made `File` objects get indexed into the new ngrams index. Fixes T8788.
Test Plan:
- uploaded a file with daemons stopped; confirmed no new rows in ngrams table
- started daemons; confirmed indexing of previously-uploaded files happened
- uploaded a new file with daemons running; confirmed it got added to the index
Not sure how to test the changes to `PhabricatorFileUploadSource->writeChunkedFile()` and `PhabricatorChunkedFileStorageEngine->allocateChunks()`. I spent a few minutes trying to find their callers, but the first looks like it requires a Diffusion repo and the 2nd is only accessible via Conduit. I can test that stuff if necessary, but it's such a small change that I'm not worried about it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8788
Differential Revision: https://secure.phabricator.com/D17718
Summary: Follows the outline in D15656 for implementing ngram search for names of File objects. Also created FileFullTextEngine, because without implementing `PhabricatorFulltextInterface`, `./bin/search` complains that `File` is not an indexable type.
Test Plan:
- ran `./bin/storage upgrade` to apply the schema change
- confirmed the presence of a new `file_filename_ngrams` table
- added a couple file objects
- ran `bin/search index --type file --force`
- confirmed the presence of rows in `file_filename_ngrams`
- did a few keyword searches and saw expected results
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8788
Differential Revision: https://secure.phabricator.com/D17702
Summary: Check the strlen of topic before adding a tag to the header in Conpherence.
Test Plan: Remove a topic, no longer see indigo bubble.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17715
Summary: Ref T12566. When we reconnect, refresh the current thread even if we replayed notifications.
Test Plan:
- Clicked the "Repaint" button, saw the thread refresh.
- Clicked the "Reconnect" button, saw the thread reresh.
- Launched `aphlict debug`, killed it, restarted it, saw the thread refresh after reconnect.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12566
Differential Revision: https://secure.phabricator.com/D17713
Summary:
Fixes T12564. We already had some code which seems to deal with this properly, it just wasn't getting used.
Assign each application-level notification a unique ID, then ignore messages with duplicate IDs.
Test Plan:
- In browser A, loaded `/T123`.
- In browser B, loaded `/T123`.
- Made a comment as B.
- Saw notification as A.
- Mashed "Replay" a bunch.
- Before patch: piles of duplicate notifications.
- After patch: no duplicates.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12564
Differential Revision: https://secure.phabricator.com/D17710
Summary: Chrome and Safari both zoom in on form (input, select, textarea) when it thinks the text is too small (less than 16px... which is huge). This turns user-scalable off. The only drawback is double-tap to zoom will be disabled as well, but given we already responsively design, I don't think thats an issue.
Test Plan: iOS simulator on secure and local test instances. Click on an input, no longer see UI zoom in.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17714
Summary:
Fixes T12563. If we've ever seen an "open", mark all future connections as reconnects. When we reconnect, replay recent history.
(Until duplicate messages (T12564) are handled better this may cause some notification duplication.)
Also emit a reconnect event (for T12566) but don't use it yet.
Test Plan: {F4912044}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12563
Differential Revision: https://secure.phabricator.com/D17708
Summary:
Ref T12563. Before broadcasting messages from the server, store them in a history buffer.
A future change will let clients retrieve them.
Test Plan:
- Used the web frontend to look at the buffer, reloaded over time, sent messages. Saw buffer size go up as I sent messages and fall after 60 seconds.
- Set size to 4 messages, sent a bunch of messages, saw the buffer size max out at 4 messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12563
Differential Revision: https://secure.phabricator.com/D17707
Summary: Ref T12568. Ref T12567. Allows you to force a reconnect, and shows the reconnect delay on connection close/failure.
Test Plan: {F4911879}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12568, T12567
Differential Revision: https://secure.phabricator.com/D17705
Summary: Ref T12568. This begins building toward a more useful realtime debugging console for Leader/Aphlict/general realtime stuff.
Test Plan: {F4911521}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12568
Differential Revision: https://secure.phabricator.com/D17701
Summary: Ref T8788. See D17702. This allows `bin/search index` to index stuff which only implements `Ngrams`, not `Fulltext`.
Test Plan: Kinda poked around `bin/search index` a bit, yell if you hit more issues deeper down the stack?
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8788
Differential Revision: https://secure.phabricator.com/D17704
Summary: Fixes T12556 Uses more common components in ConpherenceThreadList by moving to PHUIListItemView. Reduces clutter by moving privacy into the header. Gets ride of "See More" double interchanges.
Test Plan:
I need to test this more, doesn't seem to auto-select top room any more, also might build a lipsum generator.
- Create lots of rooms with various policies
- Test clicking on policy object
- Click on different rooms
- Post in rooms
- Load up second account, see room numbers
- Clear room message count by clicking on room
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12556
Differential Revision: https://secure.phabricator.com/D17698
Test Plan: Created a phurl, added some comments, confirmed that "Change Subscribers" and "Change Project Tags" are now available in the comment form.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, Korvin
Maniphest Tasks: T11661
Differential Revision: https://secure.phabricator.com/D17686
Summary: Updates the language to use "Remove Participant" instead of "Banish User"
Test Plan: Read through the various cases, test them by removing myself or others
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17697
Summary:
I think these got munged when I removed CAN_JOIN.
- If you can view the room, you can join it.
- ~~If you can view the room, you can add others to it.~~ This rule adjustment was removed, see discussion on the revision.
- If you are a participant in the room, you can remove yourself.
- If you can edit a room, you can remove anyone.
Test Plan:
Normal feature set:
- Create a new room that only I can edit, viewable by all users.
- Leave room (bye k thx)
- Create another room, myself only
- Join room from second account
- See ability to only remove myself
- Remove myself
- Rejoin
- Add third account
- Log into first account
- Boot off randos
- Test joining by green button, message, and by + sign.
Policy consistency:
- As a user who can not edit the room, tried to add other members. Received policy exception. The `+` button is currently visible and enabled for all users (even users who have not joined the room) but this is pre-existing.
Reviewers: chad
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17696
Summary:
Ref T12451. This is a GREAT comment (A++) but we only need one copy of it.
This uses a pattern similar to Projects, which is a little weird but works well enough.
Test Plan:
- Viewed all four tabs of an account.
- Viewed a page with a bad account ID which 404'd properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17694
Summary: Ref T12451. This code is the same as the other code.
Test Plan: Went through the default-account case with this code, worked the same as the other code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17693
Summary:
Ref T12451. Ref T12484. This should deal with all the `+` / `-` / `=` cases correctly, I think.
Also makes sure that members are real users, not commits or tokens or whatever. And expands the creation test case to make some other basic sanity checks.
Test Plan:
- Went through implicit first-time creation flow.
- Went through explicit second-time creation flow.
- Unit test now passes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12484, T12451
Differential Revision: https://secure.phabricator.com/D17692
Summary:
Ref T12451. Ref T12484. I think D17657 fixed this, but caused the bug in D17690. The fix for that causes this bug again.
Put a unit test on it. This test currently fails; I'll correct the bug in the next change.
Test Plan: Ran `arc unit`, saw a failure.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12484, T12451
Differential Revision: https://secure.phabricator.com/D17691
Summary:
Ref T12451. When you explicitly created a second or third account or whatever, you wouldn't be added as a member.
(The editor sees that you're "already a member", so it doesn't add you.)
Test Plan:
- Go to `/phortune/`.
- Click "Switch Accounts".
- Click "Create Account".
- Create an account.
- Before patch: unable to view it since you don't get added as a member.
- After patch: account created with you as member.
- Also created an accont with multiple members.
- Tried to create an account with no members.
- Tried to create an account with just someone else as a member.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17690
Summary: Ref T12451. `$this->getAccount()` may not return an account.
Test Plan:
- Visit `/phortune/X/`, where `X` is the ID of an account you don't have permission to view.
- Before patch: fatal.
- After patch: normal policy exception page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17689
Summary:
Via HackerOne (<https://hackerone.com/reports/220909>). When we close commits in response to "Fixes Txxx", we currently act as the omnipotent user. This allows users to close tasks they can't see by pushing commits with "Fixes Txxx" in the message.
However, we can't actually tell who authored or committed a change: we're just using the "Author" and "Committer" values from Git in most cases, and anyone can forge those. So we can't really get this right, in a security sense.
(We can tell who //pushed// a change if we host it, but that's often not the right user. If GPG signing was more prevalent, we could use that. In the future, we could use side channels like having `arc land` tell Phabrcator who was pushing changes.)
Since I think the impact of this is fairly minor and this isn't //really// a security issue (more of a confusion/abuse/product issue) I think the behavior is okay more-or-less as-is, but we can do better when we do identify an author: drop permissions, and use their privileges to load the tasks which the commit "fixes".
This effectively implements this rule:
> If we identify the author of a commit as user X, that commit can only affect tasks which user X can see and edit.
Note that:
- Commits which we can't identify the author for can still affect any task.
- Any user can forge any other user's identity (or an invalid identity) and affect any task.
So this is just a guard rail to prevent mistakes by good-faith users who type the wrong task IDs, not a real security measure.
Also note that to perform this "attack" you must already have commit access to a repository (or permission to create a repository).
Test Plan:
- Used `bin/repository reparse --message <commit> --force-autoclose` to run the relevant code.
- Made the code `throw` before it actually applied the edit.
- Verified that the edit was rejected if the author was recognized and can not see or could not edit the task.
- Verified that the edit is accepted if the author can see+edit the task.
- Verified that the edit is accepted if we can't figure out who the author is.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17688
Summary:
Fixes T12554. The SSH key cache contains usernames, but is not currently dirtied on username changes.
An alternative solution would be to use user PHIDs instead of usernames in the file, which would make this unnecessary, but that would make debugging a bit harder. For now, I think this small added complexity is worth the easier debugging, but we could look at this again if cache management gets harder in the future.
Test Plan:
- Added a key as `ducksey`, ran `bin/ssh-auth`, saw key immediately.
- Renamed `ducksey` to `ducker`, ran `bin/ssh-auth`, saw username change immediately.
- Added another key as `ducker`, ran `bin/ssh-auth`, saw key immediately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12554
Differential Revision: https://secure.phabricator.com/D17687
Summary: Also fixes insufficiently-escaped regex examples
Test Plan: Made several changes to http://local.phacility.com/config/edit/syntax.filemap/ and observed validation failures on malformed regexes, and success on well-formed regexes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12532
Differential Revision: https://secure.phabricator.com/D17684
Test Plan:
Created new paste with title '.arcconfig' without choosing a language; observed that the paste gets highlighted as JSON.
JSON mode:
{F4901762}
Javascript mode:
{F4901763}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11667
Differential Revision: https://secure.phabricator.com/D17682
Summary: We no longer display this any more in the UI, so go ahead and remove the callsites and db column.
Test Plan: New Room, with and without participants.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17683
Summary: removes old phabricator.com/countdown/{id} route and code that uses that URL scheme
Test Plan: loaded phabricator.com/countdown, verified that generated links point to phabricator.com/CXXX
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12524
Differential Revision: https://secure.phabricator.com/D17681
Summary: Swaps this transaction over.
Test Plan: Load up a few rooms with date markers, still render as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12550
Differential Revision: https://secure.phabricator.com/D17680
Summary: Will see how this goes in practice. Uses violet where color is used for non responsive peeps.
Test Plan: Create a user without email verification, test hover card, profile, mentions and lists.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17678
Summary: This moves the count on the Conpherence Menu Item into a phui-list-item-count, and removes the CSS call to the entire Conphrence stack when durable column is open.
Test Plan: Test with and without the chat column, and a menu with a count
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17677
Summary: Fixes T12178, Fixes T11704 Not sure this feature gets any use and I can't find a similar option in other software, so removing it I think simiplifies a number of things. Removes CAN_JOIN and joinable is basically now CAN_VIEW and !$participating. Also removed some old transaction strings for other policies. Don't seem used.
Test Plan: Create a new room, edit room policies, see changes. Log into second account, search for rooms, everything now is visible.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12178, T11704
Differential Revision: https://secure.phabricator.com/D17675
Summary: Does a few things. Turns off feed stories (again), removes "action" transactions from notificiations, and only updates message count on actual messages. This feels a bit cleaner and less spammy... I guess... I think @epriestley will really like it and do me a favor or something.
Test Plan: Pull up two windows. test a message, see message count on second screen. Edit a topic or title, get no notification. At all. Ever.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D17674
Summary:
Ref T12137. If a database is missing the InnoDB or MyISAM table engines, the big combined query to get both will fail.
Instead, try InnoDB first and then MyISAM.
(I have both engines locally so this worked until I deployed it.)
Test Plan: Faked an InnoDB error like `secure`, got a MyISAM result.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12137
Differential Revision: https://secure.phabricator.com/D17673
Summary:
Depends on D17670. Fixes T12137. Fixes T12003. Ref T2632.
This shows users a readout of which terms were actually searched for.
This also drops those terms from the query we submit to the backend, dodging the weird behaviors / search engine bugs in T12137.
This might need some design tweaking.
Test Plan: {F4899825}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12137, T12003, T2632
Differential Revision: https://secure.phabricator.com/D17672
Summary:
Depends on D17669. Ref T12137. Ref T12003. Ref T2632. Ref T7860.
Converts Phabricator to the new parse + compile workflow with intermediate tokens.
Also fixes a bug where searches for `cat"` or similar (unmatched quotes) wouldn't produce a nice exception.
Test Plan:
- Fulltext searched.
- Fulltext searched in Conpherence.
- Fulltext searched with bad syntax.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12137, T12003, T7860, T2632
Differential Revision: https://secure.phabricator.com/D17670
Summary:
Fixes T8285. Fulltext search relies on an underlying engine which can not realistically use cursor paging. This is unusual and creates some oddness.
Tweak a few numbers -- and how offsets are handled -- to separate the filtered offset and unfiltered offset.
Test Plan:
- Set page size to 2.
- Ran a query.
- Paged forward and backward through results sensibly, seeing the full result set.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8285
Differential Revision: https://secure.phabricator.com/D17667
Summary: Begin converting Conpherence to ModularTransactions, this converts title, topic, and picture to use modular transactions. Participants seems hairy so I'll do that in another diff
Test Plan: Create a room with a topic, change room name, topic. Add people, remove people. Set a room image. Unset topic.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17668
Summary: Fixes T11730. Removes an old transaction that hasn't been used in a year.
Test Plan: Run sql, check various rooms.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11730
Differential Revision: https://secure.phabricator.com/D17666
Summary: looked for places where Countdown monograms/uris were being constructed by hand, and updated with modern versions
Test Plan: clicked around the Countdown UI, looking for broken links
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, Korvin
Maniphest Tasks: T12524
Differential Revision: https://secure.phabricator.com/D17665
Summary: In Conpherence ProfileMenuItem we show an unread count if you're a participant, but all message count if you're not. Just remove that.
Test Plan: Log out of room in Conpherence, leave messages on second account, check menu item on both accounts.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17664
Summary: Builds a Conpherence Profile Menu Item, complete with counts for the unreads. This allows pinning to home as well as swapping out thread list in Conpherence for pinning eventually.
Test Plan: Add a menu item, chat in room, log into other account, see room count. Room count disappears after viewing.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17662
Summary: Removes this feature, makes creating a room simpler and less confusing.
Test Plan: Create a room on Conpherence.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17661
Summary: Primarily, this splits individual sections of the single account page into a more managable and robust sidenav for subscriptions, billing, and managers. The functionality on the subpages is light, but I expect to build on then in coming diffs. This also starts building out a more effective "status" area on the lead page.
Test Plan:
- Load up default account
- Make some edits
- Click on each of the new navigation items
- Verify links to "see all" work
- Test overdue and no payment states for status
{F4337317}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17589
Summary: There is currently a validation error triggered if you initialize a new account without a member set. I think this is the correct fix, but let me know.
Test Plan: truncate phortune_account database, navigate to phortune, see account automatically created to "Default Account".
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17657
Summary: Fixes T12541. `describeAutomaticCapability()` is no longer required to implement `PolicyInterface`. Use PolicyCodex instead.
Test Plan: {F4889642}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12541
Differential Revision: https://secure.phabricator.com/D17658
Summary: Builds out Phortune Merchant pages to have a sidenav and sub-pages for further expansion. For now this links Orders and Subscriptions to the query engine pages, but could be split out to be more informative (unpaid, upcoming, etc).
Test Plan:
Create a new merchant, edit some information, add a manager in new UI, edit logo, click through to subscriptions, orders.
{F4883013}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17655
Summary: This updates the backend of PhortuneAccount to use EditEngine and Modular Transactions and updates language to "account manager" for clarity of role.
Test Plan:
- Wiped `phortune_account` table
- Visit Phortune, see new account automatically created.
- Edit name and managers
- Try to set no name or remove myself as a manager, get error messages
- Visit `/phortune/` and create another new account
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17585
Summary: Fixes T12536. Nothing reads this parameter; `PhabricatorFile::newChunkedFile` sets the `isPartial` flag automatically.
Test Plan: Grepped for `isPartial`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12536
Differential Revision: https://secure.phabricator.com/D17654
Summary:
Previously, "reject" and "reject older" were separate statuses. Now, they're both shades of "reject".
Set the "older reject" flag properly when we find a non-current reject.
Test Plan:
- User A accepts a revision.
- User B rejects it.
- Author updates it.
- Before patch: incorrectly transitions to "accepted" ("older" reject is ignored).
- After patch: correctly transitions to "needs review".
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17653
Summary: Modernize PhortuneMerchant for Modular Transactions. Also changed the language of "Members" to "Managers", which I think fits better given the power/capability.
Test Plan:
- Create a new Merchant
- Test not filling in a name, see error
- Test removing myself, see error
- Edit an existing Merchant
- Add new managers
- Test removing myself, see error
- Replace Picture
- Update various fields, contact info, email, footer
- Verify transactions are now nice and pretty
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17584
Summary:
Fixes T12531. Strictness fallout from adding typechecking in D17616.
- `chunkedHash` is not a real parameter, so the new typechecking was unhappy about it.
- `mime-type` no longer allows `null`.
Test Plan:
- Ran `arc upload --conduit-uri ... 12MB.zero` on a 12MB file full of zeroes.
- Before patch: badness, failure, fallback to one-shot uploads.
- After patch: success and glory.
Reviewers: chad
Subscribers: joshuaspence
Maniphest Tasks: T12531
Differential Revision: https://secure.phabricator.com/D17651
Summary: Fixes T12138. Test for the presence of being in fullscreen mode, and disable send on enter if present. Side note, I'd love a first class "hasClass" type Javelin function.
Test Plan:
- Go to Conpherence
- Type some smack, see it send on enter
- Go fullscreen like a boss
- Let the words flow
- Close fullscreen, then send on enter.
- (might be nice someday to add a "submit" button to fullscreen editor)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12138
Differential Revision: https://secure.phabricator.com/D17590
Summary:
Fixes T12356.
- In this mail, we currently render "6:00 AM". Instead, render "6:00 AM (PDT)" or similar. This is consistent with times in other modern Transaction mail.
- Previously, we would render "UTC-7". Render "PDT" instead. For obscure zones with no known timezone abbreviation, fall back to "UTC-7".
Test Plan:
- Used `bin/calendar notify --minutes X` to trigger notifications, read email bodies.
- Used this script to list all `T` values and checked them for sanity:
```lang=php
<?php
$now = new DateTime();
$locales = DateTimeZone::listIdentifiers();
foreach ($locales as $locale) {
$zone = new DateTimeZone($locale);
$now->setTimeZone($zone);
printf(
"%s (%s)\n",
$locale,
$now->format('T'));
}
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12356
Differential Revision: https://secure.phabricator.com/D17646
Summary:
Ref T11816. Depends on D17644. When you executed a query like "upcoming, limit 5 events" you might match some recurring events starting from, say, a year ago and repeating every month.
We'd then generate the first 5 ghosts for these events (say, last January, February, ... May) and later throw them out, so the correct events in the query window (say, this April) would never get generated.
Instead, generate ghosts beginning with the start of the window. The fix in D17644 to number results correctly allows us to do this.
Test Plan:
- Made a query panel showing 5 events, scheduled an event long in the past, did not visit any of the instances of it so they didn't generate concrete objects.
- Before the patch, near-future instances failed to show; after the patch, they show.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D17645
Summary:
Ref T11816. Two minor issues:
- We used `$event`, not `$next_event`, as the event providing the PHID for "Busy at <event name>". This rendered "Busy at <most future event>" on the profile instead of "Busy at <next upcoming event".
- The TTL computation used the event start, not the event end, so we could end up rebuilding the cache too often for users busy at an event.
Test Plan:
- Attended an event in the near future and one later on.
- Saw profile now say "busy at <near future event>" correctly.
- In DarkConsole "Services" tab, no longer saw unnecessary cache refills while attending an event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D17643
Summary: Fixes T11561. Collect guidance about local configuration which hasn't been obvious in the past.
Test Plan:
- Read document carefully.
- Used `./bin/diviner generate` to generate documentation.
- Previewed in Diviner locally:
{F4795021}
Reviewers: amckinley, chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T11561
Differential Revision: https://secure.phabricator.com/D17641
Summary:
Even with `innodb_file_per_table` enabled, individual table files on disk don't normally shrink.
For most tables, like `maniphest_task`, this is fine, since the data in the table normally never shrinks, or only shinks a tiny amount.
However, some tables (like the "worker" and "daemon" tables) grow very large during a huge import but most of the data is later deleted by garbage collection. In these cases, this lost space can be reclaimed by running `OPTIMIZE TABLE` on the tables.
Add a script to `OPTIMIZE TABLE` every table.
My primary goal here is just to reduce storage pressure on `db001` since there are a couple of "import the linux kernel" installs on that host wasting a bunch of space. We're not in any trouble, but this should buy us a good chunk of headroom.
Test Plan: Ran `bin/storage optimize` locally and manually ran `OPTIMIZE TABLE` in production, saw tables get optimized.
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Differential Revision: https://secure.phabricator.com/D17640
Summary: Ref T4245. We disallow `/diffusion/` in robots.txt already because indexers tend to get lost blaming every line of every file throughout history, but didn't update the list for the `/source/` alias. Update it.
Test Plan: Visited `/robots.txt` locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D17637
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.
Test Plan: `grep`, browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17632
Summary:
Ref T12509. Many of the calls to HMAC+SHA1 are just to compute cachekeys for remarkup objects.
Make these use HMAC+SHA256 instead. There is no downside to swapping these since they just cause a cache miss in the worst case.
I also plan to get rid of `PhabricatorMarkupInterface` eventually, but this doesn't go that far.
Test Plan: Browsed some different types of documents (tasks, legalpad documents, phame blogs / posts, pholio mocks, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17631
Summary:
Ref T12509. This adds support for HMAC+SHA256 (instead of HMAC+SHA1). Although HMAC+SHA1 is not currently broken in any sense, SHA1 has a well-known collision and it's good to look at moving away from HMAC+SHA1.
The new mechanism also automatically generates and stores HMAC keys.
Currently, HMAC keys largely use a per-install constant defined in `security.hmac-key`. In theory this can be changed, but in practice essentially no install changes it.
We generally (in fact, always, I think?) don't use HMAC digests in a way where it matters that this key is well-known, but it's slightly better if this key is unique per class of use cases. Principally, if use cases have unique HMAC keys they are generally less vulnerable to precomputation attacks where an attacker might generate a large number of HMAC hashes of well-known values and use them in a nefarious way. The actual threat here is probably close to nonexistent, but we can harden against it without much extra effort.
Beyond that, this isn't something users should really have to think about or bother configuring.
Test Plan:
- Added unit tests.
- Used `bin/files integrity` to verify, strip, and recompute hashes.
- Tampered with a generated HMAC key, verified it invalidated hashes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17630
Summary:
Ref T12470. Provides an "integrity" utility which runs in these modes:
- Verify: check that hashes match.
- Compute: backfill missing hashes.
- Strip: remove hashes. Useful for upgrading across a hash change.
- Corrupt: intentionally corrupt hashes. Useful for debugging.
- Overwrite: force hash recomputation.
Users normally shouldn't need to run any of this stuff, but this provides a reasonable toolkit for managing integrity hashes.
I'll recommend existing installs use `bin/files integrity --compute all` in the upgrade guidance to backfill hashes for existing files.
Test Plan:
- Ran the script in many modes against various files, saw expected operation, including:
- Verified a file, corrupted it, saw it fail.
- Verified a file, stripped it, saw it have no hash.
- Stripped a file, computed it, got a clean verify.
- Stripped a file, overwrote it, got a clean verify.
- Corrupted a file, overwrote it, got a clean verify.
- Overwrote a file, overwrote again, got a no-op.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12470
Differential Revision: https://secure.phabricator.com/D17629
Summary:
Ref T12298. The PullLocal daemon has had hibernation code for a little while, but it never actually activated because we don't sleep for more than 15 seconds in any case.
Add a maximum sleep instead and use that to control the longest sleep we'll do for hibernation purposes.
Also, when a repository or repository URI is edited, write a NEEDS_UPDATE event into the message table to make sure the daemons de-hibernate.
Test Plan: Used `bin/phd debug pull`, saw the daemon actually hibernate instead of just sleeping for 15 seconds.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17635
Summary:
Ref T12272. I wrote this correctly, then broke it by adding the simplification which treats "accept the defaults" as "accept everything".
This simplification lets us render "epriestley accepted this revision." instead of "epriestley accepted this revision onbehalf of: long, list, of, every, default, reviewer, they, have, authority, over." so it's a good thing, but make it only affect the reviewers it's supposed to affect.
Test Plan:
- Did an accept with a force-accept available but unchecked.
- Before patch: incorrectly accepted all possible reviewers.
- After patch: accepted only checked reviewers.
- Also checked the force-accept box, accepted, got a proper force-accept.
Reviewers: chad, lvital
Reviewed By: lvital
Maniphest Tasks: T12272
Differential Revision: https://secure.phabricator.com/D17634
Summary: Allow API callers to retrieve reviewer information via a new "reviewers" attachment.
Test Plan: {F4675784}
Reviewers: chad, lvital
Reviewed By: lvital
Subscribers: lvital
Differential Revision: https://secure.phabricator.com/D17633
Summary: Fixes T12508. Files don't have an `editPolicy`, and we started actually checking that the keys are real things in D17616.
Test Plan:
- Before patch: created a paste, got an "editPolicy" exception.
- After patch: created a paste that worked properly.
Reviewers: avivey, chad
Reviewed By: avivey
Maniphest Tasks: T12508
Differential Revision: https://secure.phabricator.com/D17628
Summary: Ref T12219. Chrome can send requests with a "Range: bytes=0-" header, which just means "the whole file", but we don't respond correctly because of a `null` vs `0` issue.
Test Plan: Sent a raw `bytes=0-` request, saw a proper resonse.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12219
Differential Revision: https://secure.phabricator.com/D17627
Summary:
Ref T12219. We currently only support Range requests like "bytes=123-456", but "bytes=123-", meaning "until end of file", is valid, and Chrome can send these requests.
I suspect this is the issue with T12219.
Test Plan: Used `nc local.phacility.com 80` to pipe raw requests, saw both "bytes=123-456" and "bytes=123-" requests satisfied correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12219
Differential Revision: https://secure.phabricator.com/D17626
Summary:
Ref T12470. This helps defuse attacks where an adversary can directly take control of whatever storage engine files are being stored in and change data there. These attacks would require a significant level of access.
Such attackers could potentially attack ranges of AES-256-CBC encrypted files by using Phabricator as a decryption oracle if they were also able to compromise a Phabricator account with read access to the files.
By storing a hash of the data (and, in the case of AES-256-CBC files, the IV) when we write files, and verifying it before we decrypt or read them, we can detect and prevent this kind of tampering.
This also helps detect mundane corruption and integrity issues.
Test Plan:
- Added unit tests.
- Uploaded new files, saw them get integrity hashes.
- Manually corrupted file data, saw it fail. Used `bin/files cat --salvage` to read it anyway.
- Tampered with IVs, saw integrity failures.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12470
Differential Revision: https://secure.phabricator.com/D17625
Summary: Ref T8266. Although we compute this correctly above, we ignored it when actually setting the header. Use the computed value to set the "Content-Length" header. This is consistent with the spec/documentation.
Test Plan: Before, some audio (like `rain.mp3`) was pretty spotty about loading in Safari. It now loads consistently for me locally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8266
Differential Revision: https://secure.phabricator.com/D17624
Summary:
Fixes T12079. Currently, when a file is encrypted and a request has "Content-Range", we apply the range first, //then// decrypt the result. This doesn't work since you can't start decrypting something from somewhere in the middle (at least, not with our cipher selection).
Instead: decrypt the result, //then// apply the range.
Test Plan: Added failing unit tests, made them pass
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12079
Differential Revision: https://secure.phabricator.com/D17623
The root issue here is actually just that I cherry-picked stable locally
but did not push it. However, this is a minor issue I also caught while
double-checking things.
Auditors: chad
Summary:
Ref T12464. This defuses any possible SHA1-collision attacks by using SHA256, for which there is no known collision.
(SHA256 hashes are larger -- 256 bits -- so expand the storage column to 64 bytes to hold them.)
Test Plan:
- Uploaded the same file twice, saw the two files generate the same SHA256 content hash and use the same underlying data.
- Tried with a fake hash algorihtm ("quackxyz") to make sure the failure mode worked/degraded correctly if we don't have SHA256 for some reason. Got two valid files with two copies of the same data, as expected.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17620
Summary:
Ref T12464. We currently use SHA1 to detect when two files have the same content so we don't have to store two copies of the data.
Now that a SHA1 collision is known, this is theoretically dangerous. T12464 describes the shape of a possible attack.
Before replacing this with something more robust, shore things up so things work correctly if we don't hash at all. This mechanism is entirely optional; it only helps us store less data if some files are duplicates.
(This mechanism is also less important now than it once was, before we added temporary files.)
Test Plan: Uploaded multiple identical files, saw the uploads work and the files store separate copies of the same data.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17619
Summary:
Ref T12464. This is a very old method which let you create a file on the server by referring to data which already existed in another file.
Basically, long ago, `arc` could say "Do you already have a file with hash X?" and just skip some work if the server did.
`arc` has not called this method since D13017, in May 2015.
Since it's easy to do so, just make this method pretend that it never has the file. Very old clients will continue to work, since they would expect this response in the common case and continue by uploading data.
Test Plan:
- Grepped for `uploadhash` in Phabricator and Arcanist.
- Called the method with the console, verified it returned `null`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17618
Summary:
Ref T12464. This is a very old method which can return an existing file instead of creating a new one, if there's some existing file with the same content.
In the best case this is a bad idea. This being somewhat reasonable predates policies, temporary files, etc. Modern methods like `newFromFileData()` do this right: they share underlying data in storage, but not the actual `File` records.
Specifically, this is the case where we get into trouble:
- I upload a private file with content "X".
- You somehow generate a file with the same content by, say, viewing a raw diff in Differential.
- If the diff had the same content, you get my file, but you don't have permission to see it or whatever so everything breaks and is terrible.
Just get rid of this.
Test Plan:
- Generated an SSH key.
- Viewed a raw diff in Differential.
- (Did not test Phragment.)
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T12464
Differential Revision: https://secure.phabricator.com/D17617
Summary:
Ref T11357. When creating a file, callers can currently specify a `ttl`. However, it isn't unambiguous what you're supposed to pass, and some callers get it wrong.
For example, to mean "this file expires in 60 minutes", you might pass either of these:
- `time() + phutil_units('60 minutes in seconds')`
- `phutil_units('60 minutes in seconds')`
The former means "60 minutes from now". The latter means "1 AM, January 1, 1970". In practice, because the GC normally runs only once every four hours (at least, until recently), and all the bad TTLs are cases where files are normally accessed immediately, these 1970 TTLs didn't cause any real problems.
Split `ttl` into `ttl.relative` and `ttl.absolute`, and make sure the values are sane. Then correct all callers, and simplify out the `time()` calls where possible to make switching to `PhabricatorTime` easier.
Test Plan:
- Generated an SSH keypair.
- Viewed a changeset.
- Viewed a raw diff.
- Viewed a commit's file data.
- Viewed a temporary file's details, saw expiration date and relative time.
- Ran unit tests.
- (Didn't really test Phragment.)
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17616
Summary:
Ref T11357. In D17611, I added `file.search`, which includes a `"dataURI"`. Partly, this is building toward resolving T8348.
However, in some cases you can't GET this URI because of a security measure:
- You have not configured `security.alternate-file-domain`.
- The file isn't web-viewable.
- (The request isn't an LFS request.)
The goal of this security mechanism is just to protect against session hijacking, so it's also safe to disable it if the viewer didn't present any credentials (since that means there's nothing to hijack). Add that exception, and reorganize the code a little bit.
Test Plan:
- From the browser (with a session), tried to GET a binary data file. Got redirected.
- Got a download with POST.
- From the CLI (without a session), tried to GET a binary data file. Go a download.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17613
Summary: Ref T11357. Implements a modern `file.search` for files, and freezes `file.info`.
Test Plan: Ran `file.search` from the Conduit console.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17612
Summary:
Ref T11357. This moves editing and commenting (but not creation) to EditEngine.
Since only the name is really editable, this is pretty straightforward.
Test Plan: Renamed files; commented on files.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17611
Summary: Ref T11357. A lot of file creation doesn't go through transactions, so we only actually have one real transaction type: editing a file name.
Test Plan:
Created and edited files.
{F4559287}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17610
Summary:
Fixes T12502. This transaction probably should not be getting picked for feed rendering, but it currently does get selected in some cases.
This should probably be revisited eventually (e.g., when Maniphest moves to ModularTransactions) but just fix the brokenness for now.
Test Plan:
- Created a task in a space.
- Viewed feed.
- Saw the story render with readable text.
{F4555747}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12502
Differential Revision: https://secure.phabricator.com/D17609
Summary:
Fixes T12496. Sticky accept was accidentally impacted by the "void" changes in D17566.
Instead, don't always downgrade all accepts/rejects: on update, we only want to downgrade accepts.
Test Plan:
- With sticky accept off, updated an accepted revision: new state is "needs review".
- With sticky accept on, updated an accepted revision: new state is "accepted" (sticky accept working correctly).
- Did "reject" + "request review" to make sure that still works, worked fine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12496
Differential Revision: https://secure.phabricator.com/D17605
Summary:
Fixes T12461. This returns the field as a dictionary with a `"raw"` value, so we could eventually do this if we want without breaking the API:
```
{
"type": "remarkup",
"raw": "**raw**",
"html": "<strong>raw</strong>",
"text": "raw"
}
```
Test Plan: Called `maniphest.search`, reviewed output.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12461
Differential Revision: https://secure.phabricator.com/D17603
Summary: Ref T12450. These are like 95% my fault, but Elastic appears to spell the name "Elasticsearch" consistently in their branding.
Test Plan: `grep ElasticSearch`
Reviewers: chad, 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17601
Summary:
Ref T12450. Currently, if a write fails, we stop and don't try to write to other index services. There's no technical reason not to keep trying writes, it makes some testing easier, and it would improve behavior in a scenario where engines are configured as "primary" and "backup" and the primary service is having some issues.
Also, make "no writable services are configured" acceptable, rather than an error. This state is probably goofy but if we want to detect it I think it should probably be a config-validation issue, not a write-time check. I also think it's not totally unreasonable to want to just turn off all writes for a while (maybe to reduce load while you're doing a background update).
Test Plan:
- Configured a bad ElasticSearch engine and a good MySQL engine.
- Ran `bin/search index ... --force`.
- Saw MySQL get updated even though ElasticSearch failed.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17599
Summary:
Ref T12450. We track a "document version" for updating search indexes, so that if a document is rapidly updated many times in a row we can skip most of the work.
However, this version doesn't consider "cluster.search" configuration, so if you add a new service (like a new ElasticSearch host) we still think that every document is up-to-date. When you run `bin/search index` to populate the index (without `--force`), we just do nothing.
This isn't necessarily very obvious. D17597 makes it more clear, by printing "everything was skipped and nothing happened" at the end.
Here, fix the issue by considering the content of "cluster.search" when computing fulltext document versions: if you change `cluster.search`, we throw away the version index and reindex everything.
This is slightly more work than we need to do, but changes to "cluster.search" are rare and this is much easier than trying to individually track which versions of which documents are in which services, which probably isn't very useful anyway.
Test Plan:
- Ran `bin/search index --type project`, saw everything get skipped.
- Changed `cluster.search`.
- Ran `search index` again, saw everything get updated.
- Ran a third time without changing `cluster.search`, everything was properly skipped.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17598
Summary:
Ref T12450. There's currently a bad behavior where inserting a document into one search service marks it as up to date everywhere.
This isn't nearly as obvious as it should be because `bin/search index` doesn't make it terribly clear when a document was skipped because the index version was already up to date.
When running `bin/seach index` without `--force` or `--background`, keep track of updated vs not-updated documents and print out some guidance. In other configurations, try to provide more help too.
Test Plan: {F4452134}
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17597
Summary:
Ref T12450. This was added a very very long time ago (D2298).
I don't want to put this in the upstream index anymore because I don't want to encourage third parties to develop software which reads the index directly. Reading the index directly is a big skeleton key which bypasses policy checks.
This was added before much of the policy model existed, when that wasn't as much of a concern. On a tecnhnical note, this also doesn't update when `phabricator.base-uri` changes.
This can be written as a search index extension if an install relies on it for some bizarre reason, although none should and I'm unaware of any actual use cases in the wild for it, even at Facebook.
Test Plan: Indexed some random stuff into ElasticSearch.
Reviewers: chad, 20after4
Reviewed By: chad
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17600
Summary:
Ref T12450. General adjustments:
- Try to make "Cluster: Search" more about "stuff in common + types" instead of pretty much all being Elastic-specific, so we can add Solr or whatever later.
- Provide guidance about rebuilding indexes after making a change.
- Simplify the basic examples, then provide a more advanced example at the ed.
- Really try to avoid suggesting anyone configure Elasticsearch ever for any reason.
Test Plan: Read documents, previewed in remarkup.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17602
Summary:
D17384 added a "keywords" field but only partially implemented it.
- Remove this field.
- Index project slugs as part of the document body instead.
Test Plan:
- Ran `bin/search index PHID-PROJ-... --force`.
- Found project by searching for a unique slug.
Reviewers: chad, 20after4
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17596
Summary:
If you have `maniphest.custom-field-definitions` set to include "required" fields, a bunch of tests which create tasks can fail.
To avoid this, reset this config while running tests.
This mechanism should probably be more general (e.g., reset all config by default, only whitelist some config) but just fix this for now since it's a one-liner and doesn't make eventual cleanup any harder.
Test Plan: Ran `arc unit`, hitting tests that create tasks.
Reviewers: chad, 20after4
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17595
Summary: When building a tokenizer-based edit control for a custom field (e.g. a datasource type), preserve a field validation error whilst building edit controls.
Test Plan:
- Create custom datasource field, set it to required
- Observe that 'Required' does not appear next to control
- Apply patch
- Observe 'Required' appears next to control
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17592
Summary: Minor, uses 'user-circle' for account, and merchant logo for merchants in lists.
Test Plan: View the landing page, see updated logos and icons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17586
Summary: Move individual controller files into cooresponding folders. Makes it easier to locate sections and expand without clutter. Also made "chargelist" part of account since it's tied to having an account specifically.
Test Plan: Vist charges, merchants, subscription, accounts, and other pages. No errors from file move.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17587
Summary:
Two little issues
1. there was an extra call to getHostForWrite,
2. The engine instance was shared between multiple service definitions so it
was overwriting the list of writable hosts from one service with hosts from another.
Test Plan:
tested in wikimedia production with multiple services defined like this:
```language=json
[
{
"hosts": [
{
"host": "search.svc.codfw.wmnet",
"protocol": "https",
"roles": {
"read": true,
"write": true
},
"version": 5
}
],
"path": "/phabricator",
"port": 9243,
"type": "elasticsearch"
},
{
"hosts": [
{
"host": "search.svc.eqiad.wmnet",
"protocol": "https",
"roles": {
"read": true,
"write": true
},
"version": 5
}
],
"path": "/phabricator",
"port": 9243,
"type": "elasticsearch"
}
]
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17581
Summary:
Elasticsearch really wants a raw json body and it fails to accept
the request as of es version 5.3
Test Plan:
Tested with elasticsearch 5.2 and 5.3.
Before this change 5.2 worked but 5.3 failed with
`HTTP/406 "Content-Type header [application/x-www-form-urlencoded] is not supported"` [1]
After this change, both worked.
[1] https://phabricator.wikimedia.org/P5158
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17580
Summary:
These exception messages & comments didn't quite match reality.
Fixed and added pht() around a couple of them.
Test Plan: I didn't test this :P
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17578
Summary:
Fixes T12460. Also ":)", ":(", ":/", and oldschool ":-)" variants.
Not included are variants with actual letters (`:D`, `:O`, `:P`) and obscure variants (`:^)`, `:*)`).
Test Plan: Typed `:3` (no emoji summoned). Typed `:dog3` (emoji summoned). Typed `@3` (user autocomplete summoned).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12460
Differential Revision: https://secure.phabricator.com/D17577
Summary:
Ref T12450. The way that config repair and setup issues interact is kind of complicated, and if `cluster.search` is invalid we may end up using `cluster.search` before we repair it.
I poked at things for a bit but wasn't confident I could get it to consistently repair before we use it without doing a big messy change.
The only thing that really matters is whether "type" is valid or not, so just put a slightly softer/more-tailored check in for that.
Test Plan:
- With `"type": "elastic"`, loaded setup issues.
- Before patch: hard fatal.
- After patch: softer fatal with more useful messaging.
{F4321048}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17576
Summary:
Ref T12450. Normally, we validate config when:
- You restart the webserver.
- You edit it with `bin/config set ...`.
- You edit it with the web UI.
However, you can also change config by editing `local.json`, `some_env.conf.php`, a `SiteConfig` class, etc. In these cases, you may miss config warnings.
Explicitly re-run search config checks from `bin/search`, similar to the additional database checks we run from `bin/storage`, to try to produce a better error message if the user has made a configuration error.
Test Plan:
```
$ ./bin/search init
Usage Exception: Setting "cluster.search" is misconfigured: Invalid search engine type: elastic. Valid types are: elasticsearch, mysql.
```
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17574
Summary:
Ref T12450. Minor cleanup:
- setRoles() has no callers.
- getRoles() has no callers (these two methods are leftovers from an earlier iteration of the change).
- The `hasRole()` logic doesn't work since nothing calls `setRole()`.
- `hasRole()` has only `isreadable/iswritable` as callers.
- The `isReadable()/isWritable()` logic doesn't work since `hasRole()` doesn't work.
Instead, just check if there are any readable/writable hosts. `Host` already inherits its config from `Service` so this gets the same answer without any fuss.
Also add some read/write constants to make grepping this stuff a little easier.
Test Plan:
- Grepped for all removed symbols, saw only newer-generation calls in `Host`.
- See next diff for use of `isWritable()`.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17571
Summary:
Ref T12450. This is now pointless and just asserts that `cluster.search` has a default value.
We might restore a fancier version of this eventually, but get rid of this for now.
Test Plan: Scruitinized the test case.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17573
Summary:
Ref T12450. This mostly just smooths out the text a little to improve consistency. Also:
- Use `isWritable()`.
- Make the "skipping because not writable" message more clear and tailored.
- Try not to use the word "index" too much to avoid confusion with `bin/search index` -- instead, talk about "initialize a service".
Test Plan: Ran `bin/search init` with a couple of different (writable / not writable) configs, saw slightly clearer messaging.
Reviewers: chad, 20after4
Reviewed By: 20after4
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17572
Summary:
[ ] Write an "Upgrading: ..." guidance task with narrow instructions for installs that are upgrading.
[ ] Do we need to add an indexing activity (T11932) for installs with ElasticSearch?
[ ] We should more clearly detail exactly which versions of ElasticSearch are supported (for example, is ElasticSearch <2 no longer supported)? From T9893 it seems like we may //only// have supported ElasticSearch <2 before, so are the two regions of support totally nonoverlapping and all ElasticSearch users will need to upgrade?
[ ] Documentation should provide stronger guidance toward MySQL and away from Elastic for the vast majority of installs, because we've historically seen users choosing Elastic when they aren't actually trying to solve any specific problem.
[ ] When users search for fulltext results in Maniphest and hit too many documents, the current behavior is approximately silent failure (see T12443). D17384 has also lowered the ceiling for ElasticSearch, although previous changes lowered it for MySQL search. We should not fail silently, and ideally should build toward T12003.
[ ] D17384 added a new "keywords" field, but MySQL does not search it (I think?). The behavior should be as consistent across MySQL and Elastic as we can make it. Likely cleaner is giving "Project" objects a body, with "slugs" and "description" separated by newlines?
[ ] `PhabricatorSearchEngineTestCase` is now pointless and only detects local misconfigurations.
[ ] It would be nice to build a practical test suite instead, where we put specific documents into the index and then search for them. The upstream test could run against MySQL, and some `bin/search test` could run against a configured engine like ElasticSearch. This would make it easier to make sure that behavior was as uniform as possible across engine implementations.
[ ] Does every assigned task now match "user" in ElasticSearch?
[x] `PhabricatorElasticFulltextStorageEngine` has a `json_encode()` which should be `phutil_json_encode()`.
[ ] `PhabricatorSearchService` throws an untranslated exception.
[ ] When a search cluster is down, we probably don't degrade with much grace (unhandled exception)?
[ ] I haven't run bin/search init, but bin/search index doesn't warn me that I may want to. This might be worth adding. The UI does warn me.
[ ] bin/search init warns me that the index is "incorrect". It might be more clear to distinguish between "missing" and "incorrect", since it's more comforting to users to see "everything is as we expect, doing normal first-time setup now" than "something is wrong, fixing it".
[ ] CLI message "Initializing search service "ElasticSearch"" does not end with a period, which is inconsistent with other UI messages.
[ ] It might be nice to let bin/search commands like init and index select a specific service (or even service + host) to act on, as bin/storage --ref ... now does. You can generally get the result you want by fiddling with config.
[ ] When a service isn't writable, bin/search init reports "Search cluster has no hosts for role "write".". This is accurate but does not provide guidance: it might be more useful to the user to explain "This service is not writable, so we're skipping index check for it.".
[x] Even with write off for MySQL, bin/search index --type task --trace still updates MySQL, I think? I may be misreading the trace output. But this behavior doesn't make sense if it is the actual behavior, and it seems like reindexAbstractDocument() uses "all services", not "writable services", and the MySQL engine doesn't make sure it's writable before indexing.
[x] Searching or user fails to find task Grant users tokens when a mention is created, suggesting that stemming is not working.
[x] Searching for users finds that task, but fails to find a task containing "per user per month" in a comment, also suggesting that stemming is not working.
[x] Searching for maniphest fails to find task maniphest.query elephant, suggesting that tokenization in ElasticSearch is not as good as the MySQL tokenization for these words (see D17330).
[x] The "index incorrect" warning UI uses inconsistent title case.
[x] The "index incorrect" warning UI could format the command to be run more cleanly (with addCommand(), I think).
refs T12450
Test Plan:
* Stared blankly at the code.
* Disabled 'write' role on mysql fulltext service.
* Edited a task, ran search indexer, verified that the mysql index wasn't being updated.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T12450
Differential Revision: https://secure.phabricator.com/D17564
Summary:
Ref T12272. If you own a package which owns "/", this allows you to force-accept package reviews for packages which own sub-paths, like "/src/adventure/".
The default UI looks something like this:
```
[X] Accept as epriestley
[X] Accept as Root Package
[ ] Force accept as Adventure Package
```
By default, force-accepts are not selected.
(I may do some UI cleanup and/or annotate "because you own X" in the future and/or mark these accepts specially in some way, particularly if this proves confusing along whatever dimension.)
Test Plan: {F4314747}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12272
Differential Revision: https://secure.phabricator.com/D17569
Summary: Ref T10967. This change is similar to D17566, but for rejects.
Test Plan:
- Create a revision as A, with reviewer B.
- Reject as B.
- Request review as A.
- Before patch: stuck in "rejected".
- After patch: transitions back to "needs review".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17568
Summary: Ref T10967. This moves all remaining "request review" pathways (just `differential.createcomment`) to the new code, and removes the old action.
Test Plan: Requested review on a revision, `grep`'d for the action constant.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17567
Summary:
Ref T10967. This is explained in more detail in T10967#217125
When an author does "Request Review" on an accepted revision, void (in the sense of "cancel out", like a bank check) any "accepted" reviewers on the current diff.
Test Plan:
- Create a revision with author A and reviewer B.
- Accept as B.
- "Request Review" as A.
- (With sticky accepts enabled.)
- Before patch: revision swithced back to "accepted".
- After patch: the earlier review is "voided" by te "Request Review", and the revision switches to "Review Requested".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17566
Summary: Before the speling pollice lock us in prisun.
Test Plan: Used a dicationairey.
Reviewers: chad, jmeador
Reviewed By: jmeador
Differential Revision: https://secure.phabricator.com/D17570
Summary:
This implements a simplistic `PhabricatorRepositoryFulltextEngine`
Currently only the repository name, description, timestamps and
status are indexed.
Note: I had to change the `search index` workflow to disambiguate
PhabricatorRepository from PhabricatorRepositoryCommit
Test Plan:
* ran `./bin/search index --type PhabricatorRepository --force`
* searched for some repositories. Saw reasonable results matching on either title or description.
* Edited a repository in the web ui
* Added unique key words to the repo description.
* I was then able to find that repo by searching for the new keywords.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Tags: #search, #diffusion
Differential Revision: https://secure.phabricator.com/D17300
Summary: General CSS and usability touchup of the Remarkup bar states for fullscreen and preview. Larger fonts, more spacing, some hint of the underlying page. Disable buttons that can't be used in preview mode.
Test Plan:
Formal test coming with mobile, browsers. This is a kick the tires upload.
{F4283448}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17563
Summary:
The goal is to make fulltext search back-ends more extensible, configurable and robust.
When this is finished it will be possible to have multiple search storage back-ends and
potentially multiple instances of each.
Individual instances can be configured with roles such as 'read', 'write' which control
which hosts will receive writes to the index and which hosts will respond to queries.
These two roles make it possible to have any combination of:
* read-only
* write-only
* read-write
* disabled
This 'roles' mechanism is extensible to add new roles should that be needed in the future.
In addition to supporting multiple elasticsearch and mysql search instances, this refactors
the connection health monitoring infrastructure from PhabricatorDatabaseHealthRecord and
utilizes the same system for monitoring the health of elasticsearch nodes. This will
allow Wikimedia's phabricator to be redundant across data centers (mysql already is,
elasticsearch should be as well).
The real-world use-case I have in mind here is writing to two indexes (two elasticsearch clusters
in different data centers) but reading from only one. Then toggling the 'read' property when
we want to migrate to the other data center (and when we migrate from elasticsearch 2.x to 5.x)
Hopefully this is useful in the upstream as well.
Remaining TODO:
* test cases
* documentation
Test Plan:
(WARNING) This will most likely require the elasticsearch index to be deleted and re-created due to schema changes.
Tested with elasticsearch versions 2.4 and 5.2 using the following config:
```lang=json
"cluster.search": [
{
"type": "elasticsearch",
"hosts": [
{
"host": "localhost",
"roles": { "read": true, "write": true }
}
],
"port": 9200,
"protocol": "http",
"path": "/phabricator",
"version": 5
},
{
"type": "mysql",
"roles": { "write": true }
}
]
Also deployed the same changes to Wikimedia's production Phabricator instance without any issues whatsoever.
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Tags: #elasticsearch, #clusters, #wikimedia
Differential Revision: https://secure.phabricator.com/D17384
Under some workloads, the taskmaster may hibernate and launch more rapidly
than it should. Require 15 seconds of inactivity before hibernating. Also
hibernate for longer.
Auditors: chad
The `min()` vs `max()` fix in D17560 meant that the Trigger daemon only
hibernates for 5 seconds, so we do a full GC sweep every 5 seconds. This ends
up eating a fair amount of CPU for no real benefit.
The GC cursors should move to persistent storage, but just bump this default
up in the meantime.
Auditors: chad
If we try to render an edge transaction which uses unknown edge constants,
it turns out we fatal. Degrade instead. This happened when viewing very old
badges.
Auditors: chad
Summary: Ships Badges. I can write up some basic docs too if needed.
Test Plan: /applications/
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17360
Summary: Ref T12270. These no longer have any callsites.
Test Plan: Used `grep` to search for each edge class constant, found no hits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17562
Summary: Ref T12270. Adds a pager, plus a few little cleanups from copy/paste and accumulated cruft.
Test Plan:
- Paginated a user with 180 badges.
- Viewed a user with 0 badges.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17561
Summary: Ref T12298. Like PullLocal daemons, this allows the last daemon in the pool to hibernate if there's no work to be done, and awakens the pool when work arrives.
Test Plan:
- Ran `bin/phd debug task --trace`.
- Saw the pool hibernate and look for tasks.
- Commented on an object.
- Saw the pool wake up and process the queue.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17559
Summary:
Ref T11050. The old rule was "you can only resign if you're a reviewer".
With the new behavior of "resign", the rule should be "you can resign if you're a reviewer, or you have authority over any reviewer". Make it so.
Also fixes T12446. I don't know how to reproduce that but I'm pretty sure this'll fix it?
Test Plan:
- Could not resign from a revision with no authority/reviewer.
- Resigned from a revision with myself as a reviewer.
- Resigned from a revision with a package I owned as a reviewer.
- Could not resign from a revision I had already resigned from.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12446, T11050
Differential Revision: https://secure.phabricator.com/D17558
Summary:
Ref T12298. Two minor daemon improvements:
- Make the "waiting" message reflect hibernation.
- Don't trigger a reload right after launching.
Test Plan:
- Read "waiting" message.
- Ran "bin/phd start", didn't see an immediate SIGHUP in the log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17550
Summary: Fixes T9363. This drops empty buckets from dashboard panel context. Still see full results in Audit.
Test Plan: Create an "Active Audits" panel, add to Dashboard. See no commits found. Check Audit, see all buckets.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9363
Differential Revision: https://secure.phabricator.com/D17545
Summary: Ref T9363, If we're in a dashboard panel, only show buckets with data, or a fallback if nothing exists.
Test Plan: Test 'active revisions' panel in a dashboard and in Differential.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9363
Differential Revision: https://secure.phabricator.com/D17544
Summary: Ref T12298. This allows the PullLocal daemon to hibernate like the Trigger daemon, but automatically wakes it back up when it needs to do something.
Test Plan:
- Ran `bin/phd debug pulllocal --trace`.
- Saw the daemon hibernate after doing a checkup on repositories.
- Saw periodic queries to look for new update messages.
- After clicking "Update Now" in the web UI to schedule an update, saw the daemon wake up immediately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17540
Summary:
Ref T12444. A few issues:
- `x % (y - z)` doesn't generate values in the full range: the largest value is never generated. Instead, use `x % (1 + y - z)`.
- `digestToRange(1, count)` never generates 0. After fixing the first bug, it could generate `count`. The range of the arrays is `0..(count-1)`, inclusive. Generate the correct range instead.
- `unpack('L', ...)` can unpack a negative number on a 32-bit system. Use `& 0x7FFFFFFF` to mask off the sign bit so the result is always a positive integer.
- FileFinder might return arbitrary keys, but we rely on sequential keys (0, 1, 2, ...)
Test Plan:
- Used `bin/people profileimage ... --force` to regenerate images.
- Added some debugging to verify that the math seemed to be working.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12444
Differential Revision: https://secure.phabricator.com/D17543
Summary:
Fixes T12369. When you create objects they may technically be locked: either because the default state is legitimately locked, or because the default policies prevent you from viewing so we sort of technically end in a locked state.
Regardless, don't prompt during creation, since this prompt isn't useful even if the lock detection is completely legitimate.
Test Plan:
- In {nav Applications > Maniphest > Configure}, set "Default View Policy" to "No One".
- Tried to create a task.
- Before patch: prompted to override lock.
- After patch: no override prompt.
Reviewers: chad
Reviewed By: chad
Subscribers: d.maznekov
Maniphest Tasks: T12369
Differential Revision: https://secure.phabricator.com/D17541
Summary:
Ref T12271. Don't do anything with this yet, but store who accepted/rejected/whatever on behalf of reviewers.
In the future, we could use this to render stuff like "Blessed Committers (accepted by epriestley)" or whatever. I don't know that this is necessarily super useful, but it's easy to track, seems likely to be useful, and would be a gigantic pain to backfill later if we decide we want it.
Test Plan: Accepted/rejected a revision, saw reviewers update appropriately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17537
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
Summary: Hit this while `arc diff`'ing something which is triggering 2+ rules which add reviewers, I think.
Test Plan: Dug this out of a production stack trace; will push and `arc diff` again.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17534
Summary: Fixes T12439. This pathway was just missing a `setContinueOnMissingFields(...)` to skip enforcement of required fields.
Test Plan:
- Added a required custom field.
- Mentioned any task without a field value in a comment.
- Edited that comment.
- Saved changes.
- Before fix: fatal in log.
- After fix: clean edit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12439
Differential Revision: https://secure.phabricator.com/D17536
Summary: Fixes T11865. Part of a 'clean up remarkup' pass, removing Aleo helps simplify coding, is lighter on the wire, and gives a more consistent, clean look.
Test Plan: run celerity, grep for 'aleo' and 'Aleo', test Phriction, tasks
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11865
Differential Revision: https://secure.phabricator.com/D17535
Summary:
Ref T10967. I'm not 100% sure we need this, but the old edge table had it and I recall an issue long ago where not having this key left us with a bad query plan.
Our data doesn't really provide a way to test this key (we have many revisions and few reviewers, so the query planner always uses revision keys), and building a convincing test case would take a while (lipsum needs some improvements to add reviewers). But in the worst case this key is mostly useless and wastes a few MB of disk space, which isn't a big deal.
So I can't conclusively prove that this key does anything to the dashboard query, but the migration removed it and I'm more comfortable keeping it so I'm not worried about breaking stuff.
At the very least, MySQL does select this key in the query plan when I do a "Reviewers:" query explicitly so it isn't //useless//.
Test Plan: Ran `bin/storage upgrade`, ran dashboard query, the query plan didn't get any worse.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17532
Summary:
Fixes T11050. Today, when a user resigns, we just delete the record of them ever being a reviewer.
However, this means you have no way to say "I don't care about this and don't want to see it on my dashboard" if you are a member of any project or package reviewers.
Instead, store "resigned" as a distinct state from "not a reviewer", and treat it a little differently in the UI:
- On the bucketing screen, discard revisions any responsible user has resigned from.
- On the main `/Dxxx` page, show these users as resigned explicitly (we could just hide them, too, but I think this is good to start with).
- In the query, don't treat a "resigned" state as a real "reviewer" (this change happened earlier, in D17517).
- When resigning, write a "resigned" state instead of deleting the row.
- When editing a list of reviewers, I'm still treating this reviewer as a reviewer and not special casing it. I think that's sufficiently clear but we could tailor this behavior later.
Test Plan:
- Resigned from a revision.
- Saw "Resigned" in reviewers list.
- Saw revision disappear from my dashboard.
- Edited revision, saw user still appear as an editable reviewer. Saved revision, saw no weird side effects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11050
Differential Revision: https://secure.phabricator.com/D17531
Summary: Fixes T12434. I accidentally copy/pasted this too much in D17442.
Test Plan: Viewed a form edit page, no longer saw two copies of this action.
Reviewers: chad, cspeckmim
Reviewed By: chad, cspeckmim
Maniphest Tasks: T12434
Differential Revision: https://secure.phabricator.com/D17530
Summary: Ref T10390. Catch if the user doesn't have any dashboards they can edit and give them a helpful message instead.
Test Plan: Clean install, no dashboards, Click "Add to Dashboard" on ApplicationSearch results, see no dashboards message
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17528
Summary: Ref T10390. Dashboard usability is high enough that I think we should pin it by default for users to create custom home pages.
Test Plan: Review order of applications in sandbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17527
Summary: Ref T10390. Fixes the missing "fa-dashboard" icon and adds a few more for an even 25.
Test Plan: Create new dashboard, see dashboard icon, select new dashboard icon.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17526
Summary: Ref T10390. I find myself wanting to find dashboards I can edit, even if I am not the author. I think this is useful for larger installs with multiple admins. Also make disabled Dashboards more grey in UI results.
Test Plan: Log in a test user, create a dashboard with I cannot edit. Log into my account, search for editable dashboards and only see mine. Set dashboard to all users, search under test account and see editable dashboards.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17524
Summary: Ref T5307. Just makes the dialog a little easier to use. Picks a name if we already have one.
Test Plan: Test a builtin, custom saved, and a new advanced search (no name).
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5307
Differential Revision: https://secure.phabricator.com/D17523
Summary:
Ref T10967. Improves some method names:
- `Revision->getReviewerStatus()` -> `Revision->getReviewers()`
- `Revision->attachReviewerStatus()` -> `Revision->attachReviewers()`
- `Reviewer->getStatus()` -> `Reviewer->getReviewerStatus()` (this is mostly to make this more greppable)
Test Plan:
- bunch o' `grep`
- Browsed around.
- If I missed anything, it should fatal in an obvious way. We have a lot of other `getStatus()` calls and it's hard to be sure I got them all.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17522
Summary: Ref T10967. The old name was because we had a `getReviewers()` tied to `needRelationships()`, rename this method to use a simpler and more clear name.
Test Plan: `grep`, browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17519
Summary:
Ref T10967. There have been two different ways to load reviewers for a while: `needReviewerStatus()` and `needRelationships()`.
The `needRelationships()` stuff was a false start along time ago that didn't really go anywhere. I believe the idea was that we might want to load several different types of edges (subscribers, reviewers, etc) on lots of different types of objects. However, all that stuff pretty much ended up modularizing so that main `Query` classes did not need to know about it, so `needRelationships()` never got generalized or went anywhere.
A handful of things still use it, but get rid of them: they should either `needReviewerStatus()` to get reviewer info, or the ~3 callsites that care about subscribers can just load them directly.
Test Plan:
- Grepped for removed methods (`needRelationships()`, `getReviewers()`, `getCCPHIDs()`, etc).
- Browsed Diffusion, Differential.
- Called `differential.query`.
It's possible I missed some stuff, but it should mostly show up as super obvious fatals ("call needReviewerStatus() before getReviewerStatus()!").
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17518
Summary:
Ref T10967.
When we query for revisions with particular reviewers, use the new table to drive the query.
When we load revisions for use in the application, also use the new table to drive the query.
This doesn't convert everything: there's some old `loadRelationships()` stuff still using the old table. But this moves the major stuff over.
(This also changes the icon for "commented" from a question mark to a speech bubble.)
Test Plan:
- Viewed revision lists and detail views on old and new code, saw identical outcomes.
- Updated revisions, accepted/rejected/commented on revisions.
- Hit the "Accepted Older" and "Commented Older" states by taking an action and then updating.
- Grepped for removed methods (like `getEdgeData()` and `getDiffID()`).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17517
Summary:
Ref T10967. We have a "commented" state to help reviewers get a better sense of who is part of a discussion, and a "last action" state to help distinguish between "accept" and "accepted an older version", for the purposes of sticky accepts and as a UI hint.
Currently, these are first-class states, partly beacuse we were somewhat limited in what we could do with edges. However, a more flexible way to represent them is as flags separate from the primary state flag.
In the new storage, write them as separate state information: `lastActionDiffPHID` stores the Diff PHID of the last review action (accept, reject, etc). `lastCommentDiffPHID` stores the Diff PHID of the last comment (top-level or inline).
Test Plan: Applied storage changes, commented and acted on a revision. Saw appropriate state reflected in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17514
Summary:
Ref T10967. `differential.createcomment` is a frozen API method which has been obsoleted by `differential.revision.edit`.
It is the only remaining way to apply an "accept", "reject", or "resign" action using the old "ACTION" code.
Instead of using the old code, sneakly apply a new type of transaction in these cases instead.
Then, remove all the remaining old code for this stuff on the write pathways.
Test Plan:
- Used "differential.createcomment" to accept, reject, and resign from a revision.
- Grepped for all removed ACTION_X constants, found them only in rendering code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17513
Summary: Ref T10967. See that task for some discussion. This lets us do double writes on this pathway.
Test Plan: Set an Owners package to auto-review. Created revisions which triggered it: one with no reviewers (autoreview added); one with the package as a blocking reviewer explicitly (no automatic stuff happened, as expected).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17512
Summary:
Ref T10967. This converts the reviewer update action in Herald from an older edge write to a newer ModularTransactions write.
The major value from this is that we get a double-write to the new reviewers table.
Test Plan:
- Wrote a Herald rule to add a reviewer and a blocking reviewer.
- Saw them added properly to a revision with: no reviewers; both as blocking; A as blocking, B as nonblocking; A as nonblocking, B as blocking.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17511
Summary: Ref T5307. Actually check the built in query with query, not engine.
Test Plan: Try a builtin query, and a custom query when making a dashboard panel.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5307
Differential Revision: https://secure.phabricator.com/D17521
Summary: Ref T5307. Adds a better query check query, sets required for the name, adds the correct URI for cancelling.
Test Plan: Test a form without a name, fake a query string, test cancel button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T5307
Differential Revision: https://secure.phabricator.com/D17520
Summary: Ref T5307. This adds an additional action to Use Results for creating a panel from the query.
Test Plan:
Navigate to Maniphest, select dropdown for Use Results. Try any of the following:
- Try to set a panel without a name (fail)
- Muck up query or engine (fail)
- Set a fake Dashboard ID (fail)
Give panel a name and select a dashboard I have edit permissions to, get taken to dashboard.
Reviewers: epriestley
Subscribers: Korvin
Maniphest Tasks: T5307
Differential Revision: https://secure.phabricator.com/D17516
Summary: Fixes T12418. This is a fairly advanced feature and I think users can reasonably consult the documentation for their own editors to figure out how to do this.
Test Plan: Saw no more text.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12418
Differential Revision: https://secure.phabricator.com/D17510
Summary: Fixes T12416. See that task for discussion. Slightly older versions of `git` do not appear to support use of `--` to separate flags and arguments.
Test Plan:
- Ran `bin/repository update PHABX`.
- In T12416, had a user with Git 2.1.4 confirm that `git ls-remote X` worked while `git ls-remote -- X` failed.
- Read `git help ls-remote` to look for any kind of suspicious `--destroy-the-world` flags, didn't see any that made me uneasy.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T12416
Differential Revision: https://secure.phabricator.com/D17508
Summary:
Ref T5378. This repackages an existing check to see if a URI is a URI for the current install into a more reasonable form.
In an upcoming change, I'll use this new check to test whether `http://example.whatever.com/T123` is a link to a task on the current install or not.
Test Plan: This stuff has good test coverage already; added some more.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5378
Differential Revision: https://secure.phabricator.com/D17502
Summary:
Ref T12392. The logic currently goes like this:
- Try a fetch.
- If that fails, try repairing the origin URI.
- Then try again.
This is pretty complicated, and we can use this simpler logic instead:
- Set the origin URI to the right value.
- Try a fetch.
Setting the origin URI is very fast. This can normally only get us in any trouble in very obscure situations which haven't occurred for many years:
- Pretty much all of this is already covered by `verifyGitOrigin()`, which we run earlier.
- Origins could be configured to have multiple URIs for some reason, but shouldn't be.
- Years ago, you could configure Phabricator to point at a local repository it didn't own and that could conceivably have a different "origin" that you might not want us to delete. If you did this, the daemons have been spewing errors for 3-4 years without you fixing it. The cost of fixing the remote URI is very small even if anyone is affected by this (just set it back to the old value) and there's zero reason to do this and the scenario is ridiculous.
Test Plan: Ran `bin/repository update PHABX --trace --verbose`, saw fetches go through cleanly after URI adjustment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12392
Differential Revision: https://secure.phabricator.com/D17498
Summary:
Ref T12296. Ref T12392. Currently, when we're observing a remote repository, we periodically run `git fetch ...`.
Instead, periodically run `git ls-remote` (to list refs in the remote) and `git for-each-ref` (to list local refs) and only continue if the two lists are different.
The motivations for this are:
- In T12296, it appears that doing this is //faster// than doing a no-op `git fetch`. This effect seems to reproduce locally in a clean environment (900ms for `ls-remote` + 100ms for `for-each-ref` vs about 1.4s for `fetch`). I don't have any explanation for why this is, but there it is. This isn't a huge change, although the time we're saving does appear to mostly be local CPU time, which is good for us.
- Because we control all writes, we could cache `git for-each-ref` in the future and do fewer disk operations. This doesn't necessarily seem too valuable, though.
- This allows us to tell if a fetch will do anything or not, and make better decisions around clustering (in particular, simplify how observed repository versioning works). With `git fetch`, we can't easily distinguish between "fetch, but nothing changed" and "legitimate fetch".
If a repository updates very regularly we end up doing slightly more work this way (that is, if `ls-remote` always comes back with changes, we do a little extra work), but this is normally very rare.
This might not get non-bare repositories quite right in some cases (i.e., incorrectly detect them as changed when they are unchanged) but we haven't created non-bare repositories for many years.
Test Plan: Ran `bin/repository update --trace --verbose PHABX`, saw sensible construction of local and remote maps and accurate detection of whether a fetch would do anything or not.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12392, T12296
Differential Revision: https://secure.phabricator.com/D17497
Summary: Ref T12270. Builds out a BadgeCache for PhabricatorUser, primarily for Timeline, potentially feed? This should still work if we later let people pick which two, just switch query in BadgeCache.
Test Plan: Give out badges, test timeline for displaying badges from handles and without queries. Revoke a badge, see cache change.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17503
Summary:
Fixes T12406. When importing commits, we automatically add auditors if the message lists "Auditors: username".
If the list of auditors includes the commit author, this edit fails because you can't audit your own commits (previously, you sometimes could and/or we didn't validate).
Instead, just ignore "Auditors: author".
Test Plan:
- Made a commit with "Auditors: epriestley".
- Pushed it.
- Saw the HeraldWorker get stuck with the error in T12406.
- Applied the change; worker now succeeded.
Reviewers: chad
Reviewed By: chad
Subscribers: alexmv
Maniphest Tasks: T12406
Differential Revision: https://secure.phabricator.com/D17507
Summary:
Fixes T12409. Config entries may be marked as "deleted", and `bin/config set --database` doesn't un-delete them, so the edit doesn't do anything.
The "most correct" fix here is to swap to transactions so we run the same code, but just fix this narrowly for now since it's one line of code.
Test Plan:
- Set `maniphest.default-priority` to `123`.
- Deleted `maniphest.default-priority` from the web UI by deleting all the text in the box.
- Before patch: `bin/config set --database maniphest.default-priority 789` had no effect.
- After patch: `bin/config set --database maniphest.default-priority 789` worked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12409
Differential Revision: https://secure.phabricator.com/D17506
Summary: Extends PHUIListItemView to take an icon, link as an "Action Item" that displays on the right side of the menu link. Does not display on Favorites. This allows for adding edit, external, or other links (documentation?) to any menu item. Right now the secondary link is only visible when the item is selected. This feels right, but if we offer it in other ways, users may always want it visible. We could look at making it onhover.
Test Plan:
Add a bunch of random global and personal dashboards to my menu. Add a menu to Favorites, see no link. Test mobile, link works.
{F4136699}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17505
Summary:
Via HackerOne. When you view a raw file in Differential, we currently generate a permanent file with default permissions. This may be incorrect: default permissions may be broader than the diff's permissions.
The other three methods of downloading/viewing raw files ("Download" in Diffusion and Differential, "View Raw" in Diffusion and Differential) already apply policies correctly and generate temporary files. However, this workflow was missed when other workflows were updated.
Beyond updating the workflow, delete any files we've generated in the past. This wipes the slate clean on any security issues and frees up a little disk space.
Test Plan:
- Ran migration script, saw existing files get purged.
- Did "View Raw File", got a new file.
- Verified that the file was temporary and properly attached to the diff, with "NO ONE" permissions.
- Double-checked that Diffusion already runs policy logic correctly and applies appropriate policies.
- Double-checked that "Download Raw Diff" in Differential already runs policy logic correctly.
- Double-chekced that "Download Raw Diff" in Diffusion already runs policy logic correctly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17504
Summary:
Fixes T12400. Adds a "Has MFA" filter to People so you can figure out who you need to harass before turning on "require MFA".
When you run this as a non-admin, you don't currently actually hit the exception: the query just doesn't work. I think this is probably okay, but if we add more of these it might be better to make the "this didn't work" more explicit since it could be confusing in some weird edge cases (like, an administrator sending a non-administrator a link which they expect will show the non-administrator some interesting query results, but they actually just get no constraint). The exception is more of a fail-safe in case we make application changes in the future and don't remember this weird special case.
Test Plan:
- As an administrator and non-administrator, used People and Conduit to query MFA, no-MFA, and don't-care-about-MFA. These queries worked for an admin and didn't work for a non-admin.
- Viewed the list as an administrator, saw MFA users annotated.
- Viewed config help, clicked link as an admin, ended up in the right place.
{F4093033}
{F4093034}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12400
Differential Revision: https://secure.phabricator.com/D17500
Summary: Fixes T12398. This adds `withBadgeStatuses` as a query parameter when searching for Awards to show. In most (all?) cases we currently only show active badges.
Test Plan: Assign myself a badge, archive it and verify it does not appear on profile, comment form, or timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12398
Differential Revision: https://secure.phabricator.com/D17499
Summary: Ref T12264. This allows users to install a dashboard they are viewing to their personal home menu or as a global home menu item. Has some basic ability to be extended later for maybe projects.
Test Plan:
Build a dashboard, click "Install Dashboard".
- As user only get personal option
- As HomeApp edit person, see both options
- Try installation as either, with and without label set
- Fake "global" form as user, get error
- Don't set anything, get error
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12264
Differential Revision: https://secure.phabricator.com/D17492
Summary:
Ref T10967. This is an incremental step toward removing "reviewers" back to a dedicated storage table so we can handle changes like T11050.
This adds the storage table, and starts doing double writes to it (so new or updated reviewers write to both the old edge table and the new "reviewers" table).
Then we can do a migration, swap readers over one at a time, and eventually remove the old write and old storage and then implement new features.
This change has no user-facing impact, it just causes us to write new data to two places instead of one.
This is not completely exhaustive: the Herald "Add Reviewers" action is still doing a manual EDGE transaction. I'll clean that up next and do another pass to look for anything else I missed.
This is also a bit copy/pastey for now but the logic around "RESIGN" is a little different in the two cases until T11050. I'll unify it in future changes.
Test Plan:
- Did a no-op edit.
- Did a no-op comment.
- Added reviewers.
- Removed reviewers.
- Accepted and rejected revisions.
After all of these edits, did a `SELECT * FROM differential_reviewer` manually and saw consistent-looking rows in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17495
Summary: Fix tag alignment on project cards when there are multiple tags. Also fixes T12381.
Test Plan: Review a project and people hovercard in sandbox, ensure multiple tags look as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12381
Differential Revision: https://secure.phabricator.com/D17488
Summary: These were once ordered, but I think we switched to being defined in the Engine and never implemented the sorts there. This adds sort ordering to Tasks, Projects, and Repositories.
Test Plan: Review Favorites Menu in local install, see order is now set per the engine. Click Edit Favorites, and re-order. See order sticks.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17490
Summary:
Ref T12270. This just drops the constraint for now, rather than dealing with all the typecasting stuff and putting us in a position which will almost certainly require backward compatibility breaks in the future.
Also renames "badges.*" to "badge.*" for consistency (all other methods are singular: token.*, project.*, differential.revision.*, etc).
Test Plan:
Saw "qualities" now "Not Supported", while other constraints continue to work:
{F3887194}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17487
Summary: Allow people to award and remove badges via conduit, but not from the standard badges form.
Test Plan:
Build a generator and generate awards. Didn't test the revoke yet.
{F3857766}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17482
Summary:
Fixes T12378. Two minor issues here:
- CAN_INTERACT on tasks uses "USER", but should just use the view policy, which may be more permissive ("PUBLIC").
- CAN_INTERACT is currently prevented from being "PUBLIC" by additional safeguards. Define an explicit capability object for the permission which returns `true` from `shouldAllowPublicPolicySetting()`.
Test Plan:
- Viewed an unlocked task as a logged-out user, saw "login to comment" instead of "locked".
- Viewed a locked task as a logged-out user, saw "locked".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12378
Differential Revision: https://secure.phabricator.com/D17485
Summary: The Safari hack in place casued a truncation issue in Firefox, so that hack is now gone. Instead the bug appears to be the creative inclusion of "space". In fiddling with this adding one space inside the span and one space outside the span seems to resolve all cases.
Test Plan: Chrome, Safari, Firefox. Test "hector" and copy paste of a Task ID.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17483
Summary: This is overly broad and I missed it in local testing with just a single account. Let's pull just the author in.
Test Plan: Review a commit page that wasn't my own, see other authors commits.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17481
Summary: Fixes T12367. CSS here already truncates (or should have been) and is generally more effective. Remove the unneeded server side truncation. Any other UI place these render?
Test Plan: Set Policy to a group name of "Stanford University: Alumni Association and Friends" and see better truncation.
Reviewers: epriestley, eliaspro
Reviewed By: epriestley, eliaspro
Subscribers: eliaspro, Korvin
Maniphest Tasks: T12367
Differential Revision: https://secure.phabricator.com/D17479
Summary: This should be blue, not grey.
Test Plan: Add a milestone and subproject to a project
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17477
Summary: Fixes T10698. This shows badges under the comment preview if the application uses TransactionCommentView. I suspect not everything does, but will pick the fix up for free when modernized.
Test Plan: Test commenting on a task with and without a user that has a badge. See badge preview.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10698
Differential Revision: https://secure.phabricator.com/D17480
Summary: This currently queries all tasks, make it limit to only open tasks.
Test Plan: Assign myself an open and a resolved task. See only open on profile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17476
Summary: Fixes T12360. I'll probably make a non-audit commit list for this, maybe, eventually, until then add all the needed audit information.
Test Plan: Review commits in my profile, see data and not a fatal.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12360
Differential Revision: https://secure.phabricator.com/D17475
Summary: This just adds a few more dimensions to the generator.
Test Plan: run `bin/lipsum generate badges`, verify new icons and quality work.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17474
Summary: Probably useful if you use Phrequent.
Test Plan: I did not test this beyond lint/unit.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17473
Summary: Mostly a minor nit-pick, but I hate sending users off the profile and disorient them onto application search. These pages are pretty easy to maintain, I don't expect to need to do more here. I dropped Differential outright. Kept Tasks and Commits. Now you can browse everything about a user on their profile without leaving. Maybe add a link to ApplicationSearch? Not sure it's important.
Test Plan: Review tasks and commits on mine and other user profiles.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17470
Summary: Ref T12270. Adds the date the badge was awarded.
Test Plan: Award a badge, see date on profile badge when card is flipped.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17471
Summary: Fixes T10319. This looks for custom profile image, then falls back to a generated profile image.
Test Plan: Create a new user, log in, and see new profile image. Note this seems to break `bin/lipsum generate user`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17467
Summary: Ref T12337. This just fills out a couple more task relationships.
Test Plan: Viewed the edges in the Conduit console, queried for them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12337
Differential Revision: https://secure.phabricator.com/D17465
Summary: Ref T10319. This adds a basic means of generating default profile images for users. You can generate them for everyone, a group of users, or force updates. This only generated images and stores them in files. It does not assign them to users.
Test Plan:
`bin/people profileimage --all` to generate all images.
`bin/people profileimage --users chad` to generate a user.
`bin/people profileimage --all --force` to force rebuilding all images.
{F3662810}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17464
Summary:
Ref T12337. Ref T5873. This provides a generic "edge.search" method which feels like other "verison 3" `*.search` methods.
The major issues here are:
1. Edges use constants internally, which aren't great for an API.
2. A lot of edges are internal and probably not useful to query.
3. Edges don't have a real "id", so paginating them properly is challenging.
I've solved these things like this:
- Edges must opt-in to being available via Conduit by providing a human-readable key (like "mention" instead of "52"). This solvs (1) and (2).
- I faked a mostly-reasonable behavior for paginating.
Test Plan:
Ran various valid and invalid searches. Paginated a large search. Reviewed UI.
{F3651818}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12337, T5873
Differential Revision: https://secure.phabricator.com/D17462
Summary: Ref T12335. Allows you to lock tasks to keep your precious tokens.
Test Plan:
- Awarded tokens to an unlocked task.
- Locked the task.
- Could no longer award/rescind tokens.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12335
Differential Revision: https://secure.phabricator.com/D17461
Summary: Ref T10319. Adds in database columns for upcoming default generated avatar support.
Test Plan: Ran storage upgrade, log into local site to verify it didn't blow up.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17459
Summary: Ref T10319. This swaps the default in the Picture Chooser to allow picking of the custom unique avatar. We're currently going with 100k unique possibilities. The logic roughly hashes a user name and picks an image pack, color, and border. Based on that, we select the first character of their username, or fall back to Psyduck if not [a-z][0-9].
Test Plan:
Set the following usernames from ProfilePicture as a test: chad, epriestley, sally, 007, _cat_, -doggie-.
{F3453979}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17430
Summary:
Ref T12313. This puts a UI on revoking credentials after a widespread compromise like Cloudbleed or a local one like copy/pasting a token into public chat.
For now, I'm only providing a revoker for conduit tokens since that's the immediate use case.
Test Plan:
- Revoked in user + type, everything + user, everywhere + type, and everything + everywhere modes.
- Verified that conduit tokens were destroyed in all cases.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12313
Differential Revision: https://secure.phabricator.com/D17458
Summary: Ref T9010. This adds more icons and lets the IconChooser handle more icons more easier.
Test Plan: Test Project Icons, Badges Icons
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9006, T9010
Differential Revision: https://secure.phabricator.com/D17456
Summary: Fixes T12347. Ref T12314. Validation gets called no matter what, but is only relevant if the form supports subtypes.
Test Plan: Marked/unmarked a Paste form as editable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12347, T12314
Differential Revision: https://secure.phabricator.com/D17457
Summary: Fixes T10798. Separates these two since they don't need to be combined and it allows for more flexibility / scalability.
Test Plan:
- Add Badge
- Edit Badge
- Add myself as Recipient
- Remove myself
- Go to my profile
- Award Badge from there
- Assign myself a badge, try to re-assign it, see validation error.
Also, validation errors on dialog forms are ugly.
{F3495630}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10798, T12270
Differential Revision: https://secure.phabricator.com/D17447
Summary:
This is a consistency change to make this option consistent with `audio-mime-types`, `image-mime-types` and `icon-mime-types`, all of which are locked.
(They're locked because SVG is definitely dangerous, and other types might be dangerous or might become dangerous in the future, although I'm not aware of any actual dangers from video types today.)
Test Plan: Viewed `files.video-mime-types` in Config, saw it was locked.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17454
Summary:
Ref T12335. See that task for discussion. Here are the behavioral changes:
- Statuses can be flagged with `locked`, which means that tasks in that status are locked to further discussion and interaction.
- A new "CAN_INTERACT" permission facilitates this. For most objects, "CAN_INTERACT" is just the same as "CAN_VIEW".
- For tasks, "CAN_INTERACT" is everyone if the status is a normal status, and no one if the status is a locked status.
- If a user doesn't have "Interact" permission:
- They can not submit the comment form.
- The comment form is replaced with text indicating "This thing is locked.".
- The "Edit" workflow prompts them.
This is a mixture of advisory and hard policy checks but sholuld represent a reasonable starting point.
Test Plan: Created a new "Locked" status, locked a task. Couldn't comment, saw lock warning, saw lock prompt on edit. Unlocked a task.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12335
Differential Revision: https://secure.phabricator.com/D17453
Summary:
Ref T12335. Fixes T11207. Edit-like interactions which are not performed via "Edit <object>" are a bit of a grey area, policy-wise.
For example, you can correctly do these things to an object you can't edit:
- Comment on it.
- Award tokens.
- Subscribe or unsubscribe.
- Subscribe other users by mentioning them.
- Perform review.
- Perform audit.
- (Maybe some other stuff.)
These behaviors are all desirable and correct. But, particularly now that we offer stacked actions, you can do a bunch of other stuff which you shouldn't really be able to, like changing the status and priority of tasks you can't edit, as long as you submit the change via the comment form.
(Before the advent of stacked actions there were fewer things you could do via the comment form, and more of them were very "grey area", especially since "Change Subscribers" was just "Add Subscribers", which you can do via mentions.)
This isn't too much of a problem in practice because we won't //show// you those actions if the edit form you'd end up on doesn't have those fields. So on intalls like ours where we've created simple + advanced flows, users who shouldn't be changing task priorities generally don't see an option to do so, even though they technically could if they mucked with the HTML.
Change this behavior to be more strict: unless an action explicitly says that it doesn't need edit permission (comment, review, audit) don't show it to users who don't have edit permission and don't let them take the action.
Test Plan:
- As a user who could not edit a task, tried to change status via comment form; received policy exception.
- As a user who could not edit a task, viewed a comment form: no actions available (just "comment").
- As a user who could not edit a revision, viewed a revision form: only "review" actions available (accept, resign, etc).
- Viewed a commit form but these are kind of moot because there's no separate edit permission.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12335, T11207
Differential Revision: https://secure.phabricator.com/D17452
Summary: Ref T12270. We don't really need these, timeline does it's own thing, badges is now a profile page, and hovercards have been removed.
Test Plan: Visit timeline, still see badges, visit my profile page, bask in the warmth of fake awards.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17448
Summary: Ref T12270. Adds the name of the badge to the subject, fixes the double description.
Test Plan: Edit lots of badges with and without descriptions, see good emails.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17449
Summary: Doesn't seem popular, will rethink dashboard editing again in the future at some point.
Test Plan: Review a dashboard, edit, install.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17450
Summary:
Ref T12314. Ref T6064. Ref T11580. If an install defines several different task create forms (like "Create Plant" and "Create Animal"), allow any of them to be created directly onto a workboard column.
This is just a general consistency improvement that makes Custom Forms and Workboards work together a bit better. We might do something fancier eventually for T6064 (which wants fewer clicks) and/or T11580 (which wants per-workboard control over forms or defaults).
Test Plan:
- Created several different types of tasks directly onto a workboard.
- Faked just one create form, saw the UI unchanged (except that it respects any renaming).
{F3492928}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314, T11580, T6064
Differential Revision: https://secure.phabricator.com/D17446
Summary:
Ref T12314. When we pick an "Edit" form for a subtyped object, only consider forms with the same subtype.
For example, editing an "Animal" uses the forms with subtype "animal" which are marked as edit forms.
This also makes "Create Subtask" carry the parent task's type.
Test Plan:
- Edited an Animal, got an animal edit form.
- Edited a normal task, got a normal task form.
- Edited a paste, got the normal workflow.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17445
Summary:
Ref T12314. Allow tasks to be queried by subtype using a typeahead.
Open to a better default icon. I'll probably let you configure them later.
Just hide this constraint if there's only one subtype.
Test Plan:
- Searched for subtypes.
- Verified that the control hides if there is only one subtype.
{F3492293}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17444
Summary:
Ref T12314. If you set a form to have the "plant" subtype, then create a task with it, save "plant" as the task subtype.
For Conduit, the default subtype is used by default, but a new "subtype" transaction is exposed. You can apply this transaction at create time to create an object of a certain subtype, or at any later time to change the subtype of an object.
This still doesn't do anything particularly useful or interesting.
Test Plan:
- Created a non-subtyped object (a Paste).
- Created "task" and "plant" tasks via different forms.
- Created "default" and "plant" tasks via Conduit.
- Changed the subtype of a task via Conduit.
- Tried to set a bad subtype.
{F3492061}
{F3492066}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17443
Summary:
Ref T12314. This adds a "Change Form Subtype" workflow to the EditEngine form configuration screen, for forms that edit/create objects which support subtyping (for now, only tasks).
For example, this allows you to switch a form from being a "task" form to a "plant" or "animal" form.
Doing this doesn't yet do anything useful or interesting. I'm also not showing it in the UI yet since I'm not sure what we should make that look like (presumably, we should just echo whatever UI we end up with on tasks).
Test Plan:
- Changed the subtype of a task form.
- Verified that the "Change Subtype" action doesn't appear on other forms (for example, those for Pastes).
{F3491374}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17442
Summary: Ref T12314. Provides a field on tasks for storing subtypes. Does nothing interesting yet.
Test Plan:
- Ran storage upgrade.
- Created some tasks.
- Looked in the database.
- Used Conduit to query some tasks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17441
Summary:
Ref T12314. Builds toward letting you define "animal" and "plant" tasks.
This just adds some configuration. I'll probably add some more quality-of-life options (like "icon") later but these are the only bits I'm sure I'll need.
Test Plan:
- Configured sensible subtypes.
- Tried to configure bad subtypes: bad key, missing "default", duplicate keys. Got sensible error messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17440
Summary:
Ref T12314. This adds storage so EditEngine forms can later be marked as edit fields for particular types of objects (like an "animal edit form" vs a "plant edit form").
We'll take you to the right edit form when you click "Edit" by selecting among forms with the same subtype as the task.
This doesn't do anything very interesting on its own.
Test Plan:
- Ran `bin/storage upgrade`.
- Verified database got the field with proper values.
- Created a new form, checked the database.
- Ran unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12314
Differential Revision: https://secure.phabricator.com/D17439
Summary: Ref T12298. `phd launch` was missed in D17390 and thus broken by D17389.
Test Plan: Launched a daemon with great success.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17429
Summary: Fixes T12338. Resolves an issue where long pastes would be truncated before getting a line count, resulting in an inaccurate line count being returned.
Test Plan: Made a large paste, verified that it displayed the correct number of lines.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T12338
Differential Revision: https://secure.phabricator.com/D17438
Summary: Fixes T12330. Minor UI nit, since we use "disabled" to usually mean "no permission". Makes these links always normal looking.
Test Plan: Review a new project in sandbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12330
Differential Revision: https://secure.phabricator.com/D17436
Summary: This is incorrectly starting an ordered sublist.
Test Plan: (o_O)
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17434
Summary: Ref T12331. These changes are intended to make it easier to debug T12331 since I'm having difficulty reproducing the issue locally.
Test Plan:
- Ran `bin/phd debug task --pool 4` and got an autoscaling pool.
- Ran `bin/worker flood --duration 3` and got some 3-second-long tasks to execute with `bin/worker execute ...`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12331
Differential Revision: https://secure.phabricator.com/D17431
Summary: Ref T12324. Adds back this query for search results in dashboards.
Test Plan: Use panel in Dashboard.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12324
Differential Revision: https://secure.phabricator.com/D17428
Summary: Ref T10319. This builds out a reasonably decent avatar generator. 256 colors x 74 images x 2 borders, 38k options. Not completely sure though how names disburse though, so likely half that number. I can add lowercase lettering to double the footprint if needed though.
Test Plan:
UIExamples. Color generator here: http://tools.medialab.sciences-po.fr/iwanthue/
{F3416622}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17418
Summary:
Fixes T12322. Allows you to search for commits using the `tagged(...)` repository function, so you can find "any commmit in any repository tagged with android" or similar.
I moved the function from Differential (which was the application using it) to Diffusion (which is more accurately the application which provides it).
I fixed a bug where searching for `tagged(xyz)` would have no effect (constraint was ignored) if there were no repositories tagged with "xyz". The fix isn't perfectly clean, but should work properly for the moment.
Test Plan:
- Searched with `tagged(...)` in Diffusion and Differential.
- Searched by repository.
- Searched with `tagged(...)` for a project with no tagged repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12322
Differential Revision: https://secure.phabricator.com/D17426
Summary:
Ref T12319. With large datasets, the computation of which packages own paths in a revision is needlessly slow.
Improve performance through caching:
- Cache which paths belong to each repository.
- Cache the split fragments of each path.
- Cache the path fragment counts.
- Micro-optimize accessing `$this->path`.
Test Plan:
- Used `bin/lipsum` to generate 4,000 packages with 150,000 paths.
- Created a revision affecting 100 paths in `phabricator/` (these paths mostly overlap with `bin/lipsum` path rules, since Lipsum uses Phabricator-like rules to generate paths).
- Before optimizations, this revision spent about 5.5 seconds computing paths.
- After optimizations, it spends about 275ms.
{F3423414}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12319
Differential Revision: https://secure.phabricator.com/D17424
Summary: Ref T12319. Ref T12270. Allow badges to be generated with `bin/lipsum`. These aren't hugely sophisticated but I'm not sure about the fate of T9010 yet or what's happening with the quality levels, and didn't want to make those changes more difficult.
Test Plan:
- Used `bin/lipsum generate badges --force --quickly` to generate badges.
- Made some coffee and came back to 20K badges.
{F3422200}
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T12319, T12270
Differential Revision: https://secure.phabricator.com/D17422
Summary:
Ref T12319.
- Lipsum can trash an install by creating a lot of junk that's hard to get rid of, so we're cautious about letting you run it. Add a `--force` flag if you're sure you know what you're doing. This makes the edit/test cycle a bit easier when actually writing Lipsum generators.
- Lipsum normally sleeps for a second before creating objects, to give users more control over how much stuff they create and limit the amount of damage caused by mistakes. Sometimes, you want to generate a LOT of stuff because you want to reproduce a performance/scale issue (like T12319). Add a `--quickly` flag to generate objects as fast as possible.
- When loading random users (used as authors, assignees, etc), also load user settings so we can `ConduitCall` with them.
- Allow generators to return a PHID instead of an actual object (more convenient for Conduit-based generators).
Test Plan:
- With next change, ran `lipsum generate badges --force --quickly`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12319
Differential Revision: https://secure.phabricator.com/D17421
Summary:
Ref T12319. Currently, `bin/lipsum` uses substring matches against human-readable text to chose which objects to generate.
Instead:
- Use separate selector keys which are guaranteed to be unique.
- When a match is exact, select only that generator.
- When a match is ambiguous, fail and warn the user.
Test Plan: Generated several types of objects, tried to generate ambiguous objects like "e".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12319
Differential Revision: https://secure.phabricator.com/D17420
Summary: Ref T12319. The product name is misspelled in some methods, and a few places in the documentation.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12319
Differential Revision: https://secure.phabricator.com/D17419
Summary: Looks nicer on profiles, cards. Added some additional colors.
Test Plan: change my avatar a few times
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: avivey, Korvin
Differential Revision: https://secure.phabricator.com/D17416
Summary: We moved to having "no data" strings render in italics, but sometimes it doesn't make sense. This renders out the panel a little more expected.
Test Plan: Clean install of Phabricator, read home page activity box.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17415
Summary: Ref T12270. Moves badges into their own page and menu item. Capable of displaying hundreds of useful tokens of appreciation and dedication.
Test Plan:
Test blank state, mobile, awards badges.
{F3284139}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17410
Summary:
Ref T12296. This cache is used to cache Git ref heads (branches, tags, etc). Reasonable repositories may have more than 2048 of these.
When we miss the cache, we need to single-get refs to check them, which is relatively expensive.
Increasing the size of the cache to 65535 should only require about 7.5MB of RAM.
Additionally, fill only as much of the cache as actually fits. The FIFO nature of the cache can get us into trouble otherwise.
If we insert "A, B, C, D" and then lookup A, B, C, D, but the cache has maximum size 3, we get this:
- Insert A, B, C, D: cache is now "B, C, D".
- Lookup A: miss, single get, insert, purge, cache is now "C, D, A".
- Lookup B: miss, singel get, insert, purge, cache is now "D, A, B".
Test Plan:
- Reduced cache size to 5, observed reasonable behavior on the `array_slice()` locally with `bin/repository update` + `var_dump()`.
- Used this script to estimate the size of 65535 cache entries as 7.5MB:
```
epriestley@orbital ~ $ cat size.php
<?php
$cache = array();
$mem_start = memory_get_usage();
for ($ii = 0; $ii < 65535; $ii++) {
$cache[sha1($ii)] = true;
}
echo number_format(memory_get_usage() - $mem_start)." bytes\n";
epriestley@orbital ~ $ php -f size.php
7,602,176 bytes
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12296
Differential Revision: https://secure.phabricator.com/D17409
Summary:
Ref T12298. The trigger daemon already has routine long-term sleep, and few external events can impact when it should ideally wake up. The relevant events are:
- Someone creates a new Nuance source (ideally, we should wake up right away and start polling it).
- Someone creates a Calendar event about 16 minutes in the future (ideally, we should send them a reminder in about a minute).
- Someone changes GC config to be extremely aggressive (ideally, we should immediately respect the change).
None of these cases are very important. We don't hibernate for more than 3 minutes, so the worst case is that your Nuance source takes 3 minutes to start importing or your Calendar notification comes two minutes too late (13 minutes before the event instead of 15).
This change makes GC sightly more CPU-expensive on average: currently, we do a GC sweep every 4 hours. After this change, we'll end up doing one every 3 minutes, because we lose the fact that we did a sweep recently when the daemon restarts.
We could fix this by keeping track of when the last GC sweep was in the database, instead of in the Daemon process, but the cost of a sweep is normally very small so I don't plan to do this anytime soon.
Test Plan:
- Ran `bin/phd debug trigger`, saw daemon go through 3-minute hibernate + restart cycles.
- Ran `bin/phd debug task`, saw daemon run normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17408
Summary: Ref T6049. This moves Phurl to modular transactions.
Test Plan: Everything works here, add phurl, edit phurl, use phurl. Test various error states. Left a TODO on the validate dupe keys, not sure how to implement that in modular-land.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T6049
Differential Revision: https://secure.phabricator.com/D17405
Summary:
Fixes T12304. If you have a Herald rule which tries to add a commit author as an auditor, it fails validation when trying to apply.
Stop trying to apply these transactions, and explicitly tell the user why. Differential already uses a similar ruleset around reviewers, but Audit was using older code.
Test Plan:
- Wrote a Herald rule to add A, B and C as auditors.
- Committed as A.
- After change, saw B and C added with transacript guidance that A was the author.
{F3235660}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12304
Differential Revision: https://secure.phabricator.com/D17404
Summary:
Fixes T12302. Currently, we aren't merging multiple "AddAuditors" transactions correctly.
This can occur when Herald triggers multiple auditor rules.
Instead, merge them.
Test Plan:
- Wrote two different Herald rules that add auditors.
- Pushed a commit which triggered them.
- After the change, saw all the auditors get added correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12302
Differential Revision: https://secure.phabricator.com/D17403
Summary: Ref T12270. This converts Badges to modular transactions for editing and awarding.
Test Plan: Add Badge, edit badge, award and revoke... Still going to test this some more but feel free to comment on anything obviously wrong?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17402
Summary: Ref T12297. This could be fancier, but should make pulling profiles off `admin.phacility.com` significantly more realistic.
Test Plan: Dragged and dropped some profiles to upload them, then reviewed them via web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12297
Differential Revision: https://secure.phabricator.com/D17401
Summary:
Ref T12297. This slightly modernizes the XHProf UI. Not included here:
- Some of the code acts like samples have PHIDs, but they currently do not. I plan to add them in the next change.
- I've intentionally left the actual list untouched for now -- it has some old/buggy code (like `flag-6` is no longer an icon) that I'll fix in a future change.
Test Plan: {F3224264}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12297
Differential Revision: https://secure.phabricator.com/D17400
Summary: Ref T12297. When a page is generated with the profiler active, keep it active by adding a `__profile__` input to any forms we generate.
Test Plan: Hit Conduit API page with `__profile__` active, saw it reflected in forms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12297
Differential Revision: https://secure.phabricator.com/D17399
Summary:
Fixes T12172. Fixes T12060. This allows runtime code building CSS for mail to read CSS variables, then makes all the code do that.
It reverts the non-colorblind red/green to the colors in use before T12060, which seem better for non-colorblind users since no one really complained?
Test Plan:
- Viewed code diffs in Web UI.
- Viewed prose diffs in Web UI.
- Viewed code diffs in email.
- Viewed prose diffs in email.
All modes respected the accessibility color scheme.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12172, T12060
Differential Revision: https://secure.phabricator.com/D17269
Summary: Ref T10390. Basically hides policy controls when creating a panel on a dashboard. Shows when you edit them or through normal workflow. I think we should maybe also get rid of view policy? Not sure the benefit since results will be filtered anyways. Maybe Text panels? Not sure the use case.
Test Plan: Add a panel, edit a panel.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: hskiba, Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17393
Summary:
Fixes T12301. In D17372, this changed to use generic EditEngines instead of the proper runtime engine. Normally this doesn't matter, but can in this case.
After loading the configurations normally, swap their attached engines for the specific configured runtime engine we're currently executing.
Test Plan: Clicked "Create Form" from the Maniphest form list, saw it go to "Create Maniphest Form", not "Create Generic Meta-Form".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12301
Differential Revision: https://secure.phabricator.com/D17398
Summary:
Fixes T12306. Currently, we warn about daemons not running even if they're in normal "alive" states, particularly "waiting to restart after a failure".
This check was made more strict in D12088, back when we tried to version check running daemons. Since we implemented auto-restart-after-config-change we don't do this anymore, so it should be fine to make this more lax again.
Test Plan:
- Faked an exception for all tasks.
- Before patch: reloading the daemon setup error sometimes raised a false positive ("waiting" daemon detected as dead).
- After patch: daemon setup error no longer triggers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12306
Differential Revision: https://secure.phabricator.com/D17397
Summary:
Ref T12298. This updates `bin/phd` for minor changes to daemon configuration. In particular:
- Every daemon now has an autoscale pool (for trigger/pull, the maximum pool size is 1).
- Pools now have labels to make debugging a little easier.
- Some minor structural changes.
Test Plan: See D17389.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17390
Summary: Ref T10390. Simplifies dropdown by rolling out canUseInPanel in useless panels
Test Plan: Add a query panel, see less options.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17341
Summary:
See downstream <https://phabricator.kde.org/T5404>. This code was doing some `.firstChild` shenanigans which didn't survive some UI refactoring.
This whole UI is a little iffy but just unbreak it for now.
Test Plan: Allowed and rejected desktop notifications, got largely reasonable UI rendering.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17388
Summary: I broke this at the last second in D17374. `getStrList()` doesn't read arrays. It probably should (more modern analogs do) but don't rock the boat in the leadup to the release cut.
Test Plan: Hovered over a thing, saw a hovercard and no `getStrList()` error in my logs.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17383
Summary:
Ref T12268. Ref T12157. When you mention or interact with a user who is unlikely to be able to respond (for example, because their account is disabled), we try to show a colored dot to provide a hint about this.
Recently, we no longer send any normal mail to unverified addresses. However, the rules for showing a dot haven't been updated yet, so they only care about this if `auth.require-verification` is set. This can be misleading, because if you say `Hey @alice, what do you think about this?` and she hasn't verified her email, you may not get a response.
Update the rule so users with unverified email addresses get a grey dot in all cases. The hint is basically "you shouldn't expect a response from this user".
Make the meaning of this hint more clear on the hovercard and profile.
Also:
- Allow the non-ajax version of the hovercard page (which is basically only useful for testing hovercards) accept `?names=...` so you can just plug usernames, hashtags, etc., in there.
- Fix a bug where the user's join date was based on their profile creation date instead of account creation date on the hovercard. Users may not have a profile creation date (if they never changed any account details), and it may be different from their account creation date.
Test Plan: {F2998517}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12268, T12157
Differential Revision: https://secure.phabricator.com/D17374
Summary:
Fixes T12281. Some forms (like Settings) can't actually create new objects. Currently, though, you can select them and add them to profile menus; if you do, they fail when building an item.
Kick them out of the typeahead, and decline to render them in menus.
Test Plan:
Added "Create Settings" to a menu, no longer fatals after patch (item vanished from menu, still editable normally to get rid of it).
Tried to add another "Create Settings", no longer available in typehaead.
Added some normal stuff.
Viewed a choose-among-forms dropdown in Maniphest, which still worked normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12281
Differential Revision: https://secure.phabricator.com/D17372
Summary: See D14617. This could probably go either way but we don't currently need `$engine` in `newStandardEditField()`, so just get rid of it.
Test Plan: Edited a task with standard custom fields defined.
Reviewers: vrana, chad
Reviewed By: vrana
Differential Revision: https://secure.phabricator.com/D17370
Summary:
See D16676. When an export has an unsupported mode (bad database value, out-of-date object, etc) the intent of this code is to put it into the `<select />` so that you can save the form without silently changing the object.
However, it incorrectly calls `array_shift()` instead of `array_unshift()`.
Test Plan:
Edited a Calendar export with an invalid mode, saw the mode appear properly in the dropdown:
{F2957321}
Reviewers: vrana, chad
Reviewed By: vrana
Differential Revision: https://secure.phabricator.com/D17369
Summary: Ref T10798. Cleans up the UI a little and adds a sidenav.
Test Plan: Review badge and recipients in sandbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10798
Differential Revision: https://secure.phabricator.com/D17358
Summary: Fixes T10473. Clever, didn't know we could do this, but works well. Renders out the tab names by ', '.
Test Plan:
Add a tab panel, change some names, review transactions.
{F2929594}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10473
Differential Revision: https://secure.phabricator.com/D17359
Summary: Fixes T12248. Adds a flag for movable panels, and only allows those to be moved. Also cleaned up some CSS rules missing once a panel was drug into a new position.
Test Plan: Try to drag a tab panel content pane, cannot. Drag normal pane, see CSS, grab and drag same panel back, CSS looks the same.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12248
Differential Revision: https://secure.phabricator.com/D17356
Summary: Fixes T11449. Feels.... magical? Probably a more efficient way of doing this, but only 6 tabs so...
Test Plan: Create a tab panel in old UI. Edit panel in new UI. Create a panel in new UI, edit panel in new UI.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11449
Differential Revision: https://secure.phabricator.com/D17355
Summary: Fixes T10145. I went with "don't add two panels", since panels are easy to create, I expect this to be a reasonable limit until we have better use cases.
Test Plan: Try to add the same panel twice, get error. Add panel normally fine, move panels fine, edit panels fine.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10145
Differential Revision: https://secure.phabricator.com/D17351
