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

fix(theming): Add migration to restore primary color after separating primary and background #47586

Merged
merged 5 commits into from
Sep 11, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Aug 28, 2024

Summary

Not sure about the user color part, but at least it works 😅
Meaning not sure if that query would not take long on large instances.

Checklist

@susnux susnux added this to the Nextcloud 31 milestone Aug 28, 2024
@susnux susnux requested review from a team, icewind1991 and sorbaugh and removed request for a team August 28, 2024 18:17
@nextcloud nextcloud deleted a comment from susnux Aug 28, 2024
@AndyScherzinger
Copy link
Member

/backport to stable30

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

use IAppConfig please :)

@susnux
Copy link
Contributor Author

susnux commented Aug 29, 2024

use IAppConfig please :)

If it is fixed now, then I love to. It had a really bad bug that you cannot mix IConfig->setAppValue and e.g. IAppConfig->setValueBool.
Meaning you have to migrate every access to that value to the types access, in every place. Otherwise after using the typed access you can no longer access it with the IConfig variant.

One recent place were we needed to fix this: #46991

come-nc
come-nc previously requested changes Sep 2, 2024
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

The config key to check if repair step ran already is too ugly, please use a migration instead which by design run only once.

@susnux
Copy link
Contributor Author

susnux commented Sep 2, 2024

@come-nc I refactored this again, but we need to put parts of the migration into a background job. As we need to iterate a list of users which could take long if many users had custom backgrounds.

I reused the logic from the dashboard -> theming repair step / background job.

So it is now working correctly for me:

Screen recording

Setup

Bildschirmaufnahme_20240902_165334.webm

After Upgrade to Nextcloud 30 / 31

Please note the wrong colors in the beginning, I triggered the migration then and reload the page:

  1. In the beginning the previous primary color is set as background color causes wrong color of icon in the top bar
  2. No primary color
  3. The admin theming colors are all wrongly reset to default

This is all fixed after the migration + background job ran:

Bildschirmaufnahme_20240902_170942.webm

@susnux susnux changed the title fix: Add repair step to restore primary color after separating primary and background fix(theming): Add migration to restore primary color after separating primary and background Sep 2, 2024
@come-nc
Copy link
Contributor

come-nc commented Sep 3, 2024

Psalm is quite angry

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
…y and background

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
…-query

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Copy link
Member

@ArtificialOwl ArtificialOwl left a comment

Choose a reason for hiding this comment

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

code looks good

@susnux susnux merged commit 5fc5148 into master Sep 11, 2024
174 checks passed
@susnux susnux deleted the fix/color branch September 11, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: custom primary color lost after upgrade from 29 to 30
5 participants