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) Allow internal options for internals in non-internal group #4370

Merged

Conversation

Obi-Wana
Copy link
Contributor

@Obi-Wana Obi-Wana commented Dec 8, 2024

Internals that are not member of the internal group (e.g., Editor) and not Moderator+ should still be able to set the internal options on their uploads. They need to be in at least one Internal group, checked using count of InternalUsers.

Copy link

what-the-diff bot commented Dec 8, 2024

PR Summary

  • Improved User Permission Verification
    Updated the method in which user permissions are checked internally for enhanced security.

  • Enhanced Torrent Attribute Setting Mechanism
    Revamped the way how torrent attributes like internal, featured, doubleup, refundable, and free are set, linking it more closely to user permissions.

  • Updated User Interface for Input Fields
    Tweaked the visibility rules for input fields on the create.blade.php. This change will make the interface more intuitive and in sync with backend logic.

Copy link
Collaborator

@Roardom Roardom left a comment

Choose a reason for hiding this comment

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

Great idea!

Can we use $user->internals()->exists() instead to make it more efficient?

Further, I think we can even drop the is_internal column on the groups table and use this relation as the source of truth instead for all cases the is_internal flag is checked. What do you think @HDVinnie?

@Obi-Wana
Copy link
Contributor Author

Obi-Wana commented Dec 8, 2024

Can we use $user->internals()->exists() instead to make it more efficient?

No, ->exists() does not exist on this, just checked.

Further, I think we can even drop the is_internal column on the groups table and use this relation as the source of truth instead for all cases the is_internal flag is checked. What do you think @HDVinnie?

I'll let this up to you, and think it should be going into its own PR.

@Obi-Wana Obi-Wana requested a review from Roardom December 8, 2024 22:01
@HDVinnie
Copy link
Collaborator

HDVinnie commented Dec 8, 2024

LGTM. Just swap the count for exist.

@Obi-Wana Obi-Wana force-pushed the internal-perks-based-on-membership branch from db660a3 to 37ec70c Compare December 9, 2024 09:46
@HDVinnie HDVinnie removed the request for review from Roardom December 9, 2024 20:44
Copy link
Collaborator

@Roardom Roardom left a comment

Choose a reason for hiding this comment

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

When using loadExists, you need to specify the relation to load, i.e. ->loadExists('internals'). Then, instead of checking for if $user->internals()->exists(), you only have to check the $user->internals_exists property that's added to the user without needing further database calls.

@Obi-Wana
Copy link
Contributor Author

When using loadExists, you need to specify the relation to load, i.e. ->loadExists('internals'). Then, instead of checking for if $user->internals()->exists(), you only have to check the $user->internals_exists property that's added to the user without needing further database calls.

Added this to the APIController, but Larastan doesn't seem to like it...

@Roardom
Copy link
Collaborator

Roardom commented Dec 21, 2024

I checked through the larastan repo and you're right: unfortunately larastan doesn't yet support relation_exists (although it does support relation_count).

Try adding /** @phpstan-ignore property.notFound (Larastan doesn't yet support loadExists()) */ wherever the property is accessed.
Relevant documentation can be found here: https://phpstan.org/user-guide/ignoring-errors

@Obi-Wana Obi-Wana force-pushed the internal-perks-based-on-membership branch from 16913aa to 2f24c6a Compare December 22, 2024 11:56
@Obi-Wana Obi-Wana requested a review from Roardom December 22, 2024 12:02
@HDVinnie HDVinnie requested review from Roardom and removed request for Roardom December 30, 2024 04:13
@Obi-Wana Obi-Wana force-pushed the internal-perks-based-on-membership branch 2 times, most recently from 3513b49 to f3bb4f1 Compare January 2, 2025 13:43
@Obi-Wana Obi-Wana requested a review from Roardom January 2, 2025 13:47
Copy link
Collaborator

@Roardom Roardom left a comment

Choose a reason for hiding this comment

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

Can you do a ctrl+f in your IDE for ->group->is_internal and replace all the instances with the ->internals()->exists() logic? (And use eager loading if the query is used multiple times in a single http request).

Thanks!

@Obi-Wana Obi-Wana force-pushed the internal-perks-based-on-membership branch 2 times, most recently from 5cf51c2 to a9c2494 Compare February 1, 2025 18:01
@Obi-Wana Obi-Wana requested a review from Roardom February 1, 2025 18:15
@Obi-Wana Obi-Wana force-pushed the internal-perks-based-on-membership branch 2 times, most recently from 0adb474 to eaee5c6 Compare February 20, 2025 16:08
@Obi-Wana Obi-Wana changed the title Allow internal options for internals in non-internal group (Add) Allow internal options for internals in non-internal group Feb 21, 2025
Internals that are not member of the internal group (e.g. Editor) but not Moderator+ should still be able to set the internal options on their uploads.
Also, replace all group->is_internal with internals->exists in app & views.
@Obi-Wana Obi-Wana force-pushed the internal-perks-based-on-membership branch from eaee5c6 to ba1818b Compare February 27, 2025 13:34
@HDVinnie HDVinnie merged commit 3f9b53f into HDInnovations:8.x.x Feb 27, 2025
@Roardom
Copy link
Collaborator

Roardom commented Feb 28, 2025

Thank you!

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

Successfully merging this pull request may close these issues.

3 participants