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 Issue #181 #182

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@atodorov
Contributor

atodorov commented Apr 26, 2017

Adds a test which demonstrates the bug and then a fix for it. All explanations are in the commit log.

Notes:

----------------------------- Captured stdout call -----------------------------

************* 0

which is produced by the following code attached to post_delete

 def on_plan_delete(sender, instance, **kwargs):
+    from tcms.testplans.models import TestPlanEmailSettings
+    print "*************", TestPlanEmailSettings.objects.count()
+
     # email this deletion
     if instance.emailing.notify_on_plan_delete:
         email.email_plan_deletion(instance)

@tkdchen please review.

Note2: @tkdchen the CI logs above are produced by cherry-picking the current 2 commits on top of #171 which is still not merged. They also show that the same test produces different results when executed against PostgreSQL and SQLite.

atodorov added some commits Apr 25, 2017

Add test for #181, fails only on MySQL
Note: on MySQL the test will fail with foreign key constraint
violation b/c TestPlanEmailSettings, which has 1-1 mapping with
TestPlan does not exist!

The failure happens when we delete the TestPlan as a result of
cascading delete of a Product. There are signal handlers for TP
which send email notifications on deletion. TP.emailing
does not exist but b/c this is a property we try to create a
TestPlanEmailSettings object which will point to the deleted TP!

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Create email settings for new TestPlan. Fix #181
also change post_delete to pre_delete signal.

Explanation:

There is a TestPlan object and the TestPlan.email_settings
attribute points to TestPlanEmailSettings object. When we
delete the TP Django will first delete the TPES object, then
initiate TP.delete() and at the end of it send the post_delete
signal. Upon receiving of this signal the TPES object doesn't
exist anymore and trying to access it via TP.emailing property
tries to create a new TPES object pointing to the TP instance.

Because it has already been deleted from the DB MySQL integrity
check fails. The only way is to notify users about plan deletion
using a pre_delete signal because all objects are still available
in the database.

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 26, 2017

Coverage Status

Coverage increased (+0.3%) to 45.646% when pulling 5fa9891 on MrSenko:fix_181 into a2363f8 on Nitrate:develop.

Coverage Status

Coverage increased (+0.3%) to 45.646% when pulling 5fa9891 on MrSenko:fix_181 into a2363f8 on Nitrate:develop.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 14, 2018

Member

Updated and merged in #305. Thanks.

Member

tkdchen commented Apr 14, 2018

Updated and merged in #305. Thanks.

@tkdchen tkdchen closed this Apr 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment