Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add admin setting page with users defaults #1266

Merged
merged 16 commits into from Nov 16, 2022

Conversation

natoponen
Copy link
Contributor

Closing #1173 #1260 by adding admin's setting page for configuring user defaults, which will be applied on UserCreatedEvent or PostLoginEvent, if existing user doesn't have settings yet.

Not sure whether I should add author,copyright and license terms in the beggining of new files. Hope you can help me here :)

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general, but didn't test yet. Just wanted to quickly give the feedback after my vacation

lib/Listener/UserCreatedListener.php Show resolved Hide resolved
lib/Listener/PostLoginListener.php Show resolved Hide resolved
lib/Settings/Admin.php Show resolved Hide resolved
lib/Listener/PostLoginListener.php Outdated Show resolved Hide resolved
lib/Listener/UserCreatedListener.php Outdated Show resolved Hide resolved
lib/Settings/Admin.php Outdated Show resolved Hide resolved
lib/Settings/AdminSection.php Show resolved Hide resolved
lib/Settings/AdminSection.php Outdated Show resolved Hide resolved
src/adminSettings.js Outdated Show resolved Hide resolved
webpack.js Outdated Show resolved Hide resolved
@natoponen
Copy link
Contributor Author

Hello, @nickvergessen. I don't see any of those changes in recent releases. Is there something wrong with it?

lib/AppInfo/Application.php Outdated Show resolved Hide resolved
lib/Settings/Admin.php Show resolved Hide resolved
@nickvergessen
Copy link
Member

I don't see any of those changes in recent releases.

You don't see it because we didn't merge it.

Is there something wrong with it?

Didn't have time to check it yet, but a review is queued for this week.

natoponen and others added 7 commits November 16, 2022 10:19
Signed-off-by: Топонен Никита <Toponen@lanit.ru>
Signed-off-by: Топонен Никита <Toponen@lanit.ru>
Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: natoponen <57988162+natoponen@users.noreply.github.com>
Signed-off-by: Топонен Никита <Toponen@lanit.ru>
Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: natoponen <57988162+natoponen@users.noreply.github.com>
Signed-off-by: Топонен Никита <Toponen@lanit.ru>
Signed-off-by: Топонен Никита <Toponen@lanit.ru>
Signed-off-by: Топонен Никита <Toponen@lanit.ru>
Signed-off-by: Топонен Никита <Toponen@lanit.ru>
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coding styles seem off, but should be auto-fixable:

composer run cs:fix
npm run lint:fix

afterwards confirm with:

composer run cs:check
npm run lint

Other then that there are some minor comments but it works fine. If you can't or don't want to take care of the remaining points we can also do that.

form.append('batchSetting', this.config.setting_batchtime)
form.append('soundNotification', this.config.sound_notification ? 'yes' : 'no')
form.append('soundTalk', this.config.sound_talk ? 'yes' : 'no')
await axios.post(generateOcsUrl('apps/notifications/api/v2/settings/admin'), form)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding our own API call + endpoint here we could use

OCP.AppConfig.setValue()

But then it's not possible to have a sub admin permission for this, although I'm not convinced that would be necessary at all

src/views/AdminSettings.vue Outdated Show resolved Hide resolved
src/views/AdminSettings.vue Outdated Show resolved Hide resolved
lib/Settings/AdminSection.php Outdated Show resolved Hide resolved
lib/Settings/Admin.php Outdated Show resolved Hide resolved
lib/Listener/UserCreatedListener.php Outdated Show resolved Hide resolved
lib/Listener/PostLoginListener.php Outdated Show resolved Hide resolved
Signed-off-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
nickvergessen and others added 6 commits November 16, 2022 10:42
Signed-off-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the open comments

@nickvergessen
Copy link
Member

Thanks for all the hard work @natoponen and sorry for the delay on the review

@nickvergessen
Copy link
Member

/backport to stable25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants