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 #701 Preferable default theme #746

Merged
merged 3 commits into from
Apr 12, 2020

Conversation

KitaitiMakoto
Copy link
Contributor

This patch fixes #701 .

The issue occurs because form value of email is empty string "" when user selects "Default theme" and it is set as preferred theme name. There's no theme named "", so any theme is not applied. By setting NULL to users.preferred_theme when "Default theme" is selected, instance theme is used as the user's preferred theme.

I used Diesel's #[changeset_options(treat_none_as_null="true")] attribute on User model. This may not be preferred but save_changes doesn't have option to set NULL sometimes and keep current value otherwise(See http://diesel.rs/guides/all-about-updates/ . It says "Diesel doesn't currently provide a way to explicitly assign a field to its default value"). This is the reason why I used that attribute, but it may have too wide inpact. Could you consider?

@codecov
Copy link

codecov bot commented Apr 12, 2020

Codecov Report

Merging #746 into master will not change coverage by %.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master     #746   +/-   ##
=======================================
  Coverage   39.00%   39.00%           
=======================================
  Files          73       73           
  Lines        9699     9699           
  Branches     2229     2229           
=======================================
  Hits         3783     3783           
- Misses       4754     4787   +33     
+ Partials     1162     1129   -33     

Copy link
Member

@elegaanz elegaanz left a comment

Choose a reason for hiding this comment

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

I did a quick check to see if it could impact something else, but it looks like it is safe. Thanks for contributing!

Copy link
Member

@elegaanz elegaanz left a comment

Choose a reason for hiding this comment

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

Woops, forgot to approve

@elegaanz elegaanz merged commit b834d1c into Plume-org:master Apr 12, 2020
@KitaitiMakoto
Copy link
Contributor Author

Thank you for merging!

@KitaitiMakoto KitaitiMakoto deleted the preferred-default-theme branch April 12, 2020 15:43
@verymilan
Copy link

Sadly a new user of mine still just ran into missing CSS on 0.5.0 :(

@KitaitiMakoto
Copy link
Contributor Author

@verymilan Can you re-save configuration again, please?

@verymilan
Copy link

I am not sure what you mean, but as usual, i have made them selecting a theme and save, which was fixing their issue as always

@KitaitiMakoto
Copy link
Contributor Author

KitaitiMakoto commented Jun 22, 2020

Sorry for my poor English. What I did mean is...

Can you have your users select "Default theme" and save it? It might be fix the issue?

Under the hood, their current theme is saved as empty string "" but re-saving theme makes it NULL. And then default theme is applied.

@verymilan
Copy link

verymilan commented Jun 22, 2020

Yes, i know, and this is how we did it - i was just expecting assuming that this workaround wasn't needed anymore.

@KitaitiMakoto
Copy link
Contributor Author

Ah, sorry, I missed your point.

i was just expecting assuming that this workaround wasn't needed anymore.

Yes, your realization is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No theme at all on "Default theme"
3 participants