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

Fixed #7824 #10494

Merged
merged 2 commits into from Jan 10, 2022
Merged

Fixed #7824 #10494

merged 2 commits into from Jan 10, 2022

Conversation

exula
Copy link
Contributor

@exula exula commented Jan 10, 2022

Previously there was a 999 max seats on Licenses as anything above that seemed to cause slowdowns and failure.

This commit allievates those pain points

  • removed freeSeats as a hydrated Eloquent model on JSON requests for the licenses index
  • removed 'licenseSeats.user', 'licenseSeats.asset' from the 'with' clause as it's not needed in the view (Datatabales takes care of that)
  • removed the 999 max seats limit from the License Model,
  • reworked how new license seats are created when increasing seats or creating licenses
  • Added an index to the license_seats table to help speed up lookups

I test created about 1000 licenses with 20,000 seats each and things were zooming alone nicely.

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context, providing screenshots where practical. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • [ * ] Bug fix (non-breaking change which fixes an issue)
  • [ * ] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Test Configuration:

  • PHP version:
  • MySQL version
  • Webserver version
  • OS version

Checklist:

Previously there was a 999 max seats on Licenses as anything above that seemed to cause slowdowns and failure.

This commit allievates those pain points

- removed freeSeats as a hydrated Eloquent model on JSON requests for the licenses index
- removed 'licenseSeats.user', 'licenseSeats.asset' from the 'with' clause as it's not needed in the view (Datatabales takes care of that)
- removed the 999 max seats limit from the License Model,
- reworked how new license seats are created when increasing seats or creating licenses
- Added an index the license_seats table to help speed up lookups
@exula
Copy link
Contributor Author

exula commented Jan 10, 2022

Please take a good look at the Licenses at the changes around line 180 on License.php
The old way died when putting in LOTS of license seats. The new way just inserts the new license seats in chunks.

That's the only chunk of code that I'm iffy on it being up to your standards.

Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

This looks AWESOME. I do have a few questions which I'd love for you to address, then I'll definitely do the work of getting this integrated as quickly as I can.

I always had the suspicion that if I looked at Licenses hard enough, that eventually whatever was mucking us up would dissipate. I think you actually did that work, so thank you!

@@ -10,6 +10,7 @@
use Illuminate\Database\Eloquent\SoftDeletes;
use Illuminate\Support\Facades\Auth;
use Illuminate\Support\Facades\Session;
use phpDocumentor\Reflection\Types\Collection;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This smells funny, are we sure this is right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that right after I pushed. PHPStorm added that and I didn't notice it.

👃 bad smell lol

app/Models/License.php Show resolved Hide resolved
app/Models/License.php Show resolved Hide resolved
app/Models/License.php Show resolved Hide resolved
database/factories/LicenseFactory.php Show resolved Hide resolved
public function up()
{
Schema::table('license_seats', function (Blueprint $table) {
$table->index(['license_id']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

BOOM! Nice :)

Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

This looks GREAT. Let me chat with the boss and see if we can get this tested, then merged.

Thank you!

@snipe snipe merged commit ea83567 into snipe:develop Jan 10, 2022
snipe added a commit that referenced this pull request Apr 12, 2022
Signed-off-by: snipe <snipe@snipe.net>
snipe added a commit that referenced this pull request Apr 12, 2022
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.

None yet

3 participants