phpvms icon indicating copy to clipboard operation
phpvms copied to clipboard

Fix Cron to Set Flights Visible to True Only When Owned By The Core

Open BossOfGames opened this issue 1 year ago • 8 comments

This PR resolves an issue where the nightly cron sets the visibility of flights created and owned by modules. This functionality is not exactly desired, as module developers would need to setup a cron event to set the flights back to their desired settings.

BossOfGames avatar Feb 06 '24 15:02 BossOfGames

@BossOfGames I forgot what we decided about this

nabeelio avatar Mar 05 '24 16:03 nabeelio

I forgot too, to be quite honest. I made a modification to my repo on this a while ago for my VA. Maybe a behavior like this could be enabled/disabled via config/setting/env file?

On Wed, Mar 6, 2024 at 1:53 AM Nabeel S. @.***> wrote:

@BossOfGames https://github.com/BossOfGames I forgot what we decided about this

— Reply to this email directly, view it on GitHub https://github.com/nabeelio/phpvms/pull/1770#issuecomment-1979214175, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAV7WHARBG3UJYJGWMRXXTDYWXZ67AVCNFSM6AAAAABC4GNF42VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZZGIYTIMJXGU . You are receiving this because you were mentioned.Message ID: @.***>

BossOfGames avatar Mar 07 '24 03:03 BossOfGames

Why not

$flights = Flight::where('active', 1)->whereNull('owner_type')->get();

This will eliminate already inactive flights and the ones owned by a 3rd party module in the beginning.

Current implementation will still handle the visibility of 3rd party flights even this PR gets merged if there are days provided for the "owned" flight.

This will allow module developers to handle their flights separately and improve resource usage by skipping inactive flights at query level.

If a setting or config variable is required, we can simply change the query instead of function coding. Also this must be documented properly so addon developers can check it before applying/implementing their own scripts.

FatihKoz avatar Jul 15 '24 20:07 FatihKoz

Why not

$flights = Flight::where('active', 1)->whereNull('owner_type')->get();

This will eliminate already inactive flights and the ones owned by a 3rd party module in the beginning.

Current implementation will still handle the visibility of 3rd party flights even this PR gets merged if there are days provided for the "owned" flight.

This will allow module developers to handle their flights separately and improve resource usage by skipping inactive flights at query level.

If a setting or config variable is required, we can simply change the query instead of function coding. Also this must be documented properly so addon developers can check it before applying/implementing their own scripts.

I agree with this better. I have updated the PR to reflect this suggestion.

BossOfGames avatar Jul 23 '24 04:07 BossOfGames

If you want CI/CD to pass you should change this line:

https://github.com/nabeelio/phpvms/blob/53ab19a47d6c5a48635f6bff412d94e1e4bcfd0f/app/Database/factories/FlightFactory.php#L58

and set owner_type to null because in fact v7 sets it to null and not Flight::class when we create a Flight

Didn't realize that code was there. I have corrected that, as well as fixed the removal of setting visible to true by default.

BossOfGames avatar Jul 29 '24 01:07 BossOfGames