Refactor admin audit app by danialRahimy · Pull Request #39192 · nextcloud/server

danialRahimy · 2023-07-06T09:13:45Z

Summary

Refactor admin audit app

TODO

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

apps/admin_audit/lib/AppInfo/Application.php

apps/admin_audit/lib/Actions/Action.php

danialRahimy · 2023-07-13T06:27:22Z

Hey @szaimen

I opened this PR last week, but haven't been assigned to any reviewers yet. Is there anything that I could do?

Thanks.

szaimen · 2023-07-13T20:10:15Z

Thanks for the ping! I requested some reviewers.

danialRahimy · 2023-08-16T05:19:18Z

Dear @szaimen

Would it be necessary for me to take any action in order to proceed with the pull request?

szaimen · 2023-08-16T10:38:11Z

@nextcloud/server-backend please review!

apps/admin_audit/lib/AuditLogger.php

Altahrim · 2023-08-17T06:45:06Z

apps/admin_audit/lib/Actions/Sharing.php

+			IShare::TYPE_REMOTE_GROUP => $this->sharedRemoteGroupType($params),
+			IShare::TYPE_DECK => $this->sharedDeckType($params),
+			IShare::TYPE_SCIENCEMESH => $this->sharedSciencemeshType($params),
+			default => null,


I wonder if this one should fail (or at least log) if an unknown type is used.

Before my modifications, if a type didn't match the conditions, no action was taken.
I'm uncertain whether to implement logging or throw a specialized exception to handle unknown types.

Same here, maybe it will ring a bell for someone :)

fsamapoor

In my opinion, removing redundant annotations make the code that much cleaner. When a method's parameter is typed, having the exact type hint as an annotation does not add any value to the mix. I think it would be great if you could also review the annotations in this PR.

I would also recommend writing a summary of the changes you have made in your PR's description and explaining briefly how those changes would improve the codebase.

come-nc

This is only moving things around, and is using match in a weird way.
I’m not sure I get the point.

come-nc · 2023-08-28T10:36:31Z

apps/admin_audit/lib/Actions/Sharing.php

-				]
-			);
-		}
+		match ($params['shareType']) {


The use of match when the return value is not used is really confusing.

come-nc · 2023-08-28T10:37:41Z

apps/admin_audit/lib/AppInfo/Application.php

 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IConfig;
 use OCP\IGroupManager;
+use OCP\IServerContainer;


Why? It does not seem used.

Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Co-authored-by: Benjamin Gaussorgues <github-fa3ie@altahrim.net> Signed-off-by: danial rahimy <48244647+danialRahimy@users.noreply.github.com>

…and modern syntax. Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

skjnldsv · 2024-08-16T09:10:38Z

Hey @danialRahimy thanks for the PR
Unfortunately there seem to be quite a bit of conflicts and some comments left unanswered above, so we closed that one. If you want to re-open and ork on it again, feel free to do so and ping me, we'll be happy to help you and get it move forward

If you used a tooling to create those, feel free to run it again now and ping me @skjnldsv straight away! I'll help you get it merged faster 🚀

Have a nice day :)

solracsf added 3. to review Waiting for reviews technical debt labels Jul 6, 2023

solracsf added this to the Nextcloud 28 milestone Jul 6, 2023

github-advanced-security bot found potential problems Jul 6, 2023

View reviewed changes

apps/admin_audit/lib/AppInfo/Application.php Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Jul 6, 2023

View reviewed changes

apps/admin_audit/lib/Actions/Action.php Show resolved Hide resolved

danialRahimy force-pushed the refactor-admin-audit-app branch from 50f672c to cd9b081 Compare July 7, 2023 06:03

shdehnavi mentioned this pull request Jul 13, 2023

Refactor admin_audit app #39234

Merged

szaimen requested review from a team, ArtificialOwl, blizzz and icewind1991 and removed request for a team July 13, 2023 20:09

danialRahimy force-pushed the refactor-admin-audit-app branch from cd9b081 to 67f0d31 Compare August 16, 2023 06:21

susnux requested review from Altahrim and nfebe August 16, 2023 23:39

Altahrim reviewed Aug 17, 2023

View reviewed changes

Altahrim approved these changes Aug 17, 2023

View reviewed changes

fsamapoor reviewed Aug 23, 2023

View reviewed changes

come-nc reviewed Aug 28, 2023

View reviewed changes

danialRahimy and others added 7 commits October 10, 2023 09:40

Revert condition to iterate early in the loop

7974ab4

Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Extract codes to separate methods to more readability

202906a

Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Replace conditions with match expression according to php8

e17c223

Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Declare methods return types

9a51a0d

Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Add properties type

b06913d

Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Run php-cs-fixer

47e4add

Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

Remove property type to fix psalm error

009983e

Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

danialRahimy and others added 2 commits October 10, 2023 09:40

Update apps/admin_audit/lib/AuditLogger.php

b01dd7c

Co-authored-by: Benjamin Gaussorgues <github-fa3ie@altahrim.net> Signed-off-by: danial rahimy <48244647+danialRahimy@users.noreply.github.com>

Updated array usage from array() to [] for improved code readability …

5db849f

…and modern syntax. Signed-off-by: Danial Rahimi <48244647+danialRahimy@users.noreply.github.com>

danialRahimy force-pushed the refactor-admin-audit-app branch from f250821 to 5db849f Compare October 10, 2023 06:10

skjnldsv mentioned this pull request Nov 1, 2023

chore: 28.0.0 beta 1 #41228

Merged

This was referenced Nov 6, 2023

28.0.0 beta 2 #41314

Merged

28.0.0 beta 3 #41379

Merged

This was referenced Nov 14, 2023

28.0.0 beta 4 #41477

Merged

28.0.0 RC1 #41621

Merged

blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023

This was referenced Mar 12, 2024

29.0.0 beta 2 #44152

Merged

29.0.0 beta 3 #44186

Merged

29.0.0 beta 4 #44275

Merged

This was referenced Mar 20, 2024

29.0.0 beta 5 #44361

Merged

29.0.0 beta 6 #44449

Merged

skjnldsv mentioned this pull request Mar 28, 2024

29.0.0 RC1 #44552

Merged

81 tasks

skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024

skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024

This was referenced Jul 30, 2024

30.0.0 beta 2 #46870

Merged

30.0.0 beta 3 #46946

Merged

This was referenced Aug 5, 2024

30.0.0 beta 4 #47028

Merged

30.0.0 beta 5 #47100

Merged

skjnldsv mentioned this pull request Aug 13, 2024

30.0.0 RC1 #47190

Merged

skjnldsv closed this Aug 14, 2024

skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024

Uh oh!

Conversation

danialRahimy commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

TODO

Checklist

Uh oh!

Uh oh!

Uh oh!

danialRahimy commented Jul 13, 2023

Uh oh!

szaimen commented Jul 13, 2023

Uh oh!

danialRahimy commented Aug 16, 2023

Uh oh!

szaimen commented Aug 16, 2023

Uh oh!

Uh oh!

Altahrim Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

danialRahimy Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

Altahrim Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

fsamapoor left a comment

Choose a reason for hiding this comment

Uh oh!

come-nc left a comment

Choose a reason for hiding this comment

Uh oh!

come-nc Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

come-nc Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Aug 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

danialRahimy commented Jul 6, 2023 •

edited

Loading