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

Make default access policy configurable #61

Closed
wants to merge 2 commits into from
Closed

Make default access policy configurable #61

wants to merge 2 commits into from

Conversation

vain
Copy link
Contributor

@vain vain commented May 27, 2015

I'm not sure if code quality is up to your standards. Sorry. :(

My goal was not to move the ACCESS_POLICY_* constants. Hence, configure_default_access_policy() only reads a string and pick_constant() returns the appropriate constant.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.79%) to 20.72% when pulling 9e18766 on vain:ap-conf into b53e2bf on trehn:master.

@trehn
Copy link
Member

trehn commented May 27, 2015

I'm afraid that won't work. The access policy names are marked for translation, meaning you can't just compare them to the names you use in the config file. You'd have to introduce another mapping from the names in the config file (which are not translateable) to the numeric constants.

Taking a step back I don't believe this is something that belongs in the config file. There is already a model to store settings in the database and I believe that people should be able to change the default access policy at runtime without restarting the entire application. The problem is that there is not yet a UI for manipulating these settings. So I guess we could put it in the config file for now and move it to the database later...

@mseibert
Copy link

I have added an issue with my view: #66

@vain
Copy link
Contributor Author

vain commented Aug 1, 2015

Screw this PR. My patch doesn't work. The issue itself is addressed by #66.

@vain vain closed this Aug 1, 2015
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.

None yet

4 participants