Fix/document/get work items

AlthausKonstantin commented 1 year ago

Hi all,

I experience a bug with the method Document.get_work_items(). The bug is described in https://github.com/RedHatQE/pylero/issues/136. This is my suggested fix for the problem.

I am looking forward to your input :)

leelavg commented 1 year ago

Thx a lot for the PR, this is slipping my todo for last couple of days
I'll get by Tuesday EOD if you don't mind

AlthausKonstantin commented 1 year ago

Okay, here are some of my thoughts on the topic

Source of the issue

We get a key error in _convert_obj_fields_to_polarion() because calling this method on _WorkItem leads to looking up custom fields in the attribute _WorkItem._cls_suds_map. This attribute does not have the custom fields of the workitems we want to get from the document (with the method get_work_items() of the class Document) because the custom fields of a specific work item type are defined during creation of the types in the module pylero.work_item. In particular, they are not saved in the class _WorkItem but only in the derived classes.

Special Case get_work_items() As far as I can see it the method _convert_obj_fields_to_polarion() is always called in classmethods. Only in method get_work_items() of the class Document we call it in a method. And crucially, not as a method of the class Document but as a method of the class _Workitem. But as explained above, before calling we did not make sure the attribute _WorkItem._cls_suds_map is set up correctly.

Proposed fix

We need to iterate over all work item types created upon loading the module pylero.work_item to get all possible custom fields of a document's workitems in get_work_items(). This has the following implications on your earlier comments:

https://github.com/RedHatQE/pylero/pull/137#discussion_r1182054547: We iterate over all work item types but always pass all fields. Each type should apply _convert_obj_fields_to_polarion() only to its own custom fields, the rest must be explicitly ignored to avoid KeyErrors.
We need to get all dynamically created subclasses of _SpecificWorkItem in the module pylero.Document. If importing _SpecificWorkItem is not an option: What do you think about getting them in a new classmethod of _WorkItem?
- I do not think it is apppropriate to put these changes in BasePolarion. Rather I think it should be the job of each child-class of BasePolarion to make sure that calling the interhited method _convert_obj_fields_to_polarion() works. Therefore, if we call _WorkItem._convert_obj_fields_to_polarion(), we have to make sure beforehand that everything works (e.g. as above by exchanging _WorkItem by the dynamically created classes)

Alternative fix

In order to avoid loading and iterating over all dynamically created work item types in the module pylero.Document, we could also store the union of all custom fields in attribute _WorkItem._cls_suds_map. I do not know if this causes interference.

leelavg commented 1 year ago

@simzacks could you pls pitch in here when you get time?
@AlthausKonstantin provided very good analysis above and before taking in the proposed fix, I want to know if there's any reason that you remember wrt this statement Only in method get_work_items() of the class Document we call it in a method.

leelavg commented 1 year ago

btw @AlthausKonstantin #136 speaks of the issue while using Tasks and #137 adds a fix to Document, I don't see any explicit connection b/n these two in my usage.
did your code flow w/ Task hit Document obj as well?

simzacks commented 1 year ago

Probably the easiest fix would by in base_polarion.py function _convert_obj_fields_to_polarion line 315. If the field starts with customFields to add it to the p_fields list as is and not do any verification/processing.

@leelavg - here is the background information: The _cls_suds_map has all of the standard fields of every object. Each WorkItem type has its own custom fields. Pylero generally relates to custom fields as native fields, so it handles all the abstraction behind the scenes. Document is a unique case, because we have the work_item_id, but not the work item type. So we can't know what custom fields exist. Because of that, we use the _WorkItem object in the Document functions, and custom fields are related to differently as documented: "For custom fields you can specify which fields you want to be filled using following syntax: customFields.CUSTOM_FIELD_ID (e.g. customFields.risk)." The problem here is that document.py (line 457) passes in the customFields.Active and the function handles that poorly.

In general, if you try to use Task (or other work item), as described in the bug report, you would just add "Active" in the fields, and not the customFields.Active : same_task = Task(project_id="PHP2020", work_item_id="PHP2020-42443", fields=["Active"]) print(same_task.Active)

AlthausKonstantin commented 1 year ago

@leelavg on the topic of https://github.com/RedHatQE/pylero/pull/137#issuecomment-1531314519:

TBH, the original bug I ran into was the one of Document.get_work_items(). As I opend the issue, I indeed thought that the constructor of the work item is the main problem. Now I am not so sure.

Current state of things Adding the explicit check if x in cls._cls_suds_map.keys() in _convert_obj_fields_to_polarion seems to get rid of the issue in the MWE provided. That is, the following works

from pylero.work_item import Task
task =  Task(project_id="PHP2020",
               work_item_id="PHP2020-42443",
               fields=["Active"])
print(same_task.active) # prints correct value of the custom field `active`.

I will investigate this further and come back to you ;)

AlthausKonstantin commented 1 year ago

@simzacks on the topic of https://github.com/RedHatQE/pylero/pull/137#issuecomment-1531417303:

Haha, passing all fields of the type 'customFields.*' through _convert_obj_fields_to_polarion without further checking was also my first idea - see the workaround https://github.com/RedHatQE/pylero/issues/136#issuecomment-1525734368. Is this preferable?

leelavg commented 1 year ago

Now I am not so sure.

np, however, I appreciate the effort you put in explaining your observations!

fields=["Active"]

exactly! this is how the API should be and the one I suggested after seeing your bug report

I will investigate this further and come back to you ;)

pls consider https://github.com/RedHatQE/pylero/pull/137#issuecomment-1531417303 as a starting point, as @simzacks is kinda BDFL (in a good sense) of this project 😅

simzacks commented 1 year ago

I'm really not BDFL - more like the original developer and consultant, now that I walked away from the project. Also, I don't have a Polarion to work on, so this is completely by memory with no ability to check.

I believe that it is case sensitive. so "Active" and "active" will be different.

When the specific work item class is instantiated, the custom fields are added to the _cls_suds_map with an attribute of is_custom = True. There shouldn't be a need for the explicit check: if x in cls._cls_suds_map.keys()

simzacks commented 1 year ago

To verify this, you can try: t = Task() # without passing in fields dir(t) # and see all the attributes it contains t._cls_suds_map # to see what exactly is included in the suds map

AlthausKonstantin commented 1 year ago

Okay, thank you so much for your input. I am now able to untangle the two issues at play here.

Extracting custom fields with the WorkItem constructor

This is the original issue https://github.com/RedHatQE/pylero/issues/136. This issue has nothing to do with this PR - haha. After stepping through the code, I see that everything works almost as expected with the original code: To get the normal field "Title" and the custom field "Active", do this:

from pylero.work_item import Task
task =  Task(project_id="PHP2020",
               work_item_id="PHP2020-42443",
               fields=["title", "active"])
print(same_task.active) # prints correct value of the custom field `active`.

What confused me about this is, that if you get a list of the defined custom fields, the name is not in lower case:

# continues last snippet
print(task.get_custom_field_keys()) # this list contains the entry "Active"

Same is true for the method get_custom_field():

# continues last snippet
task.get_custom_field("Active") # gets correct value
task.get_custom_field("active") # returns `None`

In case you wonder, how the strings are transformed from one into the other: This happens in pylero.work_items 1436-1443: In partuclar everything becomes lower case and whitespaces become underscores.

Extract custom fields of the workitems in a document

This is what this PR should be about. Here we have still the issue I pointed out above in https://github.com/RedHatQE/pylero/pull/137#issuecomment-1531241648.

Suggestions

I explain the findings about the lower case transformation I talked about above in the original issue for later generations that might end up there.
I develop the PR to fix the get_work_items() method.

AlthausKonstantin commented 1 year ago

@leelavg, @simzacks please advise on the new changes :)

AlthausKonstantin commented 1 year ago

For document class, we get only work item id but not the type

Could you clarify, what you mean? id is a standard field and can be requested with the fields kwarg.

When custom fields are asked on this class, it incorrectly assumes the cache was primed and calls a func which maps polarion items & python objs

This fix makes sure the cache is primed before making the transformation

yes and yes.

@AlthausKonstantin only thing I'm a little unsure is, whether get_defined_work_item_types returns all the work items or only the specific work item that the work item id corresponds to 🤔

But we do not provide a specific workitem id and call the method on the generic class itself. Or do I miss something?

leelavg commented 1 year ago

Could you clarify, what you mean?

I meant, as an input in the document class we don't get specific work item type but only the id of work item, as a result we can't get custom fields, same as what's mentioned in https://github.com/RedHatQE/pylero/pull/137#issuecomment-1531417303 for Document.

leelavg commented 1 year ago

@waynesun09, I think this PR is good to go, ptal.

AlthausKonstantin commented 1 year ago

@leelavg on https://github.com/RedHatQE/pylero/pull/137#issuecomment-1538184502: Yes, @simzacks's comment remains true and is the reason why we have to iterate over all types and match the respective custom fields.

waynesun09 commented 1 year ago

@AlthausKonstantin @leelavg @simzacks Thanks for the fix and analysis of the issue, took me a while to understand the details.

AlthausKonstantin commented 1 year ago

sweet, thank you all for your help. Sorry for the confusion in the thread, it took me some time to get my head around it..

RedHatQE / pylero