Fix #701 Preferable default theme #746

병합
KitaitiMakoto preferred-default-theme 에서 master 로 3 commits 를 머지했습니다 4 년 전
KitaitiMakoto 코멘트됨, 4 년 전 (Migrated from github.com)

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?

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[bot] 코멘트됨, 4 년 전 (Migrated from github.com)

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     
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/746?src=pr&el=h1) Report > Merging [#746](https://codecov.io/gh/Plume-org/Plume/pull/746?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/506fe9955dbda055f70655c8e862c77678e9c567&el=desc) will **not change** coverage by `%`. > The diff coverage is `0.00%`. ```diff @@ 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 ```
elegaanz (Migrated from github.com) 검토됨 4 년 전
elegaanz (Migrated from github.com) left a comment

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

I did a quick check to see if it could impact something else, but it looks like it is safe. Thanks for contributing!
elegaanz (Migrated from github.com) 이 변경사항을 승인하였습니다. 4 년 전
elegaanz (Migrated from github.com) left a comment

Woops, forgot to approve

Woops, forgot to approve
KitaitiMakoto 코멘트됨, 4 년 전 (Migrated from github.com)

Thank you for merging!

Thank you for merging!
verymilan 코멘트됨, 4 년 전 (Migrated from github.com)

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

Sadly a new user of mine still just ran into missing CSS on 0.5.0 :(
KitaitiMakoto 코멘트됨, 4 년 전 (Migrated from github.com)

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

@verymilan Can you re-save configuration again, please?
verymilan 코멘트됨, 4 년 전 (Migrated from github.com)

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

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 코멘트됨, 4 년 전 (Migrated from github.com)

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.

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 코멘트됨, 4 년 전 (Migrated from github.com)

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

Yes, i know, and this is how we did it - i was just ~expecting~ assuming that this workaround wasn't needed anymore.
KitaitiMakoto 코멘트됨, 4 년 전 (Migrated from github.com)

Ah, sorry, I missed your point.

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

Yes, your realization is correct.

Ah, sorry, I missed your point. > i was just ~expecting~ assuming that this workaround wasn't needed anymore. Yes, your realization is correct.

리뷰어

The pull request has been merged as b834d1c282.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b preferred-default-theme master
git pull origin preferred-default-theme

Step 2:

Merge the changes and update on Forgejo.
git checkout master
git merge --no-ff preferred-default-theme
git push origin master
로그인하여 이 대화에 참여
No reviewers
마일스톤 없음
담당자 없음
참여자 1명
알림
마감일
기한이 올바르지 않거나 범위를 벗어났습니다. 'yyyy-mm-dd'형식을 사용해주십시오.

마감일이 설정되지 않았습니다.

의존성

No dependencies set.

Reference: Plume/Plume#746
불러오는 중...
아직 콘텐츠가 없습니다.