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

Feature/grpc #550

Merged
merged 29 commits into from
Mar 24, 2021
Merged

Feature/grpc #550

merged 29 commits into from
Mar 24, 2021

Conversation

dyohan9
Copy link
Contributor

@dyohan9 dyohan9 commented Mar 12, 2021

No description provided.

@dyohan9 dyohan9 added the enhancement New feature or request label Mar 12, 2021
@dyohan9 dyohan9 requested a review from mldzs March 12, 2021 15:10
@dyohan9 dyohan9 self-assigned this Mar 12, 2021
bothub/api/grpc/user/services.py Show resolved Hide resolved

return serializer.message

def remove_user_permission(self, org: Organization, user: User, permission: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't that be a function outside the class? If you need to use this functionality elsewhere in the code, you will have to rewrite it, causing duplicate code

permissions = self.get_user_permissions(org, user)
permissions.delete()

def set_user_permission(self, org: Organization, user: User, permission: int):
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't that be a function outside the class? If you need to use this functionality elsewhere in the code, you will have to rewrite it, causing duplicate code

bothub/api/grpc/user/services.py Show resolved Hide resolved
bothub/utils.py Outdated
@@ -324,3 +351,54 @@ class CountSubquery(Subquery):

def __init__(self, queryset, output_field=None, **extra):
super().__init__(queryset, output_field, **extra)


class AbstractService:
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to call yourself "Abstract Service"? since it implements specific methods? Perhaps it would be necessary to change the nomenclature.

Why is it called "Abstract" if it doesn't have any abstract methods?

bothub/utils.py Outdated
)


class AbstractUserService(generics.GenericService):
Copy link
Contributor

Choose a reason for hiding this comment

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

the comments cited above are also replicated here

grpc.StatusCode.NOT_FOUND, f"User:{request.user_email} not found!"
)

def get_orgs(self, user: User):
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't that be a function outside the class? If you need to use this functionality elsewhere in the code, you will have to rewrite it, causing duplicate code

from bothub.protos import organization_pb2


class SerializerUtils(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

here you used Serializer at the beginning of the class, in the others you used it at the end of the name. It would be interesting to standardize

key: value for key, value in data.items() if key not in ["id", "user_email"]
}

def _user_has_permisson(self, user: User, org: Organization) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't that be a function outside the class? If you need to use this functionality elsewhere in the code, you will have to rewrite it, causing duplicate code

bothub/api/grpc/organization/serializers.py Show resolved Hide resolved
Copy link
Contributor

@mldzs mldzs left a comment

Choose a reason for hiding this comment

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

The code is fine, but maybe you should think about organizing the code better. Abuse of MVC, placing business rules in the model. Do not use inheritance to reuse code, among other things. This way we avoid code smells and future problems

@sonarcloud
Copy link

sonarcloud bot commented Mar 24, 2021

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mldzs mldzs merged commit fd93d9a into develop Mar 24, 2021
@mldzs mldzs deleted the feature/grpc branch March 24, 2021 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants