Changing criteria for showing badges in object timeline view
ClosedPublic
Actions

Authored by lpriestley on Mar 28 2016, 10:09 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T8941: Badges don't show in certain Timeline conditions

Commits

rP0ea738f18fd2: Changing criteria for showing badges in object timeline view

Summary

Ref T8941

Test Plan

Create an object and create multiple transactions, some time apart to ensure that time clumping isn't interfering. Make sure that events that are large enough to have a dropdown menu show badges under author pic.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

lpriestley updated this revision to Diff 37469.Mar 28 2016, 10:09 PM

lpriestley retitled this revision from to Changing criteria for showing badges in object timeline view.

lpriestley updated this object.

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

lpriestley added a reviewer: epriestley.

lpriestley added a task: T8941: Badges don't show in certain Timeline conditions.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMar 28 2016, 10:09 PM

Herald added a subscriber: Korvin. · View Herald Transcript

lpriestley added inline comments.Mar 28 2016, 10:13 PM

src/view/phui/PHUITimelineView.php
226–227	This was filtering out an event that had a large icon, despite having children. I think the correct fix would be to have a separate attribute on the event view that determines whether events should have badges or not, but I'm not sure if there's a better way to identify the events that need to be unset than by calling `getHideCommentOptions`.
227	This was filtering out an event that had a large icon, despite having children. I think the correct fix would be to have a separate attribute on the event view that determines whether events should have badges or not, but I'm not sure if there's a better way to identify the events that need to be unset than by calling `getHideCommentOptions`.

Here are a couple of cases this seems to get wrong.

Preview with a comment doesn't show badge, but should:

These individual stories with small icons show badges, but should not:

(It's fine to add a separate attribute if that looks like the best approach.)

This revision now requires changes to proceed.Mar 28 2016, 10:26 PM

Are you sure the profile image in the preview should have badges? Depending on the timing and on other things happening with the object, the preview won't be able to predict if it's a major event or not...

Not sure why this works better, but moved the badge-showing logic to the event view

I do think we should show badges on previews eventually, but that didn't work before so this change strictly makes the behavior better.

This change will cause us to fetch some badge data which we may not end up showing. The intent of the old code was to avoid fetching badges unless we needed them. However, fetching this data is cheap (we issue the same number of queries, just fetch slightly more data) and I think fixing the behavior is more important than micro-optimizing the data fetching.

src/view/phui/PHUITimelineEventView.php
363	Can this just be a local variable instead? It looks like it's only used in this method.

This revision is now accepted and ready to land.Mar 28 2016, 11:10 PM

Switching to local variable

What is your reasoning for wanting to show badges in previews?

Primarily, it makes them a more accurate representation of what the transaction will actually look like when submitted.

You're right that in some cases we'll show a preview which isn't the same as what actually posts, but I think we'll almost always guess whether it will be a major event or not correctly, since the primary factor there is "did the user post a comment?" and race conditions don't change/suppress comments.

Okay I agree that it would be nice to show badges under all instance of the profile image if it has the appropriate size.

src/view/phui/PHUITimelineEventView.php
500–501	It seems like this is the only code in this file that deals with `getIsPreview`, but I still don't understand how the profile image gets attached to the comment preview at all. Could you point me to that code?