Fix an issue where 'Attending' would appear on calendar view unnecessarily
ClosedPublic
Actions

Authored by jcowgar on Feb 7 2016, 4:08 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T10295: Remove 'Attending' from Calendar Search Results display when empty

Commits

Restricted Diffusion Commit
rP604b78095349: Fix an issue where 'Attending' would appear on calendar view unnecessarily

Required Signatures

L28 Phacility Individual Contributor License Agreement

Summary

Ref T10295

Viewing Upcoming Events in the calendar would display 'Attending: ' even if there were not attendees. This caused confusion, such as 'Is it telling me I am "Attending?"'
When a calendar event has no attendees, simply do not display the 'Attending: ' label

Test Plan

Add a new event with no one attending.
Add a new event with one or more attendees.
View the Upcoming Events query of the Calendar app.
Notice how the one with no attendees does not show 'Attending: ' while the other with attendees will show the already existing 'Attending: jdoe, ssmith' label.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jcowgar updated this revision to Diff 36711.Feb 7 2016, 4:08 PM

jcowgar retitled this revision from to Fix an issue where 'Attending' would appear on calendar view unnecessarily.

jcowgar updated this object.

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

jcowgar added a task: T10295: Remove 'Attending' from Calendar Search Results display when empty.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptFeb 7 2016, 4:08 PM

Herald added a required legal document: L28 Phacility Individual Contributor License Agreement. · View Herald Transcript

Herald added a subscriber: epriestley. · View Herald Transcript

jcowgar planned changes to this revision.Feb 7 2016, 4:14 PM

jcowgar added inline comments.

src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php
269	Wrong variable name, d'oh! Fixing.

No longer reuse array variable for attendees and display

Minor inlines.

src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php
276–277	By convention in this codebase, prefer `if ($attendees)` over `if (count($attendees) > 0)` for testing arrays for the presence of elements.
291–292	Instead of adding an empty attribute (which may cause us to render extra space or an extra separator character like a middot (`·`) now or in the future), prefer to add the attribute conditionally: if ($attendees) { // ... $item->addAttribute(...);

This revision now requires changes to proceed.Feb 7 2016, 4:34 PM

jcowgar marked an inline comment as done.Feb 7 2016, 4:42 PM

jcowgar added inline comments.

src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php
291–292	I was actually going to do that, but followed suit with the duration. Should I change the $duration as well or should that be done in a different commit/task/review? I am unsure of project policies here.

There's no hard-and-fast rule (and I'm being extra meticulous about these details because you're a new contributor).

In this case, I'd put both changes in one diff (they're small, conceptually related, and testable in parallel). I'd split them apart only if they were a good bit more substantive or complex.