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

[Dev] Configuration: Port rewrite checks to NewSetting class #450

Closed
3 tasks done
ljacqu opened this issue Jan 17, 2016 · 3 comments
Closed
3 tasks done

[Dev] Configuration: Port rewrite checks to NewSetting class #450

ljacqu opened this issue Jan 17, 2016 · 3 comments
Assignees
Milestone

Comments

@ljacqu
Copy link
Member

ljacqu commented Jan 17, 2016

We are temporarily using Settings and NewSetting. The goal is to eventually use only NewSetting.

  • Before this can be done, we need to be able to write proper YAML in NewSetting. This should be done as much as possible by the methods SnakeYAML offers. Integration test should still work and could be enhanced a little more (e.g. have a value with a backslash, have a value with an apostrophe, have a string list with a multi-line value... Just values that might potentially break)
  • Right now we check in Settings whether we need to rewrite the config.yml file, if for example a setting is missing or an old setting is present. The task of this issue is to move this logic to the NewSetting class and to have that class decide when we need to rewrite to config.yml.
    It should be possible to do it a little more generically with the PropertyMap, e.g. to check if a property is missing.
  • Test before merging (welcome message, email message, rewrite, handling of missing config file, handling of missing email file, isEmailCorrect, force migration flat -> sqlite, force migration of PLAINTEXT hash algorithm)
@sgdc3
Copy link
Member

sgdc3 commented Jan 18, 2016

Why we can't just rename NewSettings to Settings?

@ljacqu
Copy link
Member Author

ljacqu commented Jan 18, 2016

We can once Settings is deleted. I don't think it would make sense to gradually change the code in Settings.
Edit: Now I understand how the description of the issue is kind of misleading: it sounds like we'll just keep "NewSetting" forever called like that. That wasn't the intention. :)

@ljacqu
Copy link
Member Author

ljacqu commented Jan 30, 2016

TODO:

  • ReloadCommand needs to be changed to speak with NewSettings
  • AuthMe main class uses Settings#set at one point...
  • AuthMe main class + ForceFlatToSqlite uses Settings#setValue. Both need to be ported
  • Need to relocate Settings#getRestrictedIp, Settings#isEmailCorrect
  • Do equivalent of Settings#loadEmailText and #getWelcomeMessage
  • Need to port check from Settings#checkLang

@ljacqu ljacqu closed this as completed Feb 6, 2016
@ljacqu ljacqu added this to the 5.2 Release milestone Feb 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants