Fix missing background on upgrade #34461

Pytal · 2022-10-07T02:31:12Z

Background settings previously were set in the oc_preferences table with the appid "dashboard", after #33733 the settings were queried for with the appid "theming"

This migration aims to fix the background loss on upgrade by updating the appid to "theming"

Pytal · 2022-10-07T02:31:22Z

/backport to stable25

Pytal · 2022-10-07T02:32:40Z

The migration looks right but doesn't seem to work, any ideas why @blizzz?

core/Migrations/Version25000Date20221007010957.php

Pytal · 2022-10-13T00:28:52Z

Probably just my local instance not working but updated and ready for review

PVince81

👍 as discussed

PVince81 · 2022-10-13T07:18:18Z

@blizzz we need this for 25.0.0RC4

come-nc

I would feel better if it checked theming/background is not set before overwriting it.

PVince81 · 2022-10-13T07:49:20Z

I would feel better if it checked theming/background is not set before overwriting it.

this is what we wanted to avoid as it would require iterating over all users and slow down the migration/update

in normal cases the migration would not rerun

core/Migrations/Version25000Date20221007010957.php

PVince81 · 2022-10-13T11:27:50Z

and because of those duplicates as in #34461 (comment) we cannot simply run the update like in the first version of this PR:

MariaDB [nextcloud]> update oc_preferences set appid='dashboard' where configkey in ('background', 'backgroundVersion');
ERROR 1062 (23000): Duplicate entry 'bob-dashboard-background' for key 'PRIMARY'

so the proposal was to delete the backgrounds that were set post RC, here by Bob, by deleting the matching "theming" keys.

so that in the end only users (early adopters) will have to manually re-set the background

blizzz · 2022-10-13T11:28:36Z

updated the description and linked issue.

PVince81 · 2022-10-13T11:29:24Z

@blizzz please check oc_preferences for my user on c.nc.com, my background is still black as I haven't re-selected one.
then we can see the DB state

blizzz · 2022-10-13T11:31:33Z

> select appid, configkey, configvalue from oc_preferences where configkey in ('background', 'backgroundVersion') and userid = '*******';
+---------+-------------------+-------------+
| appid   | configkey         | configvalue |
+---------+-------------------+-------------+
| theming | background        | custom      |
| theming | backgroundVersion | 5           |
+---------+-------------------+-------------+

blizzz · 2022-10-13T11:32:22Z

@blizzz please check oc_preferences for my user on c.nc.com, my background is still black as I haven't re-selected one. then we can see the DB state

I had selected on in rc2 which vanished in rc3 and was all black for me. Maybe pre-condition is coming from 24.

PVince81 · 2022-10-13T11:33:45Z

I checked user "Alice" on my test system which would represent that test case and here the background is reset to default instead of black.

Maybe c.nc.com is missing that cloudy default pic ?

blizzz · 2022-10-13T11:34:13Z

I checked user "Alice" on my test system which would represent that test case and here the background is reset to default instead of black.

Maybe c.nc.com is missing that cloudy default pic ?

Nope.

PVince81 · 2022-10-13T11:35:11Z

@blizzz or maybe one needs to upgrade through RC1, RC2, etc to reach that state.

also would need to check in which key the path to the custom bkg image is set, maybe that one can be broken as well ?

blizzz · 2022-10-13T12:00:02Z

@blizzz or maybe one needs to upgrade through RC1, RC2, etc to reach that state.

also would need to check in which key the path to the custom bkg image is set, maybe that one can be broken as well ?

Maybe RC 1 then at most. I went now from 24.0.6 (dark mode, non-default stock background) via rc2 (became default background, changed to custom picked from files) and the to RC3. It stayed. So if there was something with rc (or the beta), it does not really matter now. Will close #34458 and do another check with this PR to confirm #34216 is fixed.

blizzz · 2022-10-13T12:14:49Z

Now I took 24.0.6, uploaded a file and set this as background. Then upgrade to RC3 + this patch. Now I have a black background, so, does not really work.

Database entries were set correctly, but apparently this is not enough.

blizzz · 2022-10-13T12:21:01Z

