Form fields removal and form merges

[asankov]

As discussed in #677

delete NewPlanForm.tag and EditPlanForm.author since they are not used

DONE

delete classes that are not used, but are only inherited once - XMLRPCNewPlanForm,XMLRPCEditPlanForm
copy the fields from these intermediate classes inside the ones found under xmlrpc/forms.py
remove the intermediate classes

DONE - The forms were removed and their field were put into their respective anchestors

under xmlrpc/ better organize the forms into smaller groups, e.g. xmrpc/api/forms/testplan.py or similar and update the imports in the API modules.

DONE

investigate all of the above form classes for unused fields
this is part of the change above (separate commits for everything) and you should be careful for:
Some of these fields are not properly covered by tests and my approach in the rare event I had removed some was to inspect every field one by one, inspect what tests are there (and what is validated inside of them) and then manually try both the UI and the API to make sure it all works.
all of these inherited forms redefine class fields and from what I've seen in practice many times there are duplicates. The only reason for redefining some field is if you want to make it required/not required or change some defaults, etc. which will be different between webUI and RPC. There's no other reason for these redefined fields to exist.

There are predefined fields, but there is a reason for that - in the RPC* forms all of the fields are not required, while in the regular forms all of them are. It looks like we can't remove any more code for these respecitve forms. Also all of the fields in RPC forms are used in their respective controllers, again, nothing we can delete

move all of the RPC* classes to rpc package
Not now, open a separate issue for this. This is a rather big change which needs to be planned and accounted for. I would rather spend the time to refactor these as described above instead of just moving the around.

#681

rename all XMLRPC* to RPC*
This is renaming only and making sure everything matches. We can rename the module itself easily but I prefer to update the classes and functions one-by-one to avoid massive changes.

However the naming is only due to historical reasons and you are welcome to implement a pylint checker that will warn us about bogus names (and also drive the rename/refactor effort). Again new issue so we can plan this.

#682

[codecov-io]

Codecov Report

Merging #683 into master will decrease coverage by 0.04%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master     #683      +/-   ##
==========================================
- Coverage   64.62%   64.58%   -0.05%     
==========================================
  Files         109      110       +1     
  Lines        6159     6152       -7     
  Branches      820      820              
==========================================
- Hits         3980     3973       -7     
- Misses       1920     1922       +2     
+ Partials      259      257       -2

Impacted Files	Coverage Δ
tcms/xmlrpc/forms.py	80% <ø> (-5.72%)	⬇️
tcms/xmlrpc/api/testplan.py	57.5% <100%> (ø)	⬆️
tcms/testplans/forms.py	83.56% <100%> (-2.33%)	⬇️
tcms/xmlrpc/api/forms/testplan.py	100% <100%> (ø)
tcms/testplans/views.py	63.43% <50%> (-0.14%)	⬇️
tcms/report/data.py	28.08% <0%> (ø)	⬆️
tcms/core/history.py	90.69% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f4eb11...121c9d1. Read the comment docs.

[coveralls]

Pull Request Test Coverage Report for Build 2224

• 18 of 20 (90.0%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.03%) to 64.415%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tcms/testplans/views.py	1	2	50.0%
tcms/xmlrpc/api/testplan.py	2	3	66.67%

Totals
Change from base Build 2222:	-0.03%
Covered Lines:	4226
Relevant Lines:	6146

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 2224

• 18 of 20 (90.0%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.03%) to 64.415%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tcms/testplans/views.py	1	2	50.0%
tcms/xmlrpc/api/testplan.py	2	3	66.67%

Totals
Change from base Build 2222:	-0.03%
Covered Lines:	4226
Relevant Lines:	6146

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 2224

• 18 of 20 (90.0%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.03%) to 64.415%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tcms/testplans/views.py	1	2	50.0%
tcms/xmlrpc/api/testplan.py	2	3	66.67%

Totals
Change from base Build 2222:	-0.03%
Covered Lines:	4226
Relevant Lines:	6146

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 2224

• 18 of 20 (90.0%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.03%) to 64.415%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tcms/testplans/views.py	1	2	50.0%
tcms/xmlrpc/api/testplan.py	2	3	66.67%

Totals
Change from base Build 2222:	-0.03%
Covered Lines:	4226
Relevant Lines:	6146

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 2273

• 16 of 17 (94.12%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.03%) to 64.417%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tcms/testplans/views.py	1	2	50.0%

Totals
Change from base Build 2271:	-0.03%
Covered Lines:	4230
Relevant Lines:	6152

---

💛 - Coveralls

[atodorov]

There are predefined fields, but there is a reason for that - in the RPC* forms all of the fields are not required, while in the regular forms all of them are. It looks like we can't remove any more code for these respecitve forms. Also all of the fields in RPC forms are used in their respective controllers, again, nothing we can delete

Which classes/fields are different ?

[atodorov]

This should be something like tcms.xmlrpc.api.forms.testplan import EditPlanForm, NewPlanForm.

If you re-organize where the classes are defined and make the modules smaller, then you can keep the existing class names (resulting in smaller diff below) because now you know where the form classes are coming from. If the RPC app is self contained (but dependent on other base classes) then we don't really need to overload all class names with meaning, like it is now, and later have to change the names because the meaning has changed.

[asankov]

I have done that reorganization. IMO the next steps here should be to do the same for TestCase RPC forms - the mess there is almost the same as the one we are fixing now

[asankov]

btw for this I had to squash the two commits:

• Delete XMLRPCEditPlanForm and XMLRPCNewPlanForm
• Introduce RPCPlanForm as base form

because otherwise we had the problem with duplicate names and imports you have exposed above.

[asankov]

As for your first question - different fields in RPC and regular forms, here they are:
RPC:
name - required=False
type - required=False
product - required=False,
product_version - required=False

regular:
name - required=True
type - required=True
product - required=True
product_version - required=True

there are other fields in regular form which are not required and they are not predefined in the RPC form

[atodorov]

tcms/xmlrpc/api/forms/testplan/__init__.py - you don't need this so there's no point in that file appearing in one commit (and later be removed in another). Squash the changes so this file never appears in the history.

[atodorov]

regular:
name - required=True
type - required=True
product - required=True
product_version - required=True

These follow the definition of the TestPlan model and are fine.

RPC:
name - required=False
type - required=False
product - required=False,
product_version - required=False

when creating a new TP via the API all of these fields, including is_active and text are passed directly to TestPlan.objects.create so if one of these values is missing the creation will fail with exception b/c the database will trigger a non-null constraint! (null != empty string).

The only place where form fields may not be required is the TestPlan.update API which allows the user to update only selected fields!

[asankov]

The form that I got those fields from was: EditPlanForm. So indeed this form is used for update and the fields are optional, because the user may want to update only one of the

As for NewPlanForm (regular) and NewPlanForm (RPC) - there are only two differences:

• the text field which is SimpleMDE for regular and CharField for RPC
• the is_active fileld which is BooleanField for regular and BooleanField(widget=XMLRPCCheckboxInput) for RPC

[asankov]

This is ready for review