Skip to content

Scope the appdata theming storage for global and users #34599

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
PVince81 merged 5 commits into from
Oct 14, 2022
Merged

Scope the appdata theming storage for global and users #34599

PVince81 merged 5 commits into from
Oct 14, 2022

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Oct 14, 2022

Fix #34596

@skjnldsv skjnldsv added this to the Nextcloud 26 milestone Oct 14, 2022
@skjnldsv skjnldsv self-assigned this Oct 14, 2022
@skjnldsv skjnldsv force-pushed the fix/background-appdata-scrope branch from 2aca8e0 to 11173a9 Compare October 14, 2022 09:20
This was referenced Oct 14, 2022
Closed
Closed
@PVince81
Copy link
Member

PVince81 commented Oct 14, 2022 •
edited
Loading

  • add hash to the "v=" argument based on user id to avoid seeing another user's background
  • still need shift+refresh for seeing the updated background because the browser is using the old version:

here we should see "v=2" but we see "v=1":
image

this is likely because "apps.scss" is also a cached asset, we need to add a cache buster there as well
it might have worked before when busting the global theme cache as it would force a reload of those files, but busting the global theme is a no-go as it has side effects

PVince81
PVince81 approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 for @skjnldsv's part, please also review what I pushed

@PVince81
Copy link
Member

PVince81 commented Oct 14, 2022

I've pushed a fix that adds cache busting also for the CSS variables to make sure we load the correct background in apps.
Additionally I've hashed the user id there to make sure we don't see the cached background of another user.
This fixes #34599 (comment)

szaimen
szaimen reviewed Oct 14, 2022
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 14, 2022
View reviewed changes
apps/theming/lib/Service/ThemeInjectionService.php
$this->defaultTheme = $defaultTheme;
$this->config = $config;
if ($userSession->getUser() !== null) {
$this->userId = $userSession->getUser()->getUID();

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getUID on possibly null value
@skjnldsv skjnldsv requested review from a team, ArtificialOwl, blizzz, juliusknorr and szaimen and removed request for a team October 14, 2022 11:44
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 14, 2022
@szaimen

This comment was marked as resolved.

Sign in to view
@skjnldsv

This comment was marked as resolved.

Sign in to view
szaimen
szaimen requested changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment •
edited by skjnldsv
Loading

Choose a reason for hiding this comment

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

See

2022-10-14-134652.mp4

@szaimen

This comment was marked as off-topic.

Sign in to view
@szaimen

This comment was marked as off-topic.

Sign in to view
@PVince81

This comment was marked as resolved.

Sign in to view
@PVince81

This comment was marked as resolved.

Sign in to view
@skjnldsv

This comment was marked as resolved.

Sign in to view
@skjnldsv skjnldsv force-pushed the fix/background-appdata-scrope branch from 8a5b526 to acc7a2c Compare October 14, 2022 12:14
@skjnldsv skjnldsv requested a review from szaimen October 14, 2022 12:14
szaimen
szaimen approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

works but didn't review the code

@PVince81
Copy link
Member

PVince81 commented Oct 14, 2022

I've retested admin vs user background and no more shift+refresh is needed, it works smoothly and as expected.
Likewise, the public page had the background configured by the admin

Shipit!

@PVince81 PVince81 mentioned this pull request Oct 14, 2022
Closed
1 task
@PVince81
Copy link
Member

PVince81 commented Oct 14, 2022

I've raised the lower priority #34606 to clean up the old theme cache data since we now moved everything into "appdata*/theming/global/"

@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 14, 2022
@PVince81 PVince81 mentioned this pull request Oct 14, 2022
Closed
9 tasks
@PVince81
Copy link
Member

PVince81 commented Oct 14, 2022

/rebase

skjnldsv and others added 5 commits October 14, 2022 16:18
@PVince81 PVince81 force-pushed the fix/background-appdata-scrope branch from acc7a2c to 19f35f4 Compare October 14, 2022 14:18
@PVince81 PVince81 mentioned this pull request Oct 14, 2022
Merged
@PVince81
Copy link
Member

PVince81 commented Oct 14, 2022

rebased for CI

and manual backport to save some time... #34612

@blizzz
Copy link
Member

blizzz commented Oct 14, 2022

backgroundVersion is not used anymore, but still migrated in core/Migrations/Version25000Date20221007010957.php. Can be cleaned up afterwards though, just not to forget it.

@PVince81 PVince81 merged commit 2b87aeb into master Oct 14, 2022
@PVince81 PVince81 deleted the fix/background-appdata-scrope branch October 14, 2022 15:54
@PVince81 PVince81 mentioned this pull request Jan 19, 2023
Closed
9 tasks
@icewind1991 icewind1991 mentioned this pull request May 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PVince81 PVince81 PVince81 approved these changes

@szaimen szaimen szaimen approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

@blizzz blizzz Awaiting requested review from blizzz

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees

@skjnldsv skjnldsv

Labels

4. to release Ready to be released and/or waiting for tests to finish bug high regression

Projects

None yet

Milestone

Nextcloud 26

Development

Successfully merging this pull request may close these issues.

[Bug] [25.0.0RC4] Custom background doesn't stay (404)

5 participants

@skjnldsv @PVince81 @szaimen @blizzz