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

Custom ReachedMaxOrganizationMembersError exception instead of generic ValidationError #593

Merged

Conversation

why-not-try-calmer
Copy link
Contributor

No description provided.

@duke-nyuki
Copy link
Collaborator

@why-not-try-calmer why-not-try-calmer added the enhancement New feature or request label Mar 23, 2023
@why-not-try-calmer why-not-try-calmer force-pushed the QF-2539_error_not_exception_on_cannot_create_member branch from c5e4313 to 4e758bb Compare March 23, 2023 10:09
@@ -167,3 +167,7 @@ class ProjectAlreadyExistsError(QFieldCloudException):
code = "project_already_exists"
message = "This user already owns a project with the same name."
status_code = status.HTTP_400_BAD_REQUEST


class ReachedMaxOrgaMembers(Exception):
Copy link
Collaborator

@suricactus suricactus Mar 23, 2023

Choose a reason for hiding this comment

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

Suggested change
class ReachedMaxOrgaMembers(Exception):
class ReachedMaxOrganizationMembersError(ValidationError):

Copy link
Collaborator

@suricactus suricactus Mar 23, 2023

Choose a reason for hiding this comment

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

So far I keep everything explicit without a lot of shortening. More typing, but clear explicitness.

Using ValidationError as parent, will make Django help us in some situations with automatically showing the error.

Copy link
Contributor Author

@why-not-try-calmer why-not-try-calmer Mar 23, 2023

Choose a reason for hiding this comment

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

Despite my "accept commit" the thing I don't like is that it's not a validation error, and moreover should not to have to declare a status_code. That's why I refrained from sublcassing ValidationError. Can we find a middle ground? Perhaps more fine-grained than an Exception but not a subtype of ValidationError?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do agree with your objections and it is something we can look into in the future for sure. Please add a TODO comment as a reminder before the exception definition.

I would propose we keep it as a ValidationError for now, until we find a better solution, so we minimize the risk of breaking something in the code that relied on this ValidationError class.

Copy link
Collaborator

@suricactus suricactus Mar 23, 2023

Choose a reason for hiding this comment

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

ok, second thought. Define it like the rest of exceptions in the file, but extend QFieldCloudException.

YOLO if someone relies on ValidationError subclass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if the just added status code is correct though.

why-not-try-calmer and others added 2 commits March 23, 2023 13:53
Co-authored-by: Ivan Ivanov <suricactus@users.noreply.github.com>
@suricactus suricactus changed the title Custom exception Custom ReachedMaxOrganizationMembersError exception instead of generic ValidationError Mar 23, 2023
@suricactus
Copy link
Collaborator

You also need to fix the tests, e.g. test_max_organization_members_raises_when_adding_new_member_over_limit .

@suricactus suricactus merged commit e5ff8f3 into master Mar 23, 2023
@suricactus suricactus deleted the QF-2539_error_not_exception_on_cannot_create_member branch March 23, 2023 16:13
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.

3 participants