Refactor SQL in testcases and fix #174 #177

Merged
merged 3 commits into from May 1, 2017

Conversation

Projects
None yet
3 participants
@atodorov
Contributor

atodorov commented Apr 14, 2017

This is a minor refactoring covered by existing tests.

The second commit fixes #174. I will see if I can add automated tests for it as well but please review.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 14, 2017

Coverage Status

Coverage decreased (-0.08%) to 45.303% when pulling 56d4f8b on MrSenko:refactor_sqls_in_testcases into 8f45beb on Nitrate:develop.

Coverage Status

Coverage decreased (-0.08%) to 45.303% when pulling 56d4f8b on MrSenko:refactor_sqls_in_testcases into 8f45beb on Nitrate:develop.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 14, 2017

Coverage Status

Coverage decreased (-0.03%) to 45.345% when pulling 3c8ff5c on MrSenko:refactor_sqls_in_testcases into 8f45beb on Nitrate:develop.

Coverage Status

Coverage decreased (-0.03%) to 45.345% when pulling 3c8ff5c on MrSenko:refactor_sqls_in_testcases into 8f45beb on Nitrate:develop.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Apr 14, 2017

Contributor

Note to self: the print and export SQLs are very similar to the ones used in testplan/ so it is best they be refactored after #172 has been merged

Contributor

atodorov commented Apr 14, 2017

Note to self: the print and export SQLs are very similar to the ones used in testplan/ so it is best they be refactored after #172 has been merged

atodorov added a commit to MrSenko/Nitrate that referenced this pull request Apr 16, 2017

Fix adding tags to test cases in TP
removal of tags bug is #174 which is fixed in PR #177

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

atodorov added a commit to MrSenko/Nitrate that referenced this pull request Apr 19, 2017

Fix tags search and hint while adding tags to test cases in TP
How to reproduce:
- Open a Test Plan
- Go to Cases tab
- Select few test cases
- click Tag -> Add Tag button
- start typing tag name
- if there are tags which match the text they will be shown
  ad a drop-down hint. This was previously raising an exception.

Also simplified the strip_parameters() function which is
used internally and made minor updates to the template and
JavaScript files to match the backend changes and model field names.

Note:
Removal of tags bug is #174 which is fixed in PR #177

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

atodorov added a commit to MrSenko/Nitrate that referenced this pull request May 1, 2017

Fix tags search and hint while adding tags to test cases in TP
How to reproduce:
- Open a Test Plan
- Go to Cases tab
- Select few test cases
- click Tag -> Add Tag button
- start typing tag name
- if there are tags which match the text they will be shown
  ad a drop-down hint. This was previously raising an exception.

Also simplified the strip_parameters() function which is
used internally and made minor updates to the template and
JavaScript files to match the backend changes and model field names.

Note:
Removal of tags bug is #174 which is fixed in PR #177

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
tcms/testcases/models.py
- cursor.execute(SQL.REMOVE_COMPONENT, (self.case_id, component.id))
+ # note: cannot use self.component.remove(component) on a ManyToManyField
+ # which specifies an intermediary model so we use the model manager!
+ self.component.through.objects.filter(case=self.pk, component=component.pk).delete()

This comment has been minimized.

@tkdchen

tkdchen May 1, 2017

Member

Use intermediary table can make it much easy to understand.

TestCaseComponent.objects.filter(case=self, component=component).delete()

It's straightforward and no need of comment here.

Same comment to below changes.

@tkdchen

tkdchen May 1, 2017

Member

Use intermediary table can make it much easy to understand.

TestCaseComponent.objects.filter(case=self, component=component).delete()

It's straightforward and no need of comment here.

Same comment to below changes.

@@ -1696,7 +1697,7 @@ def tag(request):
return HttpResponse(json.dumps(ajax_response))
return HttpResponse(json.dumps(ajax_response))
- form = CaseTagForm(initial={'tag': request.REQUEST.get('o_tag')})
+ form = CaseTagForm(initial={'tag': request.POST.get('o_tag')})

This comment has been minimized.

@tkdchen

tkdchen May 1, 2017

Member

These changes in views.py are related to the issue replacing request.REQUEST that I'm working on. I'll fix it later. I think you can remove these changes from this PR.

@tkdchen

tkdchen May 1, 2017

Member

These changes in views.py are related to the issue replacing request.REQUEST that I'm working on. I'll fix it later. I think you can remove these changes from this PR.

This comment has been minimized.

@atodorov

atodorov May 1, 2017

Contributor

This is why we use git, if we do conflict one of the PRs gets precedence (I'd say the one which was opened first) and the other one simply rebases and resolves the conflict once the first PR is merged.

I have split this change into a separate commit b/c it is independent of the other changes but I'm not going to remove it.

@atodorov

atodorov May 1, 2017

Contributor

This is why we use git, if we do conflict one of the PRs gets precedence (I'd say the one which was opened first) and the other one simply rebases and resolves the conflict once the first PR is merged.

I have split this change into a separate commit b/c it is independent of the other changes but I'm not going to remove it.

tcms/testcases/tests.py
@@ -197,3 +198,33 @@ def test_remove_tag(self):
tag_pks = list(self.case.tag.all().values_list('pk', flat=True))
self.assertEqual([self.tag_fedora.pk], tag_pks)
+
+
+class CaseTagFormTest(BasePlanCase):

This comment has been minimized.

@tkdchen

tkdchen May 1, 2017

Member

Looks this test case does not need inherit from BasePlanCase, as no data created in super setUpTestData is used.

@tkdchen

tkdchen May 1, 2017

Member

Looks this test case does not need inherit from BasePlanCase, as no data created in super setUpTestData is used.

tcms/testcases/forms.py
+ tag_ids = TestCaseTag.objects.filter(
+ case__in=case_ids
+ ).values_list('tag').distinct()
+ self.fields['o_tag'].queryset = TestTag.objects.filter(pk__in=tag_ids).order_by('name')

This comment has been minimized.

@tkdchen

tkdchen May 1, 2017

Member

Replace testcase__in with cases__in works. No need to query TestCaseTag separately.

@tkdchen

tkdchen May 1, 2017

Member

Replace testcase__in with cases__in works. No need to query TestCaseTag separately.

This comment has been minimized.

@atodorov

atodorov May 1, 2017

Contributor

OK, this is counter intuitive but works. Django is able to follow backwards relationships filter. In this case the relationship is TestCaseTag -> TestTag but Django is able to figure out how to go backwards by using inner joins.

IMO this is a really bad side of Django ORM because it may lead to unpredicted results. E.g. if you forget .distinct() at the end the results are different. Anyway I've added a comment in the code for future reference.

@atodorov

atodorov May 1, 2017

Contributor

OK, this is counter intuitive but works. Django is able to follow backwards relationships filter. In this case the relationship is TestCaseTag -> TestTag but Django is able to figure out how to go backwards by using inner joins.

IMO this is a really bad side of Django ORM because it may lead to unpredicted results. E.g. if you forget .distinct() at the end the results are different. Anyway I've added a comment in the code for future reference.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen May 1, 2017

Member

Please move Fix #174 into commit message body and sign-off these two commits. Thanks.

Member

tkdchen commented May 1, 2017

Please move Fix #174 into commit message body and sign-off these two commits. Thanks.

atodorov added some commits Apr 14, 2017

Simplify removal SQLs for testcases
already covered by existing tests

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Replace deprecated request.REQUEST with request.POST
Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 1, 2017

Coverage Status

Coverage decreased (-0.03%) to 45.331% when pulling 59c13d7 on MrSenko:refactor_sqls_in_testcases into a2363f8 on Nitrate:develop.

Coverage Status

Coverage decreased (-0.03%) to 45.331% when pulling 59c13d7 on MrSenko:refactor_sqls_in_testcases into a2363f8 on Nitrate:develop.

Fix traceback when removing test case tags and add test for it
Fix #174

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

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov May 1, 2017

Contributor

Signed & rebased to latest upstream.

Contributor

atodorov commented May 1, 2017

Signed & rebased to latest upstream.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 1, 2017

Coverage Status

Coverage decreased (-0.04%) to 45.327% when pulling f124f17 on MrSenko:refactor_sqls_in_testcases into a2363f8 on Nitrate:develop.

Coverage Status

Coverage decreased (-0.04%) to 45.327% when pulling f124f17 on MrSenko:refactor_sqls_in_testcases into a2363f8 on Nitrate:develop.

@tkdchen tkdchen merged commit d1c5f80 into Nitrate:develop May 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen May 1, 2017

Member

Merged. Thanks.

Member

tkdchen commented May 1, 2017

Merged. Thanks.

atodorov added a commit to MrSenko/Nitrate that referenced this pull request May 12, 2017

Fix tags search and hint while adding tags to test cases in TP
How to reproduce:
- Open a Test Plan
- Go to Cases tab
- Select few test cases
- click Tag -> Add Tag button
- start typing tag name
- if there are tags which match the text they will be shown
  ad a drop-down hint. This was previously raising an exception.

Also simplified the strip_parameters() function which is
used internally and made minor updates to the template and
JavaScript files to match the backend changes and model field names.

Note:
Removal of tags bug is #174 which is fixed in PR #177

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

atodorov added a commit to MrSenko/Nitrate that referenced this pull request May 12, 2017

Fix tags search and hint while adding tags to test cases in TP
How to reproduce:
- Open a Test Plan
- Go to Cases tab
- Select few test cases
- click Tag -> Add Tag button
- start typing tag name
- if there are tags which match the text they will be shown
  as a drop-down hint. This was previously raising an exception.

Also introduced a slight performance optimization by using
QuerySet.values() method to fetch only the field name we're interested
in, not everything.

Note: this commit makes use of the TestCaseTag class instead of the
TestTag class in order to convert ORM queries to child -> parent
relationship, instead of parent -> child one.

Updated JavaScript files to match the backend changes and model
field names.

Note:
Removal of tags bug is #174 which is fixed in PR #177

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

atodorov added a commit to MrSenko/Nitrate that referenced this pull request May 24, 2017

Fix tags search and hint while adding tags to test cases in TP
How to reproduce:
- Open a Test Plan
- Go to Cases tab
- Select few test cases
- click Tag -> Add Tag button
- start typing tag name
- if there are tags which match the text they will be shown
  ad a drop-down hint. This was previously raising an exception.

Also simplified the strip_parameters() function which is
used internally and made minor updates to the template and
JavaScript files to match the backend changes and model field names.

Note:
Removal of tags bug is #174 which is fixed in PR #177

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment