Add basic feed stories for Conpherence
ClosedPublic
Actions

Authored by chad on Sep 16 2016, 12:05 AM.

Details

Reviewers

epriestley

Maniphest Tasks

T11645: Generate feed story for new (all user) rooms?

Commits

rP1ea6fe938744: Add basic feed stories for Conpherence

Summary

[Draft] Posting this up because feed is pulling getTitle and not getTitleForFeed and I'm super confused. Restarted phd and apache.

Test Plan

Create a new room, see link in feed. Change topic, see story, add people, don't see story.

Diff Detail

Repository

rP Phabricator

Branch

conph-feed (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 13737
Build 17733: Run Core Tests
Build 17732: arc lint + arc unit

Event Timeline

chad updated this revision to Diff 39858.Sep 16 2016, 12:05 AM

chad retitled this revision from to Add basic feed stories for Conpherence.

chad updated this object.

chad edited the test plan for this revision. (Show Details)

chad added a reviewer: epriestley.

Herald added a subscriber: Korvin. · View Herald TranscriptSep 16 2016, 12:05 AM

I'm super lost here why this would pull in getTitle....

epriestley added inline comments.Sep 16 2016, 12:14 AM

src/applications/conpherence/storage/ConpherenceTransaction.php
151	This just sets a variable called `$title`, it doesn't return it or do anything with it. Then it calls `parent::getTitleForFeed()`, which calls `getTitle()` as a fallback.

chad added inline comments.Sep 16 2016, 12:16 AM

src/applications/conpherence/storage/ConpherenceTransaction.php
151	oh

some fixes... still weird on create

sorry, what's the taskmaster debug command? I'll put it on a sticky this time :(

Only thing I can't seem to get working is the first CREATE transaction, it doesn't trigger TYPE_TITLE. Seems to trigger PARTICIPANTS.

cspeckmim mentioned this in T11648: Jumping to an reply inline comment via URL anchor should highlight the comment node accurately.Sep 16 2016, 3:41 AM

Taskmaster debug command is:

$ bin/phd debug taskmaster

The creation stuff is sort of written questionably right now, but I think it's not worth correcting before moving to modular transactions to fix it properly.

"Participants" wins over "Title" as the topic for the feed story because both of these conditions are true:

it's earlier in the transaction list in ConpherenceNewRoomController; and
there's no getActionStrength() stuff to force the transactions into a different order.

Easiest fix is likely to reorder the transactions in ConpherenceNewRoomController so that TYPE_TITLE is first.

src/applications/conpherence/storage/ConpherenceTransaction.php
136	This probably misbehaves when renaming a room from "0" to "Zebra", or from "Zebra" to "0".
152–163	Likewise, we probably get into trouble here when setting the topic to or from "0".
160	This puts double quotes around a handle, which is unusual.

This revision is now accepted and ready to land.Sep 16 2016, 6:37 PM

This feels pretty reasonable to me overall, but miggghht? be weird in some cases? I think if we add DM explicitly, those shouldn't generate feed, but Rooms with n participants should.