Files need to be moved

data/$APPDATA_DIR/dashboard/$USERID/background.jpg → data/$APPDATA_DIR/theming/$USERID/background.jpg

blizzz · 2022-10-13T12:39:07Z

We can do this on demand in the BackgroundService and copy the file contents around. But then we have to carry the cargo a bit.

Or we just have the fallback in the BackgroundService and add an expensive background or repair job, that would move them once in the maintenance times. Then the code can be removed for 26 (some case has to be given on chained upgrades to ensure those are run, but that's ops).

PVince81 · 2022-10-13T12:58:56Z

maybe the BackgroundService could migrate it at login time ?

PVince81 · 2022-10-13T14:35:00Z

adding the latest idea from internal discussion with @blizzz:

add a fallback in BackgroundService that checks both locations in oc_preferences and also on disk
have a time-insensitive (nightly) background job that migrates the oc_preferences keys and also the image file

there's some local WIP work towards this idea that will be pushed later

PVince81 · 2022-10-13T15:08:23Z

update:

the fallback + background job for migration is only for the background file, not the config.
we still migrate the config in this PR

- fallback to background image from old location - migrate background images to new location as insensitive job Signed-off-by: Arthur Schiwon <[email protected]>

PVince81 · 2022-10-13T17:01:29Z

I've redone this test:

Start with v24.0.6 with three users: alice, bob, charles
Login as alice and set a dashboard wallpaper from a local file
Login as bob and set a dashboard wallpaper from a local file
Upgrade to v25.0.0 RC2
As alice, set a theming wallpaper
As charles, set a theming wallpaper
Switch to this branch
Upgrade
Check all user's wallpaper

MariaDB [nextcloud]> select * from oc_preferences where appid in ('theming', 'dashboard');
+---------+-----------+-------------------+-------------+
| userid  | appid     | configkey         | configvalue |
+---------+-----------+-------------------+-------------+
| alice   | dashboard | firstRun          | 0           |
| alice   | theming   | background        | custom      |
| alice   | theming   | backgroundVersion | 2           |
| bob     | dashboard | firstRun          | 0           |
| bob     | theming   | background        | custom      |
| bob     | theming   | backgroundVersion | 1           |
| charles | dashboard | firstRun          | 0           |
+---------+-----------+-------------------+-------------+

in appdata:

dashboard
├── alice
│   └── background.jpg
└── bob
    └── background.jpg
theming
├── 1
│   ├── favIcon-core
│   ├── favIcon-dashboard
│   └── icon-core-filetypes_text.svg
├── alice
│   └── background.jpg
└── charles
    └── background.jpg

3 directories, 5 files

Alice: wallpaper is visible ✅
Bob: wallpaper is visible through fallback: ✅
Charles: wallpaper is gone, this is by design as discussed for users who created in RC2 ✅

Run background jobs
Check tree again

dashboard
├── alice
└── bob
theming
├── 1
│   ├── favIcon-core
│   ├── favIcon-dashboard
│   └── icon-core-filetypes_text.svg
├── alice
│   └── background.jpg
├── bob
│   └── background.jpg
└── charles
    └── background.jpg

4 directories, 6 files

Alice: wallpaper is visible ✅
Bob: wallpaper is visible: ✅

Note: sadly I still need to shift+refresh to get the wallpapers to refresh, separate issue

tldr; works as expected 👍

PVince81 · 2022-10-13T17:03:41Z

raise a ticket to make the background job more scalable with a lot of users as we don't want to fetch a lot of users every time the job runs @blizzz

apps/theming/lib/Jobs/MigrateBackgroundImages.php

+		$this->jobList = $jobList;
+	}
+
+	protected function run($argument): void {


apps/theming/lib/Migration/InitBackgroundImagesMigration.php

+		return 'Initialize migration of background images from dashboard to theming app';
+	}
+
+	public function run(IOutput $output) {


backportbot-nextcloud · 2022-10-13T18:16:27Z

~~The backport to stable25 failed. Please do this backport manually.~~

#34587

Pytal added bug 3. to review Waiting for reviews feature: theming feature: dashboard labels Oct 7, 2022

Pytal added this to the Nextcloud 26 milestone Oct 7, 2022

Pytal requested review from blizzz and nickvergessen October 7, 2022 02:31

Pytal self-assigned this Oct 7, 2022

backportbot-nextcloud bot added the backport-request label Oct 7, 2022

nickvergessen requested changes Oct 7, 2022

View reviewed changes

core/Migrations/Version25000Date20221007010957.php Outdated Show resolved Hide resolved

core/Migrations/Version25000Date20221007010957.php Outdated Show resolved Hide resolved

nickvergessen reviewed Oct 7, 2022

View reviewed changes

core/Migrations/Version25000Date20221007010957.php Outdated Show resolved Hide resolved

nickvergessen requested review from a team, PVince81 and juliusknorr and removed request for a team October 7, 2022 10:37

PVince81 reviewed Oct 10, 2022

View reviewed changes

core/Migrations/Version25000Date20221007010957.php Outdated Show resolved Hide resolved

PVince81 requested a review from CarlSchwan October 12, 2022 16:16

Pytal force-pushed the fix/migrate-background branch from b4c38a7 to 1954de0 Compare October 13, 2022 00:19

Pytal requested review from PVince81 and nickvergessen October 13, 2022 00:28

PVince81 approved these changes Oct 13, 2022

View reviewed changes

CarlSchwan approved these changes Oct 13, 2022

View reviewed changes

come-nc approved these changes Oct 13, 2022

View reviewed changes

CarlSchwan reviewed Oct 13, 2022

View reviewed changes

core/Migrations/Version25000Date20221007010957.php Outdated Show resolved Hide resolved

blizzz reviewed Oct 13, 2022

View reviewed changes

core/Migrations/Version25000Date20221007010957.php Outdated Show resolved Hide resolved

blizzz linked an issue Oct 13, 2022 that may be closed by this pull request

[Bug]: Background chosen in NC 24 is not migrated to NC 25 upon upgrade #34216

Closed

9 tasks

blizzz mentioned this pull request Oct 13, 2022

[Bug]: custom background disappeared after upgrade from 25 RC2 to RC3 #34458

Closed

9 tasks

graceful background image handling

59e0710

- fallback to background image from old location - migrate background images to new location as insensitive job Signed-off-by: Arthur Schiwon <[email protected]>

blizzz 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 13, 2022

blizzz mentioned this pull request Oct 13, 2022

[stable25] Fix missing background on upgrade #34587

Merged

PVince81 enabled auto-merge October 13, 2022 17:01

github-advanced-security bot found potential problems Oct 13, 2022

View reviewed changes

PVince81 merged commit c0c0387 into master Oct 13, 2022

PVince81 deleted the fix/migrate-background branch October 13, 2022 18:10

blizzz mentioned this pull request Oct 14, 2022

Optimize MigrateBackgroundImages background job #34602

Closed

skjnldsv removed the backport-request label Jan 15, 2024

Uh oh!

Fix missing background on upgrade #34461

Fix missing background on upgrade #34461

Uh oh!

Conversation

Pytal commented Oct 7, 2022 • edited by blizzz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pytal commented Oct 7, 2022

Uh oh!

Pytal commented Oct 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pytal commented Oct 13, 2022

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

come-nc left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

Uh oh!

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

blizzz commented Oct 13, 2022

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

blizzz commented Oct 13, 2022

Uh oh!

blizzz commented Oct 13, 2022

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

blizzz commented Oct 13, 2022

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

blizzz commented Oct 13, 2022

Uh oh!

blizzz commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blizzz commented Oct 13, 2022

Uh oh!

blizzz commented Oct 13, 2022

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

PVince81 commented Oct 13, 2022

Uh oh!

Check notice

Check notice

backportbot-nextcloud bot commented Oct 13, 2022 • edited by Pytal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Pytal commented Oct 7, 2022 •

edited by blizzz

Loading

blizzz commented Oct 13, 2022 •

edited

Loading

backportbot-nextcloud bot commented Oct 13, 2022 •

edited by Pytal

Loading