Don't use deprecated request.REQUEST #156

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@atodorov
Contributor

atodorov commented Feb 23, 2017

This PR removes deprecated request.REQUEST to allow for easier upgrade of Django in the future.

NOTES:

  • There are probably other places where this is used but I haven't touched them yet.
  • All tests report PASS but looking at coverage I think all of the lines which this PR changes are not covered by tests. I will see if I can write some tests but maybe not immediately.
@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Feb 25, 2017

Member

Please sign-off your commit, and the commit message should tell more information about where request.REQUESTs are fixed, because this patch only modifies part of the code, not all.

There are lots of request.REQUEST throughout the code. I prefer to change those code along with unit tests.

Member

tkdchen commented Feb 25, 2017

Please sign-off your commit, and the commit message should tell more information about where request.REQUESTs are fixed, because this patch only modifies part of the code, not all.

There are lots of request.REQUEST throughout the code. I prefer to change those code along with unit tests.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Feb 26, 2017

Contributor

There are lots of request.REQUEST throughout the code. I prefer to change those code along with unit tests.

I totally support that. As explained in the updated commit log I have created these some time ago before the test suite was working. Then I realized how many of these changed are necessary and stopped but decided to send this PR so my work doesn't get lost.

Contributor

atodorov commented Feb 26, 2017

There are lots of request.REQUEST throughout the code. I prefer to change those code along with unit tests.

I totally support that. As explained in the updated commit log I have created these some time ago before the test suite was working. Then I realized how many of these changed are necessary and stopped but decided to send this PR so my work doesn't get lost.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 23, 2017

Coverage Status

Coverage remained the same at 45.402% when pulling c886668 on MrSenko:remove_django19_deprecations into 980b07b on Nitrate:develop.

Coverage Status

Coverage remained the same at 45.402% when pulling c886668 on MrSenko:remove_django19_deprecations into 980b07b on Nitrate:develop.

@tkdchen tkdchen added this to the 4.0 milestone Apr 4, 2017

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 9, 2017

Member

Relative to #110

Member

tkdchen commented Apr 9, 2017

Relative to #110

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 9, 2017

Member

So far, I found two issues,

  • Cannot add tag to selected cases in Cases tab
  • Cannot add a case to plan
Member

tkdchen commented Apr 9, 2017

So far, I found two issues,

  • Cannot add tag to selected cases in Cases tab
  • Cannot add a case to plan
tcms/testcases/views.py
@@ -67,7 +67,7 @@ def plan_from_request_or_none(request, pk_enough=False):
Arguments:
- pk_enough: a choice for invoker to determine whether the ID is enough.
'''
- tp_id = request.REQUEST.get("from_plan")
+ tp_id = request.POST.get("from_plan")

This comment has been minimized.

@tkdchen

tkdchen Apr 9, 2017

Member

plan_from_request_or_none is used in both POST and GET request.

@tkdchen

tkdchen Apr 9, 2017

Member

plan_from_request_or_none is used in both POST and GET request.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Apr 11, 2017

Contributor

@tkdchen if you can merge #171 so we can have good tests on various databases I will go back to this one and rework it to add more tests before making changes b/c it is obvious I have broken something.

Contributor

atodorov commented Apr 11, 2017

@tkdchen if you can merge #171 so we can have good tests on various databases I will go back to this one and rework it to add more tests before making changes b/c it is obvious I have broken something.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 16, 2017

Member

The testing matrix in #171 does not block anything currently IMO. Please fix above two issues found in this PR so that I can merge this PR.

Member

tkdchen commented Apr 16, 2017

The testing matrix in #171 does not block anything currently IMO. Please fix above two issues found in this PR so that I can merge this PR.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Apr 16, 2017

Contributor

So far, I found two issues,
Cannot add tag to selected cases in Cases tab

Fixed. Removal of tag is issue #174 which is fixed in #177

Cannot add a case to plan

I can't reproduce. I tried:

  1. Open TestPlan, cases tab
  2. Add case from other plans
  3. search by case id & add ==> works

then open test case -> Plans tab, add plan by ID - also works. Can you test and tell me how to reproduce?

Contributor

atodorov commented Apr 16, 2017

So far, I found two issues,
Cannot add tag to selected cases in Cases tab

Fixed. Removal of tag is issue #174 which is fixed in #177

Cannot add a case to plan

I can't reproduce. I tried:

  1. Open TestPlan, cases tab
  2. Add case from other plans
  3. search by case id & add ==> works

then open test case -> Plans tab, add plan by ID - also works. Can you test and tell me how to reproduce?

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 16, 2017

Coverage Status

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

Coverage Status

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

tcms/testcases/views.py
@@ -468,7 +468,7 @@ def get_selected_testcases(request):
Arguments:
- request: REQUEST object.
'''
- REQ = request.REQUEST
+ REQ = request.POST

