Skip to content

Commit 7a48ba9

Browse files
PringelsPringels
committed
feat(settings): persist user management column visibility
Previously, column visibility settings were stored in localStorage, causing them to be lost when logging out or switching browsers. This change moves the persistence to the database as user preferences. It also refactors the frontend to use clean `userList.*` keys for better consistency between the store and the API. Signed-off-by: Peter Ringelmann <[email protected]>
1 parent 0eca299 commit 7a48ba9

File tree

11 files changed

+183
-40
lines changed

11 files changed

+183
-40
lines changed

‎apps/settings/lib/Controller/UsersController.php‎

Lines changed: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,11 @@
3838
use OCP\AppFramework\Http\TemplateResponse;
3939
use OCP\AppFramework\Services\IInitialState;
4040
use OCP\BackgroundJob\IJobList;
41+
use OCP\Config\IUserConfig;
4142
use OCP\Encryption\IManager;
4243
use OCP\EventDispatcher\IEventDispatcher;
4344
use OCP\Group\ISubAdmin;
45+
use OCP\IAppConfig;
4446
use OCP\IConfig;
4547
use OCP\IGroup;
4648
use OCP\IGroupManager;
@@ -59,13 +61,24 @@ class UsersController extends Controller {
5961
/** Limit for counting users for subadmins, to avoid spending too much time */
6062
private const COUNT_LIMIT_FOR_SUBADMINS = 999;
6163

64+
public const ALLOWED_USER_PREFERENCES = [
65+
'user_list_show_storage_path',
66+
'user_list_show_user_backend',
67+
'user_list_show_first_login',
68+
'user_list_show_last_login',
69+
'user_list_show_new_user_form',
70+
'user_list_show_languages',
71+
];
72+
6273
public function __construct(
6374
string $appName,
6475
IRequest $request,
6576
private UserManager $userManager,
6677
private IGroupManager $groupManager,
6778
private IUserSession $userSession,
6879
private IConfig $config,
80+
private IAppConfig $appConfig,
81+
private IUserConfig $userConfig,
6982
private IL10N $l10n,
7083
private IMailer $mailer,
7184
private IFactory $l10nFactory,
@@ -191,12 +204,12 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA
191204
}
192205

193206
/* QUOTAS PRESETS */
194-
$quotaPreset = $this->parseQuotaPreset($this->config->getAppValue('files', 'quota_preset', '1 GB, 5 GB, 10 GB'));
195-
$allowUnlimitedQuota = $this->config->getAppValue('files', 'allow_unlimited_quota', '1') === '1';
207+
$quotaPreset = $this->parseQuotaPreset($this->appConfig->getValueString('files', 'quota_preset', '1 GB, 5 GB, 10 GB'));
208+
$allowUnlimitedQuota = $this->appConfig->getValueBool('files', 'allow_unlimited_quota', true);
196209
if (!$allowUnlimitedQuota && count($quotaPreset) > 0) {
197-
$defaultQuota = $this->config->getAppValue('files', 'default_quota', $quotaPreset[0]);
210+
$defaultQuota = $this->appConfig->getValueString('files', 'default_quota', $quotaPreset[0]);
198211
} else {
199-
$defaultQuota = $this->config->getAppValue('files', 'default_quota', 'none');
212+
$defaultQuota = $this->appConfig->getValueString('files', 'default_quota', 'none');
200213
}
201214

202215
$event = new BeforeTemplateRenderedEvent();
@@ -219,7 +232,7 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA
219232
$serverData['isDelegatedAdmin'] = $isDelegatedAdmin;
220233
$serverData['sortGroups'] = $forceSortGroupByName
221234
? MetaData::SORT_GROUPNAME
222-
: (int)$this->config->getAppValue('core', 'group.sortBy', (string)MetaData::SORT_USERCOUNT);
235+
: (int)$this->appConfig->getValueString('core', 'group.sortBy', (string)MetaData::SORT_USERCOUNT);
223236
$serverData['forceSortGroupByName'] = $forceSortGroupByName;
224237
$serverData['quotaPreset'] = $quotaPreset;
225238
$serverData['allowUnlimitedQuota'] = $allowUnlimitedQuota;
@@ -230,9 +243,13 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA
230243
// Settings
231244
$serverData['defaultQuota'] = $defaultQuota;
232245
$serverData['canChangePassword'] = $canChangePassword;
233-
$serverData['newUserGenerateUserID'] = $this->config->getAppValue('core', 'newUser.generateUserID', 'no') === 'yes';
234-
$serverData['newUserRequireEmail'] = $this->config->getAppValue('core', 'newUser.requireEmail', 'no') === 'yes';
235-
$serverData['newUserSendEmail'] = $this->config->getAppValue('core', 'newUser.sendEmail', 'yes') === 'yes';
246+
$serverData['newUserGenerateUserID'] = $this->appConfig->getValueBool('core', 'newUser.generateUserID', false);
247+
$serverData['newUserRequireEmail'] = $this->appConfig->getValueBool('core', 'newUser.requireEmail', false);
248+
$serverData['newUserSendEmail'] = $this->appConfig->getValueBool('core', 'newUser.sendEmail', true);
249+
$serverData['showConfig'] = [];
250+
foreach (self::ALLOWED_USER_PREFERENCES as $key) {
251+
$serverData['showConfig'][$key] = $this->userConfig->getValueBool($uid, $this->appName, $key, false);
252+
}
236253

237254
$this->initialState->provideInitialState('usersSettings', $serverData);
238255

@@ -250,13 +267,22 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA
250267
*/
251268
#[AuthorizedAdminSetting(settings:Users::class)]
252269
public function setPreference(string $key, string $value): JSONResponse {
253-
$allowed = ['newUser.sendEmail', 'group.sortBy'];
254-
if (!in_array($key, $allowed, true)) {
255-
return new JSONResponse([], Http::STATUS_FORBIDDEN);
270+
switch ($key) {
271+
case 'newUser.sendEmail':
272+
$this->appConfig->setValueBool('core', $key, $value === 'yes');
273+
break;
274+
case 'group.sortBy':
275+
$this->appConfig->setValueString('core', $key, $value);
276+
break;
277+
default:
278+
if (in_array($key, self::ALLOWED_USER_PREFERENCES, true)) {
279+
$this->userConfig->setValueBool($this->userSession->getUser()->getUID(), $this->appName, $key, $value === 'true');
280+
} else {
281+
return new JSONResponse([], Http::STATUS_FORBIDDEN);
282+
}
283+
break;
256284
}
257285

258-
$this->config->setAppValue('core', $key, $value);
259-
260286
return new JSONResponse([]);
261287
}
262288

‎apps/settings/src/components/UserList.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ export default {
313313
},
314314
315315
closeDialog() {
316-
this.$store.commit('setShowConfig', {
316+
this.$store.dispatch('setShowConfig', {
317317
key: 'showNewUserForm',
318318
value: false,
319319
})

‎apps/settings/src/components/Users/UserSettingsDialog.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ export default {
297297
},
298298
299299
setShowConfig(key, status) {
300-
this.$store.commit('setShowConfig', { key, value: status })
300+
this.$store.dispatch('setShowConfig', { key, value: status })
301301
},
302302
303303
/**

‎apps/settings/src/main-apps-users-management.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ sync(store, router)
3030

3131
const pinia = createPinia()
3232

33+
// Migrate legacy local storage settings to the database
34+
store.dispatch('migrateLocalStorage')
35+
3336
export default new Vue({
3437
router,
3538
store,

‎apps/settings/src/store/users.js‎

Lines changed: 69 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
*/
55

66
import axios from '@nextcloud/axios'
7-
import { getBuilder } from '@nextcloud/browser-storage'
87
import { getCapabilities } from '@nextcloud/capabilities'
98
import { showError } from '@nextcloud/dialogs'
109
import { parseFileSize } from '@nextcloud/files'
@@ -17,8 +16,6 @@ import api from './api.js'
1716

1817
const usersSettings = loadState('settings', 'usersSettings', {})
1918

20-
const localStorage = getBuilder('settings').persist(true).build()
21-
2219
const defaults = {
2320
/**
2421
* @type {import('../views/user-types').IGroup}
@@ -47,12 +44,12 @@ const state = {
4744
disabledUsersLimit: 25,
4845
userCount: usersSettings.userCount ?? 0,
4946
showConfig: {
50-
showStoragePath: localStorage.getItem('account_settings__showStoragePath') === 'true',
51-
showUserBackend: localStorage.getItem('account_settings__showUserBackend') === 'true',
52-
showFirstLogin: localStorage.getItem('account_settings__showFirstLogin') === 'true',
53-
showLastLogin: localStorage.getItem('account_settings__showLastLogin') === 'true',
54-
showNewUserForm: localStorage.getItem('account_settings__showNewUserForm') === 'true',
55-
showLanguages: localStorage.getItem('account_settings__showLanguages') === 'true',
47+
showStoragePath: usersSettings.showConfig.user_list_show_storage_path,
48+
showUserBackend: usersSettings.showConfig.user_list_show_user_backend,
49+
showFirstLogin: usersSettings.showConfig.user_list_show_first_login,
50+
showLastLogin: usersSettings.showConfig.user_list_show_last_login,
51+
showNewUserForm: usersSettings.showConfig.user_list_show_new_user_form,
52+
showLanguages: usersSettings.showConfig.user_list_show_languages,
5653
},
5754
}
5855

@@ -241,7 +238,6 @@ const mutations = {
241238
},
242239

243240
setShowConfig(state, { key, value }) {
244-
localStorage.setItem(`account_settings__${key}`, JSON.stringify(value))
245241
state.showConfig[key] = value
246242
},
247243

@@ -801,6 +797,68 @@ const actions = {
801797
.catch((error) => { throw error })
802798
}).catch((error) => context.commit('API_FAILURE', { userid, error }))
803799
},
800+
/**
801+
* Migrate local storage keys to database
802+
*
803+
* @param {object} context store context
804+
* @param context.commit
805+
*/
806+
migrateLocalStorage({ commit }) {
807+
const preferences = {
808+
showStoragePath: 'user_list_show_storage_path',
809+
showUserBackend: 'user_list_show_user_backend',
810+
showFirstLogin: 'user_list_show_first_login',
811+
showLastLogin: 'user_list_show_last_login',
812+
showNewUserForm: 'user_list_show_new_user_form',
813+
showLanguages: 'user_list_show_languages',
814+
}
815+
816+
for (const [key, dbKey] of Object.entries(preferences)) {
817+
const localKey = `account_settings__${key}`
818+
const localValue = window.localStorage.getItem(localKey)
819+
if (localValue === null) {
820+
continue
821+
}
822+
823+
const value = localValue === 'true'
824+
commit('setShowConfig', { key, value })
825+
826+
axios.post(generateUrl(`/settings/users/preferences/${dbKey}`), {
827+
value: value ? 'true' : 'false',
828+
}).then(() => {
829+
window.localStorage.removeItem(localKey)
830+
}).catch((error) => {
831+
logger.error(`Failed to migrate preference ${key}`, { error })
832+
})
833+
}
834+
},
835+
836+
/**
837+
* Set show config
838+
*
839+
* @param {object} context store context
840+
* @param {object} options destructuring object
841+
* @param {string} options.key Key to set
842+
* @param {boolean} options.value Value to set
843+
*/
844+
setShowConfig(context, { key, value }) {
845+
context.commit('setShowConfig', { key, value })
846+
const keyMap = {
847+
showStoragePath: 'user_list_show_storage_path',
848+
showUserBackend: 'user_list_show_user_backend',
849+
showFirstLogin: 'user_list_show_first_login',
850+
showLastLogin: 'user_list_show_last_login',
851+
showNewUserForm: 'user_list_show_new_user_form',
852+
showLanguages: 'user_list_show_languages',
853+
}
854+
axios.post(generateUrl(`settings/users/preferences/${keyMap[key]}`), { value: value ? 'true' : 'false' })
855+
.catch((error) => logger.error(`Could not update ${key} preference`, { error }))
856+
},
804857
}
805858

806-
export default { state, mutations, getters, actions }
859+
export default {
860+
state,
861+
mutations,
862+
getters,
863+
actions,
864+
}

‎apps/settings/src/views/UserManagementNavigation.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ const isAdminOrDelegatedAdmin = computed(() => settings.value.isAdmin || setting
149149
* Open the new-user form dialog
150150
*/
151151
function showNewUserMenu() {
152-
store.commit('setShowConfig', {
152+
store.dispatch('setShowConfig', {
153153
key: 'showNewUserForm',
154154
value: true,
155155
})

‎apps/settings/tests/Controller/UsersControllerTest.php‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,11 @@
2323
use OCP\AppFramework\Http;
2424
use OCP\AppFramework\Services\IInitialState;
2525
use OCP\BackgroundJob\IJobList;
26+
use OCP\Config\IUserConfig;
2627
use OCP\Encryption\IEncryptionModule;
2728
use OCP\Encryption\IManager;
2829
use OCP\EventDispatcher\IEventDispatcher;
30+
use OCP\IAppConfig;
2931
use OCP\IConfig;
3032
use OCP\IGroupManager;
3133
use OCP\IL10N;
@@ -45,6 +47,8 @@ class UsersControllerTest extends \Test\TestCase {
4547
private UserManager&MockObject $userManager;
4648
private IUserSession&MockObject $userSession;
4749
private IConfig&MockObject $config;
50+
private IAppConfig&MockObject $appConfig;
51+
private IUserConfig&MockObject $userConfig;
4852
private IMailer&MockObject $mailer;
4953
private IFactory&MockObject $l10nFactory;
5054
private IAppManager&MockObject $appManager;
@@ -65,6 +69,8 @@ protected function setUp(): void {
6569
$this->groupManager = $this->createMock(Manager::class);
6670
$this->userSession = $this->createMock(IUserSession::class);
6771
$this->config = $this->createMock(IConfig::class);
72+
$this->appConfig = $this->createMock(IAppConfig::class);
73+
$this->userConfig = $this->createMock(IUserConfig::class);
6874
$this->l = $this->createMock(IL10N::class);
6975
$this->mailer = $this->createMock(IMailer::class);
7076
$this->l10nFactory = $this->createMock(IFactory::class);
@@ -106,6 +112,8 @@ protected function getController(bool $isAdmin = false, array $mockedMethods = [
106112
$this->groupManager,
107113
$this->userSession,
108114
$this->config,
115+
$this->appConfig,
116+
$this->userConfig,
109117
$this->l,
110118
$this->mailer,
111119
$this->l10nFactory,
@@ -128,6 +136,8 @@ protected function getController(bool $isAdmin = false, array $mockedMethods = [
128136
$this->groupManager,
129137
$this->userSession,
130138
$this->config,
139+
$this->appConfig,
140+
$this->userConfig,
131141
$this->l,
132142
$this->mailer,
133143
$this->l10nFactory,
@@ -992,4 +1002,55 @@ public static function dataTestCanAdminChangeUserPasswords(): array {
9921002
[false, false, false, true],
9931003
];
9941004
}
1005+
#[\PHPUnit\Framework\Attributes\DataProvider('dataSetPreference')]
1006+
public function testSetPreference(string $key, string $value, bool $isUserValue, bool $isAppValue, int $expectedStatus): void {
1007+
$controller = $this->getController(false, []);
1008+
$user = $this->createMock(IUser::class);
1009+
$user->method('getUID')->willReturn('testUser');
1010+
$this->userSession->method('getUser')->willReturn($user);
1011+
1012+
if ($isAppValue) {
1013+
if ($value === 'true' || $value === 'false' || $value === 'yes' || $value === 'no') {
1014+
$this->appConfig->expects($this->once())
1015+
->method('setValueBool')
1016+
->with('core', $key, $value === 'yes' || $value === 'true');
1017+
} else {
1018+
$this->appConfig->expects($this->once())
1019+
->method('setValueString')
1020+
->with('core', $key, $value);
1021+
}
1022+
$this->userConfig->expects($this->never())
1023+
->method('setValueBool');
1024+
} elseif ($isUserValue) {
1025+
$this->userConfig->expects($this->once())
1026+
->method('setValueBool')
1027+
->with('testUser', 'settings', $key, $value === 'true');
1028+
$this->appConfig->expects($this->never())
1029+
->method('setValueString');
1030+
$this->appConfig->expects($this->never())
1031+
->method('setValueBool');
1032+
} else {
1033+
$this->appConfig->expects($this->never())->method('setValueString');
1034+
$this->appConfig->expects($this->never())->method('setValueBool');
1035+
$this->userConfig->expects($this->never())->method('setValueString');
1036+
$this->userConfig->expects($this->never())->method('setValueBool');
1037+
}
1038+
1039+
$response = $controller->setPreference($key, $value);
1040+
$this->assertEquals($expectedStatus, $response->getStatus());
1041+
}
1042+
1043+
public static function dataSetPreference(): array {
1044+
return [
1045+
['newUser.sendEmail', 'yes', false, true, Http::STATUS_OK],
1046+
['group.sortBy', '1', false, true, Http::STATUS_OK],
1047+
['user_list_show_storage_path', 'true', true, false, Http::STATUS_OK],
1048+
['user_list_show_user_backend', 'false', true, false, Http::STATUS_OK],
1049+
['user_list_show_first_login', 'true', true, false, Http::STATUS_OK],
1050+
['user_list_show_last_login', 'true', true, false, Http::STATUS_OK],
1051+
['user_list_show_new_user_form', 'true', true, false, Http::STATUS_OK],
1052+
['user_list_show_languages', 'true', true, false, Http::STATUS_OK],
1053+
['invalidKey', 'value', false, false, Http::STATUS_FORBIDDEN],
1054+
];
1055+
}
9951056
}

‎build/psalm-baseline.xml‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2392,15 +2392,6 @@
23922392
</DeprecatedConstant>
23932393
<DeprecatedMethod>
23942394
<code><![CDATA[dispatch]]></code>
2395-
<code><![CDATA[getAppValue]]></code>
2396-
<code><![CDATA[getAppValue]]></code>
2397-
<code><![CDATA[getAppValue]]></code>
2398-
<code><![CDATA[getAppValue]]></code>
2399-
<code><![CDATA[getAppValue]]></code>
2400-
<code><![CDATA[getAppValue]]></code>
2401-
<code><![CDATA[getAppValue]]></code>
2402-
<code><![CDATA[getAppValue]]></code>
2403-
<code><![CDATA[setAppValue]]></code>
24042395
<code><![CDATA[validateMailAddress]]></code>
24052396
</DeprecatedMethod>
24062397
</file>
@@ -3290,7 +3281,6 @@
32903281
</DeprecatedClass>
32913282
<DeprecatedInterface>
32923283
<code><![CDATA[$object]]></code>
3293-
<code><![CDATA[Task]]></code>
32943284
<code><![CDATA[private]]></code>
32953285
</DeprecatedInterface>
32963286
</file>

‎cypress/e2e/settings/users_columns.cy.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,11 @@ describe('Settings: Show and hide columns', function() {
5959
getUserList().find('tbody tr').each(($row) => {
6060
cy.wrap($row).get('[data-cy-user-list-cell-language]').should('exist')
6161
})
62+
63+
// Clear local storage and reload to verify user settings DB persistence
64+
cy.clearLocalStorage()
65+
cy.reload()
66+
cy.get('[data-cy-user-list-header-languages]').should('exist')
6267
})
6368

6469
it('Can hide a column', function() {

‎dist/settings-users-3239.js‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)