Git Merge Request #264: [#8209] Personal Dashboard - Create Tickets section (merged)

Merging...

Merged

Something went wrong. Please, merge manually

Checking if merge is possible...

Something went wrong. Please, merge manually

Deshani wants to merge 0 commits from /u/deshani/allura-personal-dashboard/ to master, 2018-07-06

Commit Date  

Discussion

  • Dave Brondsema

    Dave Brondsema - 2018-07-03

    Hi Deshani,

    Looking pretty good, it seems to work well. Since this is doing some complex and interesting things, there are some issues I've found. And ideas for improvement:

    When there are no tickets and it says "No tickets to display", I think we could omit the table so it doesn't show the column headers and that'd look a little nicer.

    Sorting by last updated would be good to do I think.

    In allura/ext/personal_dashboard/templates/sections/tickets.html is count=count-1 right? I am getting the wrong number of pages sometimes. I think the -1 is not needed.

    In ForgeTracker/forgetracker/model/ticket.py where you've changed the lines involving
    query = cls.query.find(dict(ticket_num={'$in': ticket_numbers})) there is an issue. When multiple ticket tracker tools exist (on different projects, or even the same project), ticket #1 or #2 etc will exist in many trackers. Since that code now does a lookup only by ticket number, it won't get the right tickets. I'd suggest changing the fl='ticket_num_i' to fl=id since the id field contains the completely unique ObjectId value. Then parse it out something like ticket_ids = [ObjectId(match['id'].split('#')[1] for match in matches.docs] and query it like _id={'$in': ticket_ids}.

    These lines are very similar and complex, so it'd be nice not to have them duplicated:

    if (app_config is not None) and(security.has_access(ticket_for_num[tn], 'read', user, app_config.project.root_project) and
        (show_deleted or ticket_for_num[tn].deleted == False)):
    tickets.append(ticket_for_num[tn])
elif (app_config is None) and(security.has_access(ticket_for_num[tn], 'read', user) and
        (show_deleted or ticket_for_num[tn].deleted == False)):
    tickets.append(ticket_for_num[tn])

    I think this would work as a single clause instead:

    if (security.has_access(ticket_for_num[tn], 'read', user,
                        app_config.project.root_project if app_config else None) and
        (show_deleted or ticket_for_num[tn].deleted == False)):
    tickets.append(ticket_for_num[tn])

    It might be good to filter out tickets that are closed, so people don't have to see old tickets. However, since trackers are customizable there are an endless number of statuses that mean closed (e.g. closed, invalid, wont-fix, duplicate, etc). So I can't think of a good way to add a solr search clause to handle that. Possibly could do it in python code (not solr) by checking each ticket.is_closed property. Do you want to make a separate ticket for that and consider it if time allows later? Doesn't need to hold up this merge request.

    Thanks!

     

    • Deshani

      Deshani - 2018-07-04

      Hi Dave,

      Thanks for reviewing. I've added required modifications and updated the merge request. Please let me know if further modifications required.

      I will do a seperate ticket for the last suggestion

      Thank you!

       

  • Dave Brondsema

    Dave Brondsema - 2018-07-05

    I got the following error when viewing the dashboard. The split('#')[1] needs to be within the ObjectId() constructor call.

    File '/allura/Allura/allura/ext/personal_dashboard/templates/dashboard_index.html', line 47 in block "content_base"
  {{ section.display() }}
File '/allura/Allura/allura/lib/widgets/user_profile.py', line 126 in display
  'auth': AuthenticationProvider.get(request),
File '/allura/Allura/allura/ext/personal_dashboard/dashboard_main.py', line 99 in prepare_context
  result = self.query_tickets(page, limit)
File '/allura/Allura/allura/ext/personal_dashboard/dashboard_main.py', line 71 in query_tickets
  result = Ticket.paged_search(None, self.user, q, limit=limit, page=page, sort=sort)
File '/allura/ForgeTracker/forgetracker/model/ticket.py', line 1256 in paged_search
  ticket_matches = [ObjectId(match['id']).split('#')[1] for match in matches.docs]
File '/allura-data/virtualenv/local/lib/python2.7/site-packages/bson/objectid.py', line 121 in __init__
  self.__validate(oid)
File '/allura-data/virtualenv/local/lib/python2.7/site-packages/bson/objectid.py', line 228 in __validate
  _raise_invalid_id(oid)
File '/allura-data/virtualenv/local/lib/python2.7/site-packages/bson/objectid.py', line 61 in _raise_invalid_id
  oid, binary_type.__name__))
InvalidId: u'forgetracker/model/ticket/Ticket#5b3bbd975b1bb7000d9e4ff4' is not a valid ObjectId, it must be a 12-byte input of type 'str' or a 24-character hex string

    And then regarding the changes in paged_search to search & lookup tickets by id instead of number, the "id" approach will work for both the case where there is an app_config and where there isn't, so we can switch the code to use that and be simpler. To make sure this was true, I ended up making the changes, so here they are (also renamed ticket_for_num to ticket_by_id and had it store ids instead of nums) . You can review the changes and commit them:

    diff --git ForgeTracker/forgetracker/model/ticket.py ForgeTracker/forgetracker/model/ticket.py
index d242e9af9..96a25de4e 100644
--- ForgeTracker/forgetracker/model/ticket.py
+++ ForgeTracker/forgetracker/model/ticket.py
@@ -1227,51 +1227,40 @@ def paged_search(cls, app_config, user, q, limit=None, page=0, sort=None, show_d
                 # also query for choices for filter options right away
                 params = kw.copy()
                 params.update(tsearch.FACET_PARAMS)
                 if not show_deleted:
                     params['fq'] = ['deleted_b:False']
-                if app_config is None:
-                    fl = 'id'
-                else:
-                    fl = 'ticket_num_i'
                 matches = search_artifact(
                     cls, q, short_timeout=True,
-                    rows=limit, sort=refined_sort, start=start, fl=fl,
+                    rows=limit, sort=refined_sort, start=start, fl='id',
                     filter=filter, **params)
             else:
                 matches = None
             solr_error = None
         except SearchError as e:
             solr_error = e
             matches = None
         if matches:
             count = matches.hits
-            if app_config is not None:
-                # ticket_matches is in sorted order
-                ticket_matches = [match['ticket_num_i'] for match in matches.docs]
-
-                # but query, unfortunately, returns results in arbitrary order
-                query = cls.query.find(
-                    dict(app_config_id=app_config._id, ticket_num={'$in': ticket_matches}))
-            else:
-                ticket_matches = [ObjectId(match['id']).split('#')[1] for match in matches.docs]
-                query = cls.query.find(
-                    dict(_id={'$in': ticket_matches}))
+            # ticket_matches is in sorted order
+            ticket_matches = [ObjectId(match['id'].split('#')[1]) for match in matches.docs]
+            # but query, unfortunately, returns results in arbitrary order
+            query = cls.query.find(dict(_id={'$in': ticket_matches}))
             # so stick all the results in a dictionary...
-            ticket_for_num = {}
+            ticket_by_id = {}
             for t in query:
-                ticket_for_num[t.ticket_num] = t
-            # and pull them out in the order given by ticket_numbers
+                ticket_by_id[t._id] = t
+            # and pull them out in the order given by ticket_matches
             tickets = []
-            for tn in ticket_matches:
-                if tn in ticket_for_num:
+            for t_id in ticket_matches:
+                if t_id in ticket_by_id:
                     show_deleted = show_deleted and security.has_access(
-                        ticket_for_num[tn], 'delete', user, app_config.project.root_project)
-                    if (security.has_access(ticket_for_num[tn], 'read', user,
+                        ticket_by_id[t_id], 'delete', user, app_config.project.root_project)
+                    if (security.has_access(ticket_by_id[t_id], 'read', user,
                                             app_config.project.root_project if app_config else None) and
-                            (show_deleted or ticket_for_num[tn].deleted == False)):
-                        tickets.append(ticket_for_num[tn])
+                            (show_deleted or ticket_by_id[t_id].deleted == False)):
+                        tickets.append(ticket_by_id[t_id])
                     else:
                         count = count - 1
         return dict(tickets=tickets,
                     count=count, q=q, limit=limit, page=page, sort=sort,
                     filter=filter,
diff --git ForgeTracker/forgetracker/tests/unit/test_root_controller.py ForgeTracker/forgetracker/tests/unit/test_root_controller.py
index ae2f7a156..7a88f726a 100644
--- ForgeTracker/forgetracker/tests/unit/test_root_controller.py
+++ ForgeTracker/forgetracker/tests/unit/test_root_controller.py
@@ -71,7 +71,7 @@ def solr_search_returning_colors_are_wrong_ticket():
     ticket = create_colors_are_wrong_ticket()
     search_artifact = Mock()
     matches = Mock()
-    matches.docs = [dict(ticket_num_i=ticket.ticket_num)]
+    matches.docs = [dict(id=ticket.index_id())]
     matches.facets = {'facet_fields': {}}
     search_artifact.return_value = matches
     return patch('forgetracker.model.ticket.search_artifact', search_artifact)
     

    • Deshani

      Deshani - 2018-07-06

      Really sorry about the error. It didn't came out for me when tested, as I've done mistake while copying to VM.
      Thanks for reviewing. I've done the required modifications and updated the merge request.

       

  • Dave Brondsema

    Dave Brondsema - 2018-07-06
    • Status: open --> merged
     

Log in to post a comment.