Skip to content

Commit 1e17fd3

Browse files
author
epriestley
committed
Modernize Conpherence access to user preferences
Summary: Ref T4103. Conpherence is doing some weird stuff and has its own redudnant settings object. - Get rid of `ConpherenceSettings`. - Use `getUserSetting()` instead of `loadPreferences()`. - When applying transactions, add a new mechanism to efficiently prefill caches (this will still work anyway, but it's slower if we don't bulk-fetch). Test Plan: - Changed global Conpherence setting. - Created a new Conpherence, saw setting set to global default. - Changed local room setting. - Submitted messages. - Saw cache prefill for all particpiants in database. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4103 Differential Revision: https://secure.phabricator.com/D16025
1 parent 9a076b7 commit 1e17fd3

10 files changed

+200
-79
lines changed

src/__phutil_library_map__.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,6 @@
312312
'ConpherenceRoomListController' => 'applications/conpherence/controller/ConpherenceRoomListController.php',
313313
'ConpherenceRoomTestCase' => 'applications/conpherence/__tests__/ConpherenceRoomTestCase.php',
314314
'ConpherenceSchemaSpec' => 'applications/conpherence/storage/ConpherenceSchemaSpec.php',
315-
'ConpherenceSettings' => 'applications/conpherence/constants/ConpherenceSettings.php',
316315
'ConpherenceTestCase' => 'applications/conpherence/__tests__/ConpherenceTestCase.php',
317316
'ConpherenceThread' => 'applications/conpherence/storage/ConpherenceThread.php',
318317
'ConpherenceThreadIndexEngineExtension' => 'applications/conpherence/engineextension/ConpherenceThreadIndexEngineExtension.php',
@@ -3636,6 +3635,7 @@
36363635
'PhabricatorUserPreferencesPHIDType' => 'applications/settings/phid/PhabricatorUserPreferencesPHIDType.php',
36373636
'PhabricatorUserPreferencesQuery' => 'applications/settings/query/PhabricatorUserPreferencesQuery.php',
36383637
'PhabricatorUserPreferencesTransaction' => 'applications/settings/storage/PhabricatorUserPreferencesTransaction.php',
3638+
'PhabricatorUserPreferencesTransactionQuery' => 'applications/settings/query/PhabricatorUserPreferencesTransactionQuery.php',
36393639
'PhabricatorUserProfile' => 'applications/people/storage/PhabricatorUserProfile.php',
36403640
'PhabricatorUserProfileEditor' => 'applications/people/editor/PhabricatorUserProfileEditor.php',
36413641
'PhabricatorUserRealNameField' => 'applications/people/customfield/PhabricatorUserRealNameField.php',
@@ -4565,7 +4565,6 @@
45654565
'ConpherenceRoomListController' => 'ConpherenceController',
45664566
'ConpherenceRoomTestCase' => 'ConpherenceTestCase',
45674567
'ConpherenceSchemaSpec' => 'PhabricatorConfigSchemaSpec',
4568-
'ConpherenceSettings' => 'ConpherenceConstants',
45694568
'ConpherenceTestCase' => 'PhabricatorTestCase',
45704569
'ConpherenceThread' => array(
45714570
'ConpherenceDAO',
@@ -8439,6 +8438,7 @@
84398438
'PhabricatorUserPreferencesPHIDType' => 'PhabricatorPHIDType',
84408439
'PhabricatorUserPreferencesQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
84418440
'PhabricatorUserPreferencesTransaction' => 'PhabricatorApplicationTransaction',
8441+
'PhabricatorUserPreferencesTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
84428442
'PhabricatorUserProfile' => 'PhabricatorUserDAO',
84438443
'PhabricatorUserProfileEditor' => 'PhabricatorApplicationTransactionEditor',
84448444
'PhabricatorUserRealNameField' => 'PhabricatorUserCustomField',

src/applications/conpherence/constants/ConpherenceSettings.php

Lines changed: 0 additions & 22 deletions
This file was deleted.

src/applications/conpherence/controller/ConpherenceUpdateController.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,14 @@ public function handleRequest(AphrontRequest $request) {
127127
}
128128
$participant->setSettings(array('notifications' => $notifications));
129129
$participant->save();
130+
131+
$label = PhabricatorConpherenceNotificationsSetting::getSettingLabel(
132+
$notifications);
133+
130134
$result = pht(
131135
'Updated notification settings to "%s".',
132-
ConpherenceSettings::getHumanString($notifications));
136+
$label);
137+
133138
return id(new AphrontAjaxResponse())
134139
->setContent($result);
135140
break;

src/applications/conpherence/controller/ConpherenceWidgetController.php

Lines changed: 10 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,6 @@
22

33
final class ConpherenceWidgetController extends ConpherenceController {
44

5-
private $userPreferences;
6-
7-
public function setUserPreferences(PhabricatorUserPreferences $pref) {
8-
$this->userPreferences = $pref;
9-
return $this;
10-
}
11-
12-
public function getUserPreferences() {
13-
return $this->userPreferences;
14-
}
15-
165
public function shouldAllowPublic() {
176
return true;
187
}
@@ -35,8 +24,6 @@ public function handleRequest(AphrontRequest $request) {
3524
}
3625
$this->setConpherence($conpherence);
3726

38-
$this->setUserPreferences($user->loadPreferences());
39-
4027
switch ($request->getStr('widget')) {
4128
case 'widgets-people':
4229
$content = $this->renderPeopleWidgetPaneContent();
@@ -143,28 +130,24 @@ private function renderSettingsWidgetPaneContent() {
143130
),
144131
$text);
145132
}
146-
$default = ConpherenceSettings::EMAIL_ALWAYS;
147-
$preference = $this->getUserPreferences();
148-
if ($preference) {
149-
$default = $preference->getPreference(
150-
PhabricatorUserPreferences::PREFERENCE_CONPH_NOTIFICATIONS,
151-
ConpherenceSettings::EMAIL_ALWAYS);
152-
}
133+
$notification_key = PhabricatorConpherenceNotificationsSetting::SETTINGKEY;
134+
$notification_default = $viewer->getUserSetting($notification_key);
135+
153136
$settings = $participant->getSettings();
154137
$notifications = idx(
155138
$settings,
156139
'notifications',
157-
$default);
140+
$notification_default);
158141
$options = id(new AphrontFormRadioButtonControl())
159142
->addButton(
160-
ConpherenceSettings::EMAIL_ALWAYS,
161-
ConpherenceSettings::getHumanString(
162-
ConpherenceSettings::EMAIL_ALWAYS),
143+
PhabricatorConpherenceNotificationsSetting::VALUE_CONPHERENCE_EMAIL,
144+
PhabricatorConpherenceNotificationsSetting::getSettingLabel(
145+
PhabricatorConpherenceNotificationsSetting::VALUE_CONPHERENCE_EMAIL),
163146
'')
164147
->addButton(
165-
ConpherenceSettings::NOTIFICATIONS_ONLY,
166-
ConpherenceSettings::getHumanString(
167-
ConpherenceSettings::NOTIFICATIONS_ONLY),
148+
PhabricatorConpherenceNotificationsSetting::VALUE_CONPHERENCE_NOTIFY,
149+
PhabricatorConpherenceNotificationsSetting::getSettingLabel(
150+
PhabricatorConpherenceNotificationsSetting::VALUE_CONPHERENCE_NOTIFY),
168151
'')
169152
->setName('notifications')
170153
->setValue($notifications);

src/applications/conpherence/editor/ConpherenceEditor.php

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -541,26 +541,29 @@ protected function getMailTo(PhabricatorLiskDAO $object) {
541541

542542
$participant_phids = mpull($participants, 'getParticipantPHID');
543543

544-
$preferences = id(new PhabricatorUserPreferencesQuery())
544+
$users = id(new PhabricatorPeopleQuery())
545545
->setViewer(PhabricatorUser::getOmnipotentUser())
546-
->withUserPHIDs($participant_phids)
546+
->withPHIDs($participant_phids)
547+
->needUserSettings(true)
547548
->execute();
548-
$preferences = mpull($preferences, null, 'getUserPHID');
549+
$users = mpull($users, null, 'getPHID');
550+
551+
$notification_key = PhabricatorConpherenceNotificationsSetting::SETTINGKEY;
552+
$notification_email =
553+
PhabricatorConpherenceNotificationsSetting::VALUE_CONPHERENCE_EMAIL;
549554

550555
foreach ($participants as $phid => $participant) {
551-
$default = ConpherenceSettings::EMAIL_ALWAYS;
552-
$preference = idx($preferences, $phid);
553-
if ($preference) {
554-
$default = $preference->getPreference(
555-
PhabricatorUserPreferences::PREFERENCE_CONPH_NOTIFICATIONS,
556-
ConpherenceSettings::EMAIL_ALWAYS);
556+
$user = idx($users, $phid);
557+
if ($user) {
558+
$default = $user->getUserSetting($notification_key);
559+
} else {
560+
$default = $notification_email;
557561
}
562+
558563
$settings = $participant->getSettings();
559-
$notifications = idx(
560-
$settings,
561-
'notifications',
562-
$default);
563-
if ($notifications == ConpherenceSettings::EMAIL_ALWAYS) {
564+
$notifications = idx($settings, 'notifications', $default);
565+
566+
if ($notifications == $notification_email) {
564567
$to_phids[] = $phid;
565568
}
566569
}

src/applications/people/cache/PhabricatorUserPreferencesCacheType.php

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,16 @@ public function canManageKey($key) {
2020
public function newValueForUsers($key, array $users) {
2121
$viewer = $this->getViewer();
2222

23+
$user_phids = mpull($users, 'getPHID');
24+
2325
$preferences = id(new PhabricatorUserPreferencesQuery())
2426
->setViewer($viewer)
25-
->withUserPHIDs(mpull($users, 'getPHID'))
27+
->withUserPHIDs($user_phids)
2628
->execute();
2729

28-
return mpull($preferences, 'getPreferences', 'getUserPHID');
30+
$empty = array_fill_keys($user_phids, array());
31+
32+
return mpull($preferences, 'getPreferences', 'getUserPHID') + $empty;
2933
}
3034

3135
}

src/applications/people/query/PhabricatorPeopleQuery.php

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ final class PhabricatorPeopleQuery
2323
private $needProfileImage;
2424
private $needAvailability;
2525
private $needBadges;
26+
private $cacheKeys = array();
2627

2728
public function withIDs(array $ids) {
2829
$this->ids = $ids;
@@ -119,6 +120,18 @@ public function needBadges($need) {
119120
return $this;
120121
}
121122

123+
public function needUserSettings($need) {
124+
$cache_key = PhabricatorUserPreferencesCacheType::KEY_PREFERENCES;
125+
126+
if ($need) {
127+
$this->cacheKeys[$cache_key] = true;
128+
} else {
129+
unset($this->cacheKeys[$cache_key]);
130+
}
131+
132+
return $this;
133+
}
134+
122135
public function newResultObject() {
123136
return new PhabricatorUser();
124137
}
@@ -238,6 +251,8 @@ protected function didFilterPage(array $users) {
238251
}
239252
}
240253

254+
$this->fillUserCaches($users);
255+
241256
return $users;
242257
}
243258

@@ -481,4 +496,91 @@ private function rebuildAvailabilityCache(array $rebuild) {
481496
}
482497
}
483498

499+
private function fillUserCaches(array $users) {
500+
if (!$this->cacheKeys) {
501+
return;
502+
}
503+
504+
$user_map = mpull($users, null, 'getPHID');
505+
$keys = array_keys($this->cacheKeys);
506+
507+
$hashes = array();
508+
foreach ($keys as $key) {
509+
$hashes[] = PhabricatorHash::digestForIndex($key);
510+
}
511+
512+
// First, pull any available caches. If we wanted to be particularly clever
513+
// we could do this with JOINs in the main query.
514+
515+
$cache_table = new PhabricatorUserCache();
516+
$cache_conn = $cache_table->establishConnection('r');
517+
518+
$cache_data = queryfx_all(
519+
$cache_conn,
520+
'SELECT cacheKey, userPHID, cacheData FROM %T
521+
WHERE cacheIndex IN (%Ls) AND userPHID IN (%Ls)',
522+
$cache_table->getTableName(),
523+
$hashes,
524+
array_keys($user_map));
525+
526+
$need = array();
527+
528+
$cache_data = igroup($cache_data, 'userPHID');
529+
foreach ($user_map as $user_phid => $user) {
530+
$raw_rows = idx($cache_data, $user_phid, array());
531+
if (!$raw_rows) {
532+
continue;
533+
}
534+
$raw_data = ipull($raw_rows, 'cacheData', 'cacheKey');
535+
536+
foreach ($keys as $key) {
537+
if (isset($raw_data[$key]) || array_key_exists($key, $raw_data)) {
538+
continue;
539+
}
540+
$need[$key][$user_phid] = $user;
541+
}
542+
543+
$user->attachRawCacheData($raw_data);
544+
}
545+
546+
// If we missed any cache values, bulk-construct them now. This is
547+
// usually much cheaper than generating them on-demand for each user
548+
// record.
549+
550+
if (!$need) {
551+
return;
552+
}
553+
554+
$writes = array();
555+
foreach ($need as $cache_key => $need_users) {
556+
$type = PhabricatorUserCacheType::getCacheTypeForKey($cache_key);
557+
if (!$type) {
558+
continue;
559+
}
560+
561+
$data = $type->newValueForUsers($cache_key, $need_users);
562+
563+
foreach ($data as $user_phid => $value) {
564+
$raw_value = $type->getValueForStorage($value);
565+
$data[$user_phid] = $raw_value;
566+
$writes[] = array(
567+
'userPHID' => $user_phid,
568+
'key' => $cache_key,
569+
'type' => $type,
570+
'value' => $raw_value,
571+
);
572+
}
573+
574+
foreach ($need_users as $user_phid => $user) {
575+
if (isset($data[$user_phid]) || array_key_exists($user_phid, $data)) {
576+
$user->attachRawCacheData(
577+
array(
578+
$cache_key => $data[$user_phid],
579+
));
580+
}
581+
}
582+
}
583+
584+
PhabricatorUserCache::writeCaches($writes);
585+
}
484586
}

src/applications/people/storage/PhabricatorUserCache.php

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -42,27 +42,54 @@ public static function writeCache(
4242
$key,
4343
$user_phid,
4444
$raw_value) {
45+
self::writeCaches(
46+
array(
47+
array(
48+
'type' => $type,
49+
'key' => $key,
50+
'userPHID' => $user_phid,
51+
'value' => $raw_value,
52+
),
53+
));
54+
}
4555

56+
public static function writeCaches(array $values) {
4657
if (PhabricatorEnv::isReadOnly()) {
4758
return;
4859
}
4960

61+
if (!$values) {
62+
return;
63+
}
64+
5065
$table = new self();
5166
$conn_w = $table->establishConnection('w');
5267

68+
$sql = array();
69+
foreach ($values as $value) {
70+
$key = $value['key'];
71+
72+
$sql[] = qsprintf(
73+
$conn_w,
74+
'(%s, %s, %s, %s, %s)',
75+
$value['userPHID'],
76+
PhabricatorHash::digestForIndex($key),
77+
$key,
78+
$value['value'],
79+
$value['type']->getUserCacheType());
80+
}
81+
5382
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
5483

55-
queryfx(
56-
$conn_w,
57-
'INSERT INTO %T (userPHID, cacheIndex, cacheKey, cacheData, cacheType)
58-
VALUES (%s, %s, %s, %s, %s)
59-
ON DUPLICATE KEY UPDATE cacheData = VALUES(cacheData)',
60-
$table->getTableName(),
61-
$user_phid,
62-
PhabricatorHash::digestForIndex($key),
63-
$key,
64-
$raw_value,
65-
$type->getUserCacheType());
84+
foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
85+
queryfx(
86+
$conn_w,
87+
'INSERT INTO %T (userPHID, cacheIndex, cacheKey, cacheData, cacheType)
88+
VALUES %Q
89+
ON DUPLICATE KEY UPDATE cacheData = VALUES(cacheData)',
90+
$table->getTableName(),
91+
$chunk);
92+
}
6693

6794
unset($unguarded);
6895
}

0 commit comments

Comments
 (0)