-
Notifications
You must be signed in to change notification settings - Fork 48
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
Adding missing help_text and check #647
base: devel
Are you sure you want to change the base?
Conversation
cd8f8aa
to
b6d9126
Compare
name = models.CharField("name", max_length=255) | ||
content_type = models.ForeignKey(ContentType, models.CASCADE, verbose_name="content type") | ||
codename = models.CharField("codename", max_length=100) | ||
name = models.CharField("name", max_length=255, help_text=_("The name of this permission.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll find that in a lot of these, they are not exposed in the API.
This model is also a mirror of auth.Permission
so why doesn't it have help_text? Because the model it mirrors doesn't have help text.
The permission name is more of a python / admin pages display oriented thing. Hopefully we don't ever use it. If you wanted to get more specific about that, you could. Again, it shouldn't be outward API-facing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I take the help_text
off these and add them to the .help_text.ignore
file or just leave it on for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The permission model, itself, maybe one day (soon-ish) be surfaced in the API in a read-only way. So it might be good to have these, although I realize I probably just have to make a PR.
Quality Gate passedIssues Measures |
content_type = models.ForeignKey(ContentType, models.CASCADE, verbose_name="content type") | ||
codename = models.CharField("codename", max_length=100) | ||
name = models.CharField("name", max_length=255, help_text=_("The name of this permission.")) | ||
content_type = models.ForeignKey(ContentType, models.CASCADE, verbose_name="content type", help_text=_("The content type this permission will apply to.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say we have a permission statement "John can pet the dog named Fido". That would be an instantiation of Permission
(note that a permission entry it not an instantiation), the content type of the permission being dog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing more comments further down, I realize "subject" is a hazardous word. "object" might be better. English SVO grammar structure would imply "subject" is what is referred to as "actor" elsewhere.
codename = models.CharField( | ||
"codename", | ||
max_length=100, | ||
help_text=_("The name of the permission, giving the action and the model, from the Django Permission model."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only one that matters, and unfortunately, you can't say the format rule always applies, but this does conform to the existing Django auth app pattern.
So the format of the permission is usually in the form of {action}_{model_name}
, but can be customized for a specific entry in a model's Meta class. The model name is well-defined formally, and the action is usually / probably the DRF viewset action. This is actually under-specified and in some internal Django uses the actually unique combination of (app_label, model_name, action)
is used. But in DAB RBAC, it is expected to be unique without app_label, due to additional enforcement.
|
||
# human readable name for the resource | ||
name = models.CharField(max_length=512, null=True) | ||
name = models.CharField(max_length=512, null=True, help_text=_("The name of this resource.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might not be the name...
Remind me again why we have this? Oof, ask @newswangerd
b6d9126
to
3c97fb3
Compare
This adds a help_text check, validating that every model has all of its help_text defined.
It also addresses missing help_text from existing models.