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

Fix wrong test in test_organization #705

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

faebebin
Copy link
Member

@faebebin faebebin commented Jun 27, 2023

I am pretty sure this is meant?

@faebebin faebebin requested a review from suricactus June 27, 2023 14:49
@faebebin faebebin added patch Requires patch version change chore Maintanance, clean-up and other not fancy improvements. labels Jun 27, 2023
@@ -259,7 +259,7 @@ def _active_users_count(base_date=None):
# User 3 creates a job
Job.objects.create(
project=project1,
created_by=self.user3,
created_by=self.user4,
)
Copy link
Member Author

@faebebin faebebin Jun 27, 2023

Choose a reason for hiding this comment

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

Anyway I would suggest reqriting all test comments so that are easier to read. The comments mention User 1 and the refer to self.user2 and User 2 referring to self.user3 ... . This is super hard to read and probably led to this bug?

I would suggest using user2 directly in the comment or writing:

# 1. user2 creates ...
# 2. user3 does x
...

what you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Absolutely in favor of everything that easily adds a readability improvement.

@suricactus suricactus merged commit 3756840 into master Jul 3, 2023
@suricactus suricactus deleted the fix-wrong-test-organiztation branch July 3, 2023 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Maintanance, clean-up and other not fancy improvements. patch Requires patch version change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants