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

ensure the user field in categorizers table is one to one #148

Merged
merged 10 commits into from
Dec 1, 2021

Conversation

ZhongweiL
Copy link
Contributor

@ZhongweiL ZhongweiL commented Nov 17, 2021

Intends to resolve issue #147 and #132.

ponder/models.py Outdated
@@ -62,7 +62,8 @@ def email_categorizer(self):
class Categorizer(models.Model):
name = models.CharField(unique=True, max_length=254)
initials = models.CharField(unique=True, max_length=3)
user = models.CharField(max_length=254)
#user = models.CharField(max_length=254)
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this line instead of commenting.

@khatchad
Copy link
Member

@ZhongweiL Are we ready to merge this?

@ZhongweiL
Copy link
Contributor Author

@khatchad Yes.

@khatchad
Copy link
Member

Didn't we discuss tests?

@khatchad
Copy link
Member

#147 (comment)

@ZhongweiL
Copy link
Contributor Author

I was going to create another PR for that, but I can do that in this branch if you want.

ZhongweiL and others added 7 commits November 28, 2021 17:30
Add comments.
Comments.
Initials should be unique.
Clarify tests.
More comments.
Remove comment.
@khatchad khatchad requested a review from mZneit November 30, 2021 22:50
@khatchad
Copy link
Member

@mZneit We can't get the tests to run for this PR but the changes seem good to us. Please review and merge. Thanks,.

@khatchad khatchad requested review from khatchad and removed request for khatchad November 30, 2021 22:53
@mZneit mZneit merged commit 116aac8 into ponder-lab:main Dec 1, 2021
@mZneit
Copy link
Contributor

mZneit commented Dec 2, 2021

@khatchad This seems to be a MySQL issue. Some tests ran when changes were applied to the database directly, but when applying the same changes using django code, they had no effect in MySQL.

@khatchad
Copy link
Member

khatchad commented Dec 2, 2021

Hm. But, since you've merged this change, the tests are passing. Are they running but have no effect?

@khatchad
Copy link
Member

khatchad commented Dec 2, 2021

@khatchad
Copy link
Member

khatchad commented Dec 2, 2021

@ZhongweiL Looks like your test test_same_initials and others are failing now in the main branch (I still don't know why we couldn't get this running inside the PR, but that is a different problem). Can you see why this is the case? It looks like there's a constraint violation. Please output the SQL command here to debug it. Thanks. Let us know if the test code needs to change or there is a problem with the tested code. Maybe the DB starts with info and you need to delete them in some order?

@ZhongweiL
Copy link
Contributor Author

@khatchad From this answer, it seems like Django wraps each test into a transaction, and Django prevents any ORM operation in the same transaction after an exception is thrown. The code can be fixed by deleting the last two assertions in the function. Another way to solve it is to use the solution in the post which is to limit the scope of the transaction to just that one line, the code would look like this:

        try:
            with transaction.atomic():
                Categorizer.objects.create(name='Jane Scott', initials='JS', user=self.user2)
            self.fail()
        except IntegrityError:
            pass

which will fail the test if the exception was not thrown, and does nothing if it was thrown. But this time the last two assertions can be run because they are not in the same transaction as the categorizer insertion. But I don't think the second solution is necessary unless we really want to keep the last two assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants