Allows to search over a time range. Further this brings the posibility to export a search result and to create/edit single entries.
Details
- Reviewers
epriestley chad - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5149: Allow to add and edit Phrequent entries manually
T5150: Extend query by time restrictions
T5151: Allow to export query results from Phrequent - Required Signatures
L28 Phacility Individual Contributor License Agreement
Use the Phrequent search and you'll notice two additional fields to filter the result (T5150). Edit Phrequent entries by searching for your own tracked time and you'll see an edit button for each entry. Search tracked time of other users and you're not able to edit them (T5149). Export a search result by using the corresponding button in the search result at the bottom (T5151).
Diff Detail
- Repository
- rP Phabricator
- Branch
- phrequent
- Lint
Lint Warnings Severity Location Code Message Warning src/applications/phrequent/export/PhrequentExcelDefaultFormat.php:103 XHP30 Implicit Fallthrough Warning src/applications/phrequent/query/PhrequentUserTimeQuery.php:44 XHP9 Naming Conventions Warning src/applications/phrequent/query/PhrequentUserTimeQuery.php:45 XHP9 Naming Conventions Warning src/applications/phrequent/query/PhrequentUserTimeQuery.php:49 XHP9 Naming Conventions Warning src/applications/phrequent/query/PhrequentUserTimeQuery.php:50 XHP9 Naming Conventions Advice src/applications/phrequent/controller/PhrequentExportController.php:51 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 2061 Build 2062: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
- Modernize global search typeahead datasource in Phrequent edit controller
- Corrects lint warnings
Please let me know, if I can do anything to speed this up here (e.g. contribute tests), or if that feature is not desired at all.
This diff is doing a lot of stuff. Generally, I think the best way forward is to break it up in to smaller pieces, since the different parts have different technical and product barriers.
Search by IDs and Date Range: This is basically fine and can upstream immediately, but we should add (dateStarted) and (dateEnded) keys to the table. A couple of inlines. Separate this into a standalone diff and I can pull it after a couple tweaks.
Arbitrary Creation / Editing: This seems technically fine, but I'm not as sure about it from a product perspective. Can you discuss your use cases a little more? In particular, it seems odd that you can create time for other users. If we do pursue this, I think we should track who created the time record (e.g., epriestley noted that alincoln spent 6 hours on this.). If we're allowing record mutation, we should keep a transaction history. Finally, we don't allow future-end-times elsewhere yet. Basically, if we're dramatically expanding users' abilities to create and edit records, we need corresponding expansions in audibility of their actions.
Excel Export: This is fine from a product perspective but not great technically. This has a lot of copy/pasting from Maniphest and hand-rolls new UI. T5307 discusses building a consistent UI for these kinds of actions. This is conceptually fine, but the technical implementation needs to share more code with Maniphest and likely wait for (or, at least, take into account) T5307.
This would probably look something like defining a new excel export application, moving all the existing excel export code there, defining a PhabricatorSpreadsheetInterface which objects implement in order to get excel export, and then making all the excel code accept those objects instead of tasks or times. Both applications would pass a SearchEngine class plus a QueryKey to the Excel app, and it would run the query, load the objects, and perform the export. Only application-specific code (e.g., column definitions) should be left in Maniphest and Phrequent.
Search by IDs and Date Range
I'll add the keys and try to make an separate diff (see my question below).
Arbitrary Creation / Editing
In particular, it seems odd that you can create time for other users.
Ups! This should not be possible. Respectively we don't want that in any case. Currently (with this diff) you should only be able to create/edit your own entries. I think this is line 98 in the edit controller right? I'll fix that and try to make an separate diff (see my question below).
Excel Export
I agree totally with you! I wasn't happy ether to copy all that Excel stuff from Maniphest and then going an odd path to get this action into the search result. So it looks like this feature have to wait a while? But I would contribute when the infrastructure is ready! Is there any ticket about the missing Excel integration?
Separate Differentials
Regarding to that topic: I've tried that but I failed ;). arc diff didn't allow me to submit a single commit, for a good reason I guess. Instead it forces me to use a range including the HEAD. So is there any easy possibility to post a single commit with arc diff or is it necessary to cherry pick that in a new feature branch? Currently I've all commits together in a branch because we need them all in production.
Search by IDs and Date Range: This is basically fine and can upstream immediately, but we should add (dateStarted) and (dateEnded) keys to the table. A couple of inlines. Separate this into a standalone diff and I can pull it after a couple tweaks.
@epriestley I've created a revision for this (D10674). Is there any chance to get this pulled or at least reviewed?