[IMP] web, onboarding: avoid unneccessary rpc request #143212
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
fdardenne
force-pushed
the
master-onboarding-optimization-dafl
branch
16 times, most recently
from
bd7fd74
to
c6fdd5f
Compare
C3POdoo
requested review from
a team,
caburj and
Julien00859
and removed request for
a team
flch-odoo
reviewed
Dec 8, 2023
There was a problem hiding this comment.
Thanks for the changes @fdardenne ;
I wonder if we shouldn't also have a time limit to this cache to keep the perf improvement but also allow more frequent refreshes as other admins could interact with the steps as well and we would "never" see it.
Maybe a few hours?
fdardenne
force-pushed
the
master-onboarding-optimization-dafl
branch
2 times, most recently
from
b1202a0
to
fcafded
Compare
my 2cents here: whilst you're technically correct that this could lead to desynchronization, i don't think this is worth the hassle for the following reasons:
As long as it's ultimately consistent, i think it's fine. no need to make the solution more complex for this. |
fdardenne
force-pushed
the
master-onboarding-optimization-dafl
branch
7 times, most recently
from
f9aa270
to
95e4304
Compare
fdardenne
force-pushed
the
master-onboarding-optimization-dafl
branch
from
95e4304
to
04f59dd
Compare
fdardenne
commented
Jan 10, 2024
fdardenne
force-pushed
the
master-onboarding-optimization-dafl
branch
6 times, most recently
from
39c7e76
to
2f6bb6e
Compare
kebeclibre
approved these changes
Jan 11, 2024
aab-odoo
approved these changes
Jan 11, 2024
The OnboardingBanner is a component that allows having banner in
views. For example you can find banners in Accounting->Invoices,
Calendar, ...
To do that, the OnboardingBanner component fetch from the server the html
to display in the banner.
However we would like to avoid the following unnecessary requests:
- When the banner is closed
- When the user do not have the right to display the banner (not in the
`base.group_system group`)
This commit avoid thoses unnecessary requests for the `/onboarding/*`
routes by storing the displayable banner in the session_info.
This optimization is made to the generic `/onboarding` route because
this route is linked to the `onboarding.onboarding` model and each
record specify if the steps are done and if the banner is manually
closed. Additionally, we know that this route is only accessible for
user with `base.group_system` group.
task-id: 3561730
fdardenne
force-pushed
the
master-onboarding-optimization-dafl
branch
from
2f6bb6e
to
dd4fff8
Compare
robodoo
pushed a commit
that referenced
this pull request
Jan 11, 2024
The OnboardingBanner is a component that allows having banner in
views. For example you can find banners in Accounting->Invoices,
Calendar, ...
To do that, the OnboardingBanner component fetch from the server the html
to display in the banner.
However we would like to avoid the following unnecessary requests:
- When the banner is closed
- When the user do not have the right to display the banner (not in the
`base.group_system group`)
This commit avoid thoses unnecessary requests for the `/onboarding/*`
routes by storing the displayable banner in the session_info.
This optimization is made to the generic `/onboarding` route because
this route is linked to the `onboarding.onboarding` model and each
record specify if the steps are done and if the banner is manually
closed. Additionally, we know that this route is only accessible for
user with `base.group_system` group.
closes #143212
Task-id: 3561730
Signed-off-by: Aaron Bohy (aab) <aab@odoo.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The OnboardingBanner is a component that allows having banner in
views. For example you can find banners in Accounting->Invoices,
Calendar, ...
[IMP] onboarding: use session_info to avoid unnecessary requests
The OnboardingBanner component fetch from the server the html
to display in the banner.
However we would like to avoid the following unnecessary requests:
base.group_systemgroup)This commit avoid thoses unnecessary requests for the
/onboarding/*routes by storing the displayable banner in the session_info.
This optimization is made to the generic
/onboardingroute becausethis route is linked to the
onboarding.onboardingmodel and eachrecord specify if the steps are done and if the banner is manually
closed. Additionally, we know that this route is only accessible for
user with
base.group_systemgroup.On a side note, in #141110 we tried to make OnboardingBanner rpc fetching asynchronous. Allowing views to render without the need of waiting the rpc response of the banner.
Unfortunately it caused a flickering, i.e. the banner was rendered after the view leading to the repositioning of user interface elements. For the moment it is not possible to show a skeleton to avoid such flickering because the client does not know the height of the banner without the rpc answer.
task-id: 3561730