-
Notifications
You must be signed in to change notification settings - Fork 138
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
Fix Cron to Set Flights Visible to True Only When Owned By The Core #1770
base: dev
Are you sure you want to change the base?
Conversation
@BossOfGames I forgot what we decided about this |
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
<#1770 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAV7WHARBG3UJYJGWMRXXTDYWXZ67AVCNFSM6AAAAABC4GNF42VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZZGIYTIMJXGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want CI/CD to pass you should change this line:
'owner_type' => Flight::class, |
and set owner_type
to null
because in fact v7 sets it to null
and not Flight::class
when we create a Flight
@@ -43,10 +43,7 @@ public function checkFlights(): void | |||
if (!$flight->active) { | |||
continue; | |||
} | |||
|
|||
// Set to visible by default | |||
$flight->visible = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this should be removed ?
Since you have changed the query above, we are necessarily in the case where the flight belongs to v7, so why change the logic?
Didn't realize that code was there. I have corrected that, as well as fixed the removal of setting visible to true by default. |
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.