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

Checklist API performance on default tenants and email invitations #4526

Merged
merged 3 commits into from
Feb 16, 2022

Conversation

mike12345567
Copy link
Collaborator

@mike12345567 mike12345567 commented Feb 16, 2022

Description

There was an issue with the checklist call taking upwards of two seconds to respond. This is a relatively simple call, it shouldn't have any heavy database calls.

This also fixes #4522 - there was some issues introduced by the frontend-core that needed fixed for this flow to work correctly.

Can see the call here:
image

It appeared to be tied to the default tenant, as there was an edge case which meant that the default tenant couldn't use the more efficient app tenant lookup, instead it has to lookup the full list of app IDs. I added an efficient flag to the getAllApps function so that we can state that we want it to be as fast as possible, perhaps sacrificing some accuracy in the default tenant. This is fine for the checklist call because it only needs a list of apps in the tenant. This should speed up the login process for the default tenant fairly significantly.

@mike12345567 mike12345567 self-assigned this Feb 16, 2022
Copy link
Member

@shogunpurple shogunpurple 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! Good catch

@codecov-commenter
Copy link

Codecov Report

Merging #4526 (7bac5d5) into master (3aac333) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4526   +/-   ##
=======================================
  Coverage   67.92%   67.92%           
=======================================
  Files         144      144           
  Lines        4923     4923           
  Branches      761      761           
=======================================
  Hits         3344     3344           
  Misses       1105     1105           
  Partials      474      474           
Impacted Files Coverage Δ
...s/server/src/api/controllers/row/internalSearch.js 5.26% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8da7b9...7bac5d5. Read the comment docs.

Copy link
Member

@aptkingston aptkingston left a comment

Choose a reason for hiding this comment

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

Knew there would be at least one issue with all the renaming in frontend core! I'm glad it's taken us this long to find one though haha.

packages/builder/src/stores/portal/users.js Show resolved Hide resolved
packages/builder/src/stores/portal/users.js Show resolved Hide resolved
@mike12345567 mike12345567 changed the title Checklist API performance on default tenants Checklist API performance on default tenants and email invitations Feb 16, 2022
@mike12345567 mike12345567 merged commit 3447366 into master Feb 16, 2022
@mike12345567 mike12345567 deleted the fix/checklist-perf branch February 16, 2022 13:44
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants