Skip to content

feat: add user trash restore#4

Merged
SlimDeluxe merged 10 commits intoDataLinx:mainfrom
sparkybug:add-user-trash-restore
Mar 21, 2025
Merged

feat: add user trash restore#4
SlimDeluxe merged 10 commits intoDataLinx:mainfrom
sparkybug:add-user-trash-restore

Conversation

@sparkybug
Copy link
Contributor

/claim #1

Copy link
Member

@SlimDeluxe SlimDeluxe left a comment

Choose a reason for hiding this comment

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

I see you are not checking permissions, but only the logged in state.
Please see here how to update permissions.
Also note that you can set up a super admin by calling this method in your test:
$this->set_up_super_admin_and_tenant();
This "logs in" a super admin with all permissions.

@SlimDeluxe
Copy link
Member

@sparkybug , I see you added permissions checks, but they are custom and not using Filament Shield.

As I've mentioned before, you need to follow this procedure to create new permissions and update the policy.
So:

  1. Add the prefixes in UserResource: restore. restore_any, force_delete, force_delete_any
  2. Run php artisan shield:generate --panel=admin --resource=UserResource in the main app to update the permissions in the database and create a new policy class
  3. Copy the new generated methods to UserPolicy
  4. Update tests to use the correct permissions

@sparkybug
Copy link
Contributor Author

@sparkybug , I see you added permissions checks, but they are custom and not using Filament Shield.

As I've mentioned before, you need to follow this procedure to create new permissions and update the policy. So:

  1. Add the prefixes in UserResource: restore. restore_any, force_delete, force_delete_any
  2. Run php artisan shield:generate --panel=admin --resource=UserResource in the main app to update the permissions in the database and create a new policy class
  3. Copy the new generated methods to UserPolicy
  4. Update tests to use the correct permissions

Hello @SlimDeluxe , I have done as requested. Sorry I was misunderstanding you all these while.
I have also added some more tests to ensure an all round check.
If you think they're not needed though, I could remove them.

@SlimDeluxe
Copy link
Member

Thank you, I've taken a quick look and unfortunately it appears it's not quite ready. I'll do the review tomorrow.

Copy link
Member

@SlimDeluxe SlimDeluxe left a comment

Choose a reason for hiding this comment

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

Sorry, some more fixes are required.

@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.26%. Comparing base (712beac) to head (66c6de6).

Additional details and impacted files
@@            Coverage Diff            @@
##               main       #4   +/-   ##
=========================================
  Coverage     59.26%   59.26%           
  Complexity      111      111           
=========================================
  Files            28       28           
  Lines           734      734           
=========================================
  Hits            435      435           
  Misses          299      299           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sparkybug
Copy link
Contributor Author

Hello @SlimDeluxe , I have made all fixes as you pointed out.
kindly review.

@SlimDeluxe SlimDeluxe merged commit dc021e4 into DataLinx:main Mar 21, 2025
1 check passed
@SlimDeluxe
Copy link
Member

Hi @sparkybug, thank you, this is now merged :)

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.

3 participants