Skip to content

Add default permission config#348

Merged
jackycute merged 4 commits intohackmdio:masterfrom
nvsofts:add_default_permission
Feb 10, 2017
Merged

Add default permission config#348
jackycute merged 4 commits intohackmdio:masterfrom
nvsofts:add_default_permission

Conversation

@nvsofts
Copy link
Copy Markdown
Contributor

@nvsofts nvsofts commented Feb 9, 2017

Use defaultpermission(config.json) or HMD_DEFAULT_PERMISSION(environment value) to set default permission of newly created note.

@jackycute
Copy link
Copy Markdown
Member

Hi @nvsofts
Thank you very much to send this PR.
But there already is a mechanism handling the default permission.
You might need to change that part instead of response.js.
https://github.com/hackmdio/hackmd/blob/master/lib/models/note.js#L516

@SISheogorath
Copy link
Copy Markdown
Contributor

@SISheogorath
Copy link
Copy Markdown
Contributor

To mention it: it's untested which is the reason why I didn't open a PR

@nvsofts nvsofts force-pushed the add_default_permission branch from f55ffb2 to 0a7adaf Compare February 10, 2017 01:17
@nvsofts
Copy link
Copy Markdown
Contributor Author

nvsofts commented Feb 10, 2017

@jackycute use model/note.js instead of response.js (0a7adaf)

@SISheogorath
Copy link
Copy Markdown
Contributor

SISheogorath commented Feb 10, 2017

You still have a problem with an inconsistent database entry in case you have a typo in your default permission as you don't do a real validation. That should never happen.

Edit: Let me correct: you end up in a loop of rejections of the note. As your database model tries to set an invalid entry. That's ugly...

@jackycute
Copy link
Copy Markdown
Member

Hmm, I think @SISheogorath was right about the validation.
@nvsofts Could you please follow this part https://github.com/SISheogorath/hackmd/commit/1ce93737670a02e4da7462103fd233d57e5aa400#diff-f7bf2b665273dfa66ffa4ad7c0a52bb9R24 to this PR?

@jackycute
Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Member

@jackycute jackycute left a comment

Choose a reason for hiding this comment

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

Hey @nvsofts
Thanks again.
I made few suggestions for you.
Please check.

Comment thread lib/config.js Outdated

var allowfreeurl = process.env.HMD_ALLOW_FREEURL ? (process.env.HMD_ALLOW_FREEURL === 'true') : !!config.allowfreeurl;

var defaultpermission = process.env.HMD_DEFAULT_PERMISSION || config.defaultpermission || 'editable';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please validate the permission passed to here is in the scopes.
We have 6 permission types for now.

Comment thread README.md Outdated
| usecdn | `true` or `false` | set to use CDN resources or not (default is `true`) |
| allowanonymous | `true` or `false` | set to allow anonymous usage (default is `true`) |
| allowfreeurl | `true` or `false` | set to allow new note by accessing not exist note url |
| defaultpermission | `freely`, `editable`, `limited`, `locked` or `private` | when creates new note, uses this permission (only applied when logged in) |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer following description:
set notes default permission (only applied on signed users)

Comment thread README.md Outdated
| HMD_USECDN | `true` or `false` | set to use CDN resources or not (default is `true`) |
| HMD_ALLOW_ANONYMOUS | `true` or `false` | set to allow anonymous usage (default is `true`) |
| HMD_ALLOW_FREEURL | `true` or `false` | set to allow new note by accessing not exist note url |
| HMD_DEFAULT_PERMISSION | `freely`, `editable`, `limited`, `locked` or `private` | when creates new note, uses this permission (only applied when logged in) |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer following description:
set notes default permission (only applied on signed users)

@nvsofts
Copy link
Copy Markdown
Contributor Author

nvsofts commented Feb 10, 2017

@SISheogorath @jackycute followed.

@jackycute
Copy link
Copy Markdown
Member

Well, that was a fast move 😄
Nice job @nvsofts

@jackycute jackycute merged commit cd24e75 into hackmdio:master Feb 10, 2017
@nvsofts nvsofts deleted the add_default_permission branch February 10, 2017 03:00
@SISheogorath
Copy link
Copy Markdown
Contributor

@jackycute maybe I should stop writing everything into a single line :D

@jackycute
Copy link
Copy Markdown
Member

@SISheogorath Yeah, always consider readability even on writing code.
This might be useful: https://github.com/airbnb/javascript

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.

3 participants