UMB-CS682-Team-02 / tracker

0 stars 1 forks source link

Handle group on 2 properties where the properties are a multilink or the value can be unset #65

Closed rouilj closed 6 months ago

rouilj commented 6 months ago

Make sure #62 is complete first as these touch the same code and the refactoring on #62 will make this easier.

The current code in get_data_from_query does something like this (cut down as there are two group variables involved):

  for each issue:
    s1 = get value of group property (step 1)
    s2 = get key/name for group property value (step 2)

There is an issue with this. It assumes step 1 returns a single id that can be resolved by step 2. In reality the value from 1 can be in one of three conditions:

  1. It can be None, None or an empty list (for Boolean, Link and MultiLink respectively)
  2. It can be a single id value (for Link type) or a single 0 or 1 value (for Boolean types).
  3. It can be a list of one or more id values (for MultiLink)

Currently step (2) crashes in every case except condition 2.

So we need to support:

  if (not (s1)): # true for all cases in condition 1
         s2 = "Unset"
  elif (isinstance(s1, str):  # Link
         s2 = get key/name for group property value (step 2)
  elif (isinstance(s1, list)):
        do something useful

do something useful for barchart and piechart is to iterate over each element of the list (s1) and increment the value. See: https://github.com/UMB-CS682-Team-02/tracker/blob/bc7028db83e942391f9ac3dd59ee359da8bd62d6/demo/extensions/chart.py#L94-L113

For the case of two group parameters, this I think iterating over s1 for both group parameters still makes sense as it allows me to find:

  1. the keywords used on tickets grouped by creator (@group0=creator, group1=keyword)
  2. the keywords on tickets by grouped by status (@group0=status, @group1=keyword). If keyword is used to define subsystems we can tell which subsystems are generating the most issues and what status they are in.
  3. what users are nosy on what keywords. So rouilj is nosy on a lot of issues with the security keyword.

Please commit this in two stages to main.

Stage 1:

deal with unset properties

raise an error when a multilink property is used by testing for a list.

Stage 2:

support iterating and counting over multilink properties.

If you want to move stage 2 to a new issue, that is fine.

Pratik0116 commented 6 months ago

I have made some changes to solve this issue. can you check it out before I close this issue and let me know if any modifications required.

rouilj commented 6 months ago

What checkin should I look at?

Also what part of this issue is your commit addressing?

The only two commit I see are: commit 4f3f1ba2ade on test and commit 8cd949dc1531d on main.

Neither seem to handle unpacking a multilink. They seem to handle part of the stage 1 commit where the link is unset by using the "unset" value.

rouilj commented 6 months ago

To followup on our standup discussion.

Setup the test case by creating 4 or 5 keywords. Then assign 2 keywords to issue 1.

This url calling multibarchart should produce a chart:

http://localhost:8080/demo/issue?@action=multibarchart&@columns=title,id,activity,status,assignedto&@sort=activity&@group=priority,keyword&@filter=id&@pagesize=50&@startwith=0&id=1

but it produces an error:

 TypeError: unhashable type: 'list'
[...]
second_prop_value = class2.get(second_prop_id, key2) if second_prop_id else "unset" # Get the name of the second property

At this point, second_prop_id is a list ['1', '2'], not a single id value that can be looked up in the database.

If the two keywords are key1 and key2, and the issues only use priority bug and crash, I expect a chart like:

key1 @                                           @ 
key2 #                                           @ 
                                  @              @ 
                                  @ #            @ #
                                  @ #            @ #
                           --------------------------------
                                   bug         crash

For the case where I have issues like:

issue # key1 key2 comment
1 (bug) + + issue 1 has both keywords
2 (bug) + +
3 (bug) +
4 (crash) + +
5 (crash) +
6 (crash) +
7 (crash) +
8 (crash) +
9 (crash) +

Note that the values of the bars (3, 2, 5, 2) add up to 12 which is not the number of issues. This is fine. I had hoped to have a discussion about how to represent/find options/filter charts like this, but we are out of time.


The chart url shown above results from this query on the index page:

http://locahost:8080/demo/issue?%40sort0=activity&%40sort1=&%40group0=priority&%40group1=keyword&%40columns=title%2Cid%2Cactivity%2Cstatus%2Cassignedto&%40filter=id&id=1&%40pagesize=50&%40startwith=0

and this multibar link on the index page

http://localhost:8080/demo/issue?@template=chart&@charttype=multibarchart&@columns=title,id,activity,status,assignedto&@sort=activity&@group=priority,keyword&@filter=id&@pagesize=50&@startwith=0&id=1

Pratik0116 commented 6 months ago

I think I have solved the multilink issue theoretically because I don't know how did you add those 2 keywords and all. So I have pushed the code in main branch so please have a look at it and tell me if it solves the problem practically too. I'm sharing commit details. Commit - d30409c

rouilj commented 6 months ago

To add keyword use the add keyword link in the left hand pane:

image

then open issue 1 and add 1,2 in the box for keywords.

Note I opened #87 because I see inconsistent data between the index table and the chart.

rouilj commented 6 months ago

Also you left a log of log.append() lines in the commit to main. You probably want to remove them before your final push.

Also I may have been unclear. You created a new branch to do this work on. That's what I wanted. If you want a review, I can pull it from your branch on github (provided you remember to push). It must be on main before you close the ticket however.

Also we had a number of long lived branches that were missing commits from main that made testing from the branched code less useful/harmful. So I test only main code unless asked to look at a particular commit/branch.

Pratik0116 commented 6 months ago

The changes I made were right both theoretically and practically. I have added some keywords and review it myself. New changes does solve this issue and I also have merged changes into main and deleted the test_multilink branch.