Refactor get_data_from_query - change/clarify variable naming and values.

[rouilj]

Why are there two functions: get_data_from_query and get_sample_data_from_query? What makes them different? Under what circumstances are they called?

Rename get_sample_data_from_query to better describe why it exists and what role it plays.

In `get_sample_data_from_query of the current chart.py on main I see code like:

            status_id = cl.get(nodeid, 'status')  # Get the status of the issue
            title = db.getclass('priority').get(title_id, 'name')  # Get the name of the priority
            status = db.getclass('status').get(status_id, 'name')
            data[title][status] += 1  # Increment count for the specific title and status

shouldn't instances of 'status', and 'priority' be the group[0] and group[1] names?

If so than the variable names status and title should be changed to match the contents of these variables.

This seems to be a holdover from the original less flexible single group implementation.

[rouilj]

Pratik, I think your changes that landed on main fixed the issues originally brought up. So I am retargeting this to the get_data_from_query method.

You have:

            # set the key value based on the property type
            key1 = 'name'
            key2 = 'name'
            if first_prop == 'actor' or first_prop == 'assignedto' or first_prop
== 'creator':
                key1='username'
            if second_prop == 'actor' or second_prop == 'assignedto' or second_prop== 'creator':
                key2='username'

this can fail in a number of ways. An admin can modify the schema and add a new link property called "validator" that is a link to the User class. Your code wouldn't catch that.

The best way to do this is to query the database for the key.

key1 = db.getclass(first_prop_type.classname).getkey()

and similarly for key2 will pull the key info from the database and be accurate.

In fact I just ran across this while debugging the URL (referenced in #56):

http://localhost:8080/demo/query?@template=chart&@charttype=barchart&@group0=creator&@group1=private_for

The chart was blank because private_for is a link to the User class, but your list doesn't include that property so it was trying to look up the "name" property.

Also you are reusing the variable names first_prop/second_prop. In one place it's defined as the name of the group properties:

  first_prop = group[0][1]

In the second place their used as the values of the properties data for each issue:

first_prop = db.getclass(first_prop_type.classname).get(first_prop_id, key1)  # Get the name of the first property

Because of this, you are also using the more expensive dereference of the group object:

first_prop_id = cl.get(nodeid, group[0][1])  # Get the first property of the issue

inside the issue loop.

As I mentioned naming is hard. Also first_prop_id is misleading. It can be a list of id's for a multilink. We have to three cases for this value (empty, single id, list). Fixing this is the subject of issue #65. But this work should be done first.

You can refactor these to:

first_group_propname = group[0][1]

[...]

first_prop_value = cl.get(nodeid, first_group_propname)  # Get the first property of the issue

and

first_prop_key_value = db.getclass(first_prop_type.classname).get(first_prop_id, key1)  # Get the name of the first property

and similarly for second_prop.

Also first_prop_type.classname shows up in a couple of places. Consider creating a variable: first_prop_classname and using it in place of first_prop_type.classname. This doesn't change the readability of the code, but it prevents doing an object property dereference inside the loop over all matched issues.

[rouilj]

It's not on main branch.

[Pratik0116]

Sorry John, I commit the code but forget to push it. Now it is on the main branch.