Skip to content

DPMMA-3039 Company lookup limit #14461

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 5, 2025
Merged

DPMMA-3039 Company lookup limit #14461

merged 3 commits into from
Mar 5, 2025

Conversation

patrykgruszka
Copy link
Member

@patrykgruszka patrykgruszka commented Jan 15, 2025 •
edited
Loading

Q A
Bug fix? (use the a.b branch) 🟢
New feature/enhancement? (use the a.x branch) 🔴
Deprecations? 🔴
BC breaks? (use the c.x branch) 🔴
Automated tests included? 🟢
Related user documentation PR URL mautic/user-documentation#...
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #14452

Description

This PR fixes the issue of slow loading and browser hangs in the contact edit view when dealing with a large number of companies in Mautic 5.2. Changes include the same limit for initially loaded companies as it was before 5.2 without reverting the changes made in #13578

Steps to Test:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Navigate to the contact edit view with a lot of companies in the system.
  3. Verify that the page loads quickly and only shows up to 100 companies initially.
  4. Try searching for a company
  5. Verify the steps to test from related PR: Cannot merge company with itself #13578

@patrykgruszka
fix: [DPMMA-3039] Company lookup limit
4b6b18d 
fix: [DPMMA-3039] change default limit and fix phpstan

fix: [DPMMA-3039] update company list limit

fix: [DPMMA-3039] refactor EntityLookupChoiceLoader::fetchChoices

fix: [DPMMA-3039] test testMax100CompaniesShouldBeFetchedOnContactEditAction
@patrykgruszka patrykgruszka marked this pull request as draft January 15, 2025 14:56
@patrykgruszka patrykgruszka added regression A bug that broke something in the last release performance-scalability Anything related to performance and scalability labels Jan 15, 2025
@patrykgruszka patrykgruszka marked this pull request as ready for review January 16, 2025 09:33
@kuzmany
Copy link
Member

kuzmany commented Jan 16, 2025

I suppose we use similar PR #13869, probably you can take look

AlanWierzchonCA
AlanWierzchonCA reviewed Jan 31, 2025
View reviewed changes
@patrykgruszka
Copy link
Member Author

@kuzmany yes, it changes similar methods but fixes a different problem. I can review yours when it's ready.

@codecov Codecov
Copy link

codecov bot commented Jan 31, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.48%. Comparing base (4035b79) to head (38ea32f).
Report is 128 commits behind head on 5.2.

Files with missing lines Patch % Lines
...dle/Form/ChoiceLoader/EntityLookupChoiceLoader.php 42.85% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.2   #14461      +/-   ##
============================================
+ Coverage     63.45%   63.48%   +0.03%     
- Complexity    34636    34638       +2     
============================================
  Files          2273     2273              
  Lines        103603   103615      +12     
============================================
+ Hits          65741    65780      +39     
+ Misses        37862    37835      -27
Files with missing lines Coverage Δ
...pp/bundles/LeadBundle/Entity/CompanyRepository.php 61.50% <100.00%> (+0.48%) ⬆️
...p/bundles/LeadBundle/Form/Type/CompanyListType.php 100.00% <ø> (ø)
app/bundles/LeadBundle/Model/CompanyModel.php 70.54% <100.00%> (+0.07%) ⬆️
...dle/Form/ChoiceLoader/EntityLookupChoiceLoader.php 86.13% <42.85%> (+2.64%) ⬆️

... and 9 files with indirect coverage changes

@patrykgruszka patrykgruszka added pending-test-confirmation PR's that require one test before they can be merged code-review-passed PRs which have passed code review labels Jan 31, 2025
@nick-vanpraet nick-vanpraet added this to the 5.2.3 milestone Feb 19, 2025
matbcvo
matbcvo approved these changes Feb 22, 2025
View reviewed changes
Copy link
Contributor

@matbcvo matbcvo left a comment

Choose a reason for hiding this comment

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

The code changes look good to me. I have tested the PR, and it works as expected.

document.querySelectorAll('.active-result').length
100

@matbcvo matbcvo added user-testing-passed PRs which have been successfully tested by the required number of people. ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Feb 22, 2025
@nick-vanpraet nick-vanpraet modified the milestones: 5.2.3, 5.2.4 Feb 24, 2025
@escopecz escopecz merged commit 79b2a3e into mautic:5.2 Mar 5, 2025
17 checks passed
@github-project-automation github-project-automation bot moved this from 🎉 Ready to commit to 🥳 Done in Open Source Fridays Mar 5, 2025
@patrykgruszka patrykgruszka mentioned this pull request Apr 28, 2025
Closed
1 task
@mallezie mallezie mentioned this pull request Jun 4, 2025
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matbcvo matbcvo matbcvo approved these changes

+1 more reviewer

@AlanWierzchonCA AlanWierzchonCA AlanWierzchonCA approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
code-review-passed PRs which have passed code review performance-scalability Anything related to performance and scalability ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged regression A bug that broke something in the last release user-testing-passed PRs which have been successfully tested by the required number of people.
Projects
Open Source Fridays
Status: 🥳 Done
Milestone
5.2.4
Development

Successfully merging this pull request may close these issues.
6 participants
@patrykgruszka @kuzmany @matbcvo @AlanWierzchonCA @escopecz @nick-vanpraet