Page MenuHomePhabricator

Extends Phrequent search capabilities
AbandonedPublic

Authored by witrin on Aug 6 2014, 11:49 AM.

Details

Summary

Allows to search over a time range. Further this brings the posibility to export a search result and to create/edit single entries.

Test Plan

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 OK
SeverityLocationCodeMessage
Advicesrc/applications/phrequent/controller/PhrequentExportController.php:51XHP16TODO Comment
Unit
Unit Tests OK
Build Status
Buildable 2062
Build 2063: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

witrin retitled this revision from to Extends Phrequent search capabilities.Aug 6 2014, 11:49 AM
witrin updated this object.
witrin edited the test plan for this revision. (Show Details)
witrin added a reviewer: epriestley.
witrin added a subscriber: witrin.
witrin updated this revision to Diff 24448.
witrin edited the test plan for this revision. (Show Details)Aug 6 2014, 11:53 AM
witrin edited edge metadata.
witrin updated this object.Aug 6 2014, 12:11 PM
witrin updated this revision to Diff 24449.Aug 6 2014, 3:40 PM
  • 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.

witrin edited the test plan for this revision. (Show Details)Aug 8 2014, 12:10 AM
epriestley requested changes to this revision.

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.

This revision now requires changes to proceed.Aug 12 2014, 1:58 PM

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.

witrin abandoned this revision.Oct 10 2014, 3:05 PM

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?