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)

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!

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

+ 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

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


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

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

 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-securityAI found potential problems Jul 6, 2023
View reviewed changes

Comment threadapps/admin_audit/lib/AppInfo/Application.php Fixed

github-advanced-securityAI found potential problems Jul 6, 2023
View reviewed changes

Comment threadapps/admin_audit/lib/Actions/Action.php

danialRahimy force-pushed the refactor-admin-audit-app branch from 50f672c to cd9b081Compare 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 teamJuly 13, 2023 20:09

danialRahimy force-pushed the refactor-admin-audit-app branch from cd9b081 to 67f0d31Compare 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

danialRahimyand 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>

danialRahimyand 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 5db849fCompare 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!

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!

AltahrimAug 17, 2023

Choose a reason for hiding this comment

Uh oh!

danialRahimyAug 17, 2023

Choose a reason for hiding this comment

Uh oh!

AltahrimAug 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-ncAug 28, 2023

Choose a reason for hiding this comment

Uh oh!

come-ncAug 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!

9 participants

danialRahimy commented Jul 6, 2023•
edited
Loading