This comment has been minimized.

@tkdchen

tkdchen Apr 18, 2017

Member
REQ = request.POST or request.GET

This change fixes Cannot add tag to selected cases in Cases tab.

@tkdchen

tkdchen Apr 18, 2017

Member
REQ = request.POST or request.GET

This change fixes Cannot add tag to selected cases in Cases tab.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 18, 2017

Member

f956f83 includes too many other changes that are not relative to the issue found during review. And, only one line code change fixes the first issue, please see my comment above. Just update bf5041a is enough, no need of f956f83.

I tried to reproduce issue Cannot add a case to plan, but also cannot reproduce it again in both "Write new case" and "Add cases from other plans".

Member

tkdchen commented Apr 18, 2017

f956f83 includes too many other changes that are not relative to the issue found during review. And, only one line code change fixes the first issue, please see my comment above. Just update bf5041a is enough, no need of f956f83.

I tried to reproduce issue Cannot add a case to plan, but also cannot reproduce it again in both "Write new case" and "Add cases from other plans".

Remove some occurences of deprecated request.REQUEST
removed from

- tcms/core/ajax.py
- tcms/core/context_processors.py
- tcms/testcases/views.py

there are more to remove but first we need adequate tests. The
changes here were crated before the test suite was working and
are committed so they don't get lost until I have more time to
work on this particular item.

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

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Apr 19, 2017

Contributor

Updated. I will open another PR for the fixes from f956f83.

Contributor

atodorov commented Apr 19, 2017

Updated. I will open another PR for the fixes from f956f83.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 19, 2017

Coverage Status

Coverage remained the same at 45.366% when pulling 28753d7 on MrSenko:remove_django19_deprecations into a1c47ec on Nitrate:develop.

Coverage Status

Coverage remained the same at 45.366% when pulling 28753d7 on MrSenko:remove_django19_deprecations into a1c47ec on Nitrate:develop.

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

Updates to previous merge of PR #156.
Remove some occurences of deprecated request.REQUEST

removed from

- tcms/core/ajax.py
- tcms/core/context_processors.py
- tcms/testcases/views.py

there are more to remove but first we need adequate tests. The
changes here were crated before the test suite was working and
are committed so they don't get lost until I have more time to
work on this particular item.

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

Conflicts:
	tcms/testcases/views.py
@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Apr 25, 2017

Member

Merged. Thanks.

Member

tkdchen commented Apr 25, 2017

Merged. Thanks.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Apr 25, 2017

Contributor

Merged manually, closing.

@tkdchen FYI you can use the merge button to Rebase & merge which will place the PR commits on top of the develop branch.

Contributor

atodorov commented Apr 25, 2017

Merged manually, closing.

@tkdchen FYI you can use the merge button to Rebase & merge which will place the PR commits on top of the develop branch.

@atodorov atodorov closed this Apr 25, 2017

@atodorov atodorov deleted the MrSenko:remove_django19_deprecations branch Apr 25, 2017

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