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

Move admin_audit hooks to proper event listeners #37193

Closed
wants to merge 4 commits into from

Conversation

tcitworld
Copy link
Member

@tcitworld tcitworld commented Mar 13, 2023

Requires #35677

Summary

Moves all of the hooks used by admin_audit to event listeners, where appropriate typed events already exist.

  • Bases itself on Introduced app enable/disable/update typed events #35677 to have app management events
  • Adds a new ConsoleEventV2 typed event to replace the legacy ConsoleEvent event in OCP (already depreciated). However, this doesn't seem used anywhere else, so we could replace it straight away.
  • Adds preview details on the BeforePreviewFetchedEvent event

Part of #14552. Follow #37194 to track all of the hooks left.

TODO

  • Add tests for the new listeners

Checklist

@tcitworld tcitworld added this to the Nextcloud 27 milestone Mar 13, 2023
@tcitworld tcitworld requested review from ChristophWurst, juliushaertl, come-nc, a team, ArtificialOwl and blizzz and removed request for a team March 13, 2023 10:26
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@tcitworld tcitworld force-pushed the admin_audit/enh/move-to-event-listeners branch 2 times, most recently from 921aa53 to 82b9461 Compare March 13, 2023 10:54
@come-nc
Copy link
Contributor

come-nc commented Mar 13, 2023

See #32128 as well

@come-nc
Copy link
Contributor

come-nc commented Mar 13, 2023

Is that different/better than #32019 ?

@tcitworld
Copy link
Member Author

I missed this one, thanks. 🙈

Well, there's more stuff in mine. I'll simply take the LDAP stuff from Carl's PR, and take over #32018 as well.

@tcitworld tcitworld force-pushed the admin_audit/enh/move-to-event-listeners branch from 82b9461 to 3fb14cc Compare March 13, 2023 17:54
@tcitworld tcitworld force-pushed the admin_audit/enh/move-to-event-listeners branch 2 times, most recently from 01e64ac to b543f31 Compare March 13, 2023 18:14

$params = [
'itemType' => $share->getNodeType(),
'path' => $share->getNode()->getPath(),
Copy link
Member Author

@tcitworld tcitworld Mar 13, 2023

Choose a reason for hiding this comment

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

question: Unfortunately, this makes extra calls to get the Node. Should the share or the event hold this information?

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra PHP calls? Or extra database queries?
Does not seem that overkill to me.

apps/admin_audit/lib/AppInfo/Application.php Outdated Show resolved Hide resolved

$params = [
'itemType' => $share->getNodeType(),
'path' => $share->getNode()->getPath(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra PHP calls? Or extra database queries?
Does not seem that overkill to me.

lib/public/Console/ConsoleEventV2.php Outdated Show resolved Hide resolved
@come-nc
Copy link
Contributor

come-nc commented Mar 14, 2023

Please strict type the hell out of this, return types on all new methods and strict_type declare on all new files, otherwise nice cleanup!

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
…epreciated ConsoleEvent event

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld tcitworld force-pushed the admin_audit/enh/move-to-event-listeners branch from b543f31 to 52a289c Compare May 2, 2023 13:39
CarlSchwan and others added 2 commits May 2, 2023 15:54
Based on work from #32019

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@skjnldsv skjnldsv deleted the admin_audit/enh/move-to-event-listeners branch August 30, 2024 07:30
@come-nc
Copy link
Contributor

come-nc commented Sep 9, 2024

@tcitworld Can I take over this one?

@tcitworld
Copy link
Member Author

Yes please, I don't even want to have a look at the conflicts! 🙈

@tcitworld tcitworld restored the admin_audit/enh/move-to-event-listeners branch September 9, 2024 17:30
@tcitworld tcitworld reopened this Sep 9, 2024
@come-nc come-nc self-assigned this Sep 10, 2024
@come-nc
Copy link
Contributor

come-nc commented Sep 13, 2024

Supplanted by #47865

@come-nc come-nc closed this Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants