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

Optimization method FormsService::canSubmit #2225

Merged

Conversation

AIlkiv
Copy link
Collaborator

@AIlkiv AIlkiv commented Jun 27, 2024

Currently, to check if a user has filled out the form, all entities are retrieved, and then two loops are performed at the PHP. My version does the same thing but with a single simple query.

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 52.63158% with 9 lines in your changes missing coverage. Please review.

Project coverage is 46.03%. Comparing base (f6c1b8a) to head (a0b26df).
Report is 394 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2225      +/-   ##
============================================
+ Coverage     45.95%   46.03%   +0.08%     
- Complexity      797      800       +3     
============================================
  Files            65       66       +1     
  Lines          3066     3065       -1     
============================================
+ Hits           1409     1411       +2     
+ Misses         1657     1654       -3     

Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

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

Nice one :) Just a little comment about the new name for the function :)

@susnux what do you think?

And it would be nice if you could add some more tests to satisfy the Codecov checks 😉

lib/Db/SubmissionMapper.php Outdated Show resolved Hide resolved
@Chartman123 Chartman123 added 3. to review Waiting for reviews performances Performances issues and optimisations labels Jun 27, 2024
@Chartman123 Chartman123 added this to the 4.3 milestone Jun 27, 2024
@AIlkiv AIlkiv force-pushed the optimization-formsService-canSubmit branch 2 times, most recently from 9c44b3c to b3d13cc Compare June 27, 2024 17:22
@AIlkiv AIlkiv requested a review from Chartman123 June 28, 2024 09:09
@Chartman123 Chartman123 requested a review from susnux July 2, 2024 12:31
Copy link
Collaborator

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Generally nice! Some notes :)

(and we should add a composite index on "formId userId"

lib/Db/SubmissionMapper.php Outdated Show resolved Hide resolved
lib/Db/SubmissionMapper.php Outdated Show resolved Hide resolved
lib/Db/SubmissionMapper.php Show resolved Hide resolved
@AIlkiv AIlkiv force-pushed the optimization-formsService-canSubmit branch 3 times, most recently from 7d667ff to 548075a Compare July 9, 2024 08:39
@AIlkiv AIlkiv force-pushed the optimization-formsService-canSubmit branch from 548075a to d252a41 Compare July 9, 2024 08:53
@AIlkiv AIlkiv requested review from susnux and Chartman123 July 9, 2024 13:43
@Chartman123
Copy link
Collaborator

@AIlkiv could you please rebase on current main?

Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

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

Some small comments, but rest seems ok to me :)

lib/Controller/ApiController.php Outdated Show resolved Hide resolved
lib/Db/SubmissionMapper.php Outdated Show resolved Hide resolved
lib/Migration/Version040300Date20240708163401.php Outdated Show resolved Hide resolved
lib/Service/FormsService.php Show resolved Hide resolved
lib/Service/FormsService.php Outdated Show resolved Hide resolved
@AIlkiv AIlkiv force-pushed the optimization-formsService-canSubmit branch 2 times, most recently from 2421d7c to 0aa2748 Compare July 10, 2024 05:22
@AIlkiv AIlkiv requested a review from Chartman123 July 10, 2024 05:27
@AIlkiv AIlkiv force-pushed the optimization-formsService-canSubmit branch from 0aa2748 to c1e789a Compare July 10, 2024 06:36
@AIlkiv AIlkiv force-pushed the optimization-formsService-canSubmit branch from c1e789a to a0b26df Compare July 10, 2024 06:52
@Chartman123 Chartman123 merged commit 96e6790 into nextcloud:main Jul 10, 2024
51 checks passed
Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews feedback-requested performances Performances issues and optimisations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants