Fix tags search and hint while adding tags to test cases in TP #178

Open
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@atodorov
Contributor

atodorov commented Apr 19, 2017

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

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 19, 2017

Coverage Status

Coverage increased (+0.004%) to 45.37% when pulling c9b6ed6 on MrSenko:fix_adding_tags_to_test_cases_in_testplan into a1c47ec on Nitrate:develop.

Coverage Status

Coverage increased (+0.004%) to 45.37% when pulling c9b6ed6 on MrSenko:fix_adding_tags_to_test_cases_in_testplan into a1c47ec on Nitrate:develop.

return User.objects.filter(**query)
def versions(self):
from tcms.management.models import Version
return Version.objects.filter(product__id=self.product_id)
- objects = Objects(request=request, product_id=request.GET['product_id'])
+ objects = Objects(request=request, product_id=request.GET.get('product_id'))

This comment has been minimized.

@tkdchen

tkdchen May 12, 2017

Member

Only this line and above https://github.com/Nitrate/Nitrate/pull/178/files#diff-8823511185e05be4d663bf99d5baf21bR129 are required to fix the issue.

Refactor of strip_parameters should move to a separate patch with tests.

@tkdchen

tkdchen May 12, 2017

Member

Only this line and above https://github.com/Nitrate/Nitrate/pull/178/files#diff-8823511185e05be4d663bf99d5baf21bR129 are required to fix the issue.

Refactor of strip_parameters should move to a separate patch with tests.

This comment has been minimized.

@atodorov

atodorov May 12, 2017

Contributor

added tests

@atodorov

atodorov May 12, 2017

Contributor

added tests

This comment has been minimized.

@tkdchen

tkdchen May 19, 2017

Member

As mentioned 8ca779b#r116201023, please move other unrelated changes to separate patches.

@tkdchen

tkdchen May 19, 2017

Member

As mentioned 8ca779b#r116201023, please move other unrelated changes to separate patches.

@@ -231,7 +231,7 @@
</form>
</script>
<script id="batch_tag_summary_template" type="text/x-handlebars-template">
- <div class="dia_title" style=" margin:10px 20px;">You have successfully operate tags in the following case:</div>
+ <div class="dia_title" style=" margin:10px 20px;">You have successfully updated tags for the following case(s):</div>

This comment has been minimized.

@tkdchen

tkdchen May 12, 2017

Member

Please make a separate commit for this fix.

@tkdchen

tkdchen May 12, 2017

Member

Please make a separate commit for this fix.

This comment has been minimized.

@atodorov

atodorov May 12, 2017

Contributor

Done

@atodorov

atodorov May 12, 2017

Contributor

Done

- 'testcase__plan__pk': plan_id,
- 'field': 'name'
+ 'case__plan': plan_id,
+ 'field': 'tag__name'

This comment has been minimized.

@tkdchen

tkdchen May 12, 2017

Member

What is this for?

@tkdchen

tkdchen May 12, 2017

Member

What is this for?

This comment has been minimized.

@atodorov

atodorov May 12, 2017

Contributor

This is the result of replacing the TestTag ORM query with TestCaseTag. Using the child class also changes the names of the fields and the AJAX call needs to be updated appropriately.

@atodorov

atodorov May 12, 2017

Contributor

This is the result of replacing the TestTag ORM query with TestCaseTag. Using the child class also changes the names of the fields and the AJAX call needs to be updated appropriately.

- for o in obj():
- response_str += '<li>' + getattr(o, field, None) + '</li>'
+ for o in obj().values(field):
+ response_str += '<li>' + o.get(field, None) + '</li>'

This comment has been minimized.

@tkdchen

tkdchen May 12, 2017

Member

Why need to make this change?

@tkdchen

tkdchen May 12, 2017

Member

Why need to make this change?

This comment has been minimized.

@atodorov

atodorov May 12, 2017

Contributor

obj() is the tags() method from ajax.py which returns a query set and then we loop over the objects returned from this query set. Since we only need the value of a single field in this query set values(field) makes a small performance optimization and returns the values only for that single field. obj().values() in this case is the QuerySet.values method.

@atodorov

atodorov May 12, 2017

Contributor

obj() is the tags() method from ajax.py which returns a query set and then we loop over the objects returned from this query set. Since we only need the value of a single field in this query set values(field) makes a small performance optimization and returns the values only for that single field. obj().values() in this case is the QuerySet.values method.

This comment has been minimized.

@tkdchen

tkdchen May 19, 2017

Member

Good catch. This should go to a separate patch.

@tkdchen

tkdchen May 19, 2017

Member

Good catch. This should go to a separate patch.

- query = strip_parameters(request, self.internal_parameters)
- tags = TestTag.objects
+ query = strip_parameters(request.GET, self.internal_parameters)
+ tags = TestCaseTag.objects

This comment has been minimized.

@tkdchen

tkdchen May 12, 2017

Member

Why need to use TestCaseTag rather than TestTag?

@tkdchen

tkdchen May 12, 2017

Member

Why need to use TestCaseTag rather than TestTag?

This comment has been minimized.

@atodorov

atodorov May 12, 2017

Contributor

This is the same reversed child -> parent (or parent -> child) relationship that we talked about in another PR. In this case the relationship goes from TestCaseTag (child) to TestTag (parent). I think it is more natural and more understandable to always use the child -> parent side of the relationship, especially when it is very easy to do so.

The backwards relationship is possible in Django ORM but IMO it is less clear and TBH in many cases it doesn't bring any other benefits. Behind the scenes the parent -> child ORM queries are translated to inner joins which I think are also less efficient.

@atodorov

atodorov May 12, 2017

Contributor

This is the same reversed child -> parent (or parent -> child) relationship that we talked about in another PR. In this case the relationship goes from TestCaseTag (child) to TestTag (parent). I think it is more natural and more understandable to always use the child -> parent side of the relationship, especially when it is very easy to do so.

The backwards relationship is possible in Django ORM but IMO it is less clear and TBH in many cases it doesn't bring any other benefits. Behind the scenes the parent -> child ORM queries are translated to inner joins which I think are also less efficient.

This comment has been minimized.

@tkdchen

tkdchen May 19, 2017

Member

What do you mean by child -> parent relationship?

Whatever the direction of relationship, here, it only needs to query matched tags by name from test_tags table (the TestTag model). In contrast, using TestCaseTag to query tags requires inner join which is not good. I don't think this change is necessary.

@tkdchen

tkdchen May 19, 2017

Member

What do you mean by child -> parent relationship?

Whatever the direction of relationship, here, it only needs to query matched tags by name from test_tags table (the TestTag model). In contrast, using TestCaseTag to query tags requires inner join which is not good. I don't think this change is necessary.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 12, 2017

Coverage Status

Coverage increased (+0.004%) to 51.263% when pulling e7bec80 on MrSenko:fix_adding_tags_to_test_cases_in_testplan into 75472fb on Nitrate:develop.

Coverage Status

Coverage increased (+0.004%) to 51.263% when pulling e7bec80 on MrSenko:fix_adding_tags_to_test_cases_in_testplan into 75472fb on Nitrate:develop.

atodorov added some commits May 12, 2017

Update wording in template
Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
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>
Simplify strip_parameters() function and add tests for it
this makes it easier to fix the usage of deprecated request.REQUEST
later

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

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 12, 2017

Coverage Status

Coverage increased (+0.05%) to 51.305% when pulling 7687d2c on MrSenko:fix_adding_tags_to_test_cases_in_testplan into 75472fb on Nitrate:develop.

Coverage Status

Coverage increased (+0.05%) to 51.305% when pulling 7687d2c on MrSenko:fix_adding_tags_to_test_cases_in_testplan into 75472fb on Nitrate:develop.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen May 19, 2017

Member

ad55cb5 is merged.

I'll merge this 7687d2c after replacing REQUEST with POST and GET properly, because there will be tests.

Please update 8ca779b by removing unrelated code according to my comment. Thanks.

Member

tkdchen commented May 19, 2017

ad55cb5 is merged.

I'll merge this 7687d2c after replacing REQUEST with POST and GET properly, because there will be tests.

Please update 8ca779b by removing unrelated code according to my comment. Thanks.

+from tcms.core.ajax import strip_parameters
+
+
+class Test_strip_parameters(unittest.TestCase):

This comment has been minimized.

@tkdchen

tkdchen May 19, 2017

Member

Please do not use _ in class name. In this case, for example, TestStripParameters is ok.

@tkdchen

tkdchen May 19, 2017

Member

Please do not use _ in class name. In this case, for example, TestStripParameters is ok.

@tkdchen tkdchen referenced this pull request May 26, 2017

Merged

Replace request core ajax #206

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen May 26, 2017

Member

7687d2c is merged.
8ca779b is merged by removing unrelated code according to my comment.

Member

tkdchen commented May 26, 2017

7687d2c is merged.
8ca779b is merged by removing unrelated code according to my comment.

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