test: Add tests for permissions

[abhiabhi94]

• almost but takes test coverage to 100%.

[abhiabhi94]

Hey, see what I found testing the current permissions. The permission delete_comment is same as the default one created with by django for all models.

https://github.com/django/django/blob/main/django/contrib/auth/models.py#L35-L56

Was this done knowingly?

[abhiabhi94]

i can't think of a scenario, where this condition would become false

Comment/comment/migrations/0008_comment_urlhash.py

Lines 14 to 18 in 0e22c13

	def set_unique_urlhash(model, instance):
	if not instance.urlhash:
	instance.urlhash = generate_urlhash()
	while model.objects.filter(urlhash=instance.urlhash).exists():
	instance.urlhash = generate_urlhash()

should we probably remove this?

[Radi85]

Hey, see what I found testing the current permissions. The permission delete_comment is same as the default one created with by django for all models.

https://github.com/django/django/blob/main/django/contrib/auth/models.py#L35-L56

Was this done knowingly?

Yes, we are using get_or_create to make sure that the permission will be created if won't by default for any reason.
Our permission is matching django default one Can delete comment.

[Radi85]

i can't think of a scenario, where this condition would become false

Comment/comment/migrations/0008_comment_urlhash.py

Lines 14 to 18 in 0e22c13

	def set_unique_urlhash(model, instance):
	if not instance.urlhash:
	instance.urlhash = generate_urlhash()
	while model.objects.filter(urlhash=instance.urlhash).exists():
	instance.urlhash = generate_urlhash()

should we probably remove this?

This function was added to comply old version with the new one. I believe it will be always needed.

[abhiabhi94]

This function was added to comply old version with the new one. I believe it will be always needed.

no, my bad i should have explained the exact thing that i wanted to mention. I was just talking about the scenario where if not instance.urlhash: would return False. This is the only part which is not covered by tests.

As the attribute will only be created and filled in this migration itself, i can't think of a real world scenario where a comment can have urlhash property beforehand and the condition would become False.

I think we can safely remove this.

[abhiabhi94]

Our permission is matching django default one Can delete comment.

Is this a good thing? should we be concerned about it. I can adjust the current tests to bypass this thing but just wanted an extra set of brains to just discuss.

[Radi85]

I think we can safely remove this.

I think yes. not instance.urlhash this is always True since the comment model has no urlhash before this migration.

[Radi85]

Is this a good thing? should we be concerned about it. I can adjust the current tests to bypass this thing but just wanted an extra set of brains to just discuss.

I am not sure I fully got what your concern is here? We are using django default permission or we create it if not exist, what's the issue?

[abhiabhi94]

I am not sure I fully got what your concern is here? We are using django default permission or we create it if not exist, what's the issue?

nothing, i can't think of a reason to support my concern now. maybe i was just concerned that we may be overriding something from the default permissions, but we actually aren't. so we should be safe i think.

[Radi85]

Thanks @abhiabhi94