it is possible to assign ticket to closed milestone

[nenad@…]

With multi-projects enabled, and milestones associated to projects, it is possible to set ticket's milestone to the closed one.

[anonymous]

Why should we restrict that? Have you experienced a special problem?

[nenad@…]

Well, with ongoing projects that have plenty of milestones set one week apart, once milestone is marked as "completed" it is pointless to allow new tickets to set milestone to any of closed/completed milestones - the list of milestones that appears contains all of them, completed ones are not even marked in any way.

If I'm not mistaken, default Trac beahviour is to present only active milestones in drop-down list.

[Ryan J Ollos]

Replying to nenad@…:

If I'm not mistaken, default Trac beahviour is to present only active milestones in drop-down list.

That's correct. However, users with TICKET_ADMIN will see all the milestones grouped by status: Closed, Open (by due date), Open (no due date).

[nenad@…]

Replying to rjollos:

Replying to nenad@…:

If I'm not mistaken, default Trac beahviour is to present only active milestones in drop-down list.

That's correct. However, users with TICKET_ADMIN will see all the milestones grouped by status: Closed, Open (by due date), Open (no due date).

With "SimpleMultiProjectPlugin" plugin activated and with newly created user that does not have "TICKET_ADMIN" permission assigned, but has access to the project itself (definied within Admin / Manage Projects / Project / Restricted To Users), it is still possible to set ticket milestone to the completed one.

User has been granted these permissions:

User  Action               
---------------------------
test  EMAIL_VIEW           
test  FILE_VIEW            
test  LOG_VIEW             
test  MILESTONE_VIEW       
test  PROJECT_SETTINGS_VIEW
test  REPORT_SQL_VIEW      
test  REPORT_VIEW          
test  ROADMAP_VIEW         
test  SEARCH_VIEW          
test  TAGS_MODIFY          
test  TAGS_VIEW            
test  TICKETLOG_VIEW       
test  TICKET_ADD_HOURS     
test  TICKET_APPEND        
test  TICKET_CHGPROP       
test  TICKET_CREATE        
test  TICKET_EDIT_CC       
test  TICKET_EDIT_COMMENT  
test  TICKET_MODIFY        
test  TICKET_VIEW          
test  TICKET_VIEW_HOURS    
test  TIMELINE_VIEW        
test  WIKI_CREATE          
test  WIKI_MODIFY          
test  WIKI_VIEW            
test  WORK_LOG
test  XML_RPC

[igor.paunov@…]

I have very intensive development and many short milestones (up to two or tree weeks). For one year we have many milestones and is become annoying to pick correct one when creating a ticket.

[falkb]

Is it OK for you to hide closed milestones from the new ticket form, only? The same ticket form is used also for ticket changes but in such case I'd keep the possibility to fix a milestone or version to an old (closed) one, for cleaning up things so to speak. What do you think?

[nenad@…]

I think that it would be the best if owner of TICKET_ADMIN permission (or, even better, some other/new permission) would have possibility to change milestone to the closed one, but default (for anyone without that permission) to be not to show closed milestones neither in "create new ticket" nor in "modify existing ticket" cases.

Reasoning for this is:

• when there is a need to change milestone on a per-ticket basis (no 'batch' changes), appearence of closed milestones will again make it hard to do.
• generally speaking, I believe that it will be confusing for users that on "create new" there won't be closed milestones shown, where the same user that goes to "modify ticket" can see them.

[falkb]

Yes, that sounds reasonable. I'm going to fix it as you suggested. Let's see if I can find the time this week.

[nenad@…]

Great! :) Thank you in advance!

[falkb]

Could you please review the following patch? Take care on the set condition logic:

model.py

@@ -731 +731 @@
             def execute_sql_statement(db):
                 cursor = db.cursor()
                 cursor.execute(query, [new_component, old_component])
+
+    def is_milestone_completed(self, milestone_name):
+        db = self.env.get_db_cnx()
+        cursor = db.cursor()
+        query = """SELECT completed FROM milestone WHERE name=%s;"""
+        cursor.execute(query, [milestone_name])
+        msCompletedDate = cursor.fetchone()
+        if msCompletedDate and msCompletedDate[0]>0:
+            return True
+        return False

ticket.py

@@ -42 +42 @@
         
     def post_process_request(self, req, template, data, content_type):
         if data and req.path_info.startswith('/ticket'):
+            is_newticket = False
             ticket = data.get('ticket', None)
             if ticket:
                 project_name = self.__SmpModel.get_ticket_project(ticket.id)
                 if project_name and project_name[0]:
                     self.__SmpModel.check_project_permission(req, project_name[0])
+        else:
+            is_newticket = True
 
         if template == 'ticket.html':
             all_components = model.Component.select(self.env)
@@ -80 +83 @@
             add_script_data(req, def_version)
             add_script_data(req, project_versions)
             
-            self._add_milestones_maps(req, data['ticket'])
+            self._add_milestones_maps(req, data['ticket'], is_newticket)
 
         return template, data, content_type
 
@@ -97 +100 @@
 
         return stream
 
-    def _add_milestones_maps(self, req, ticket_data):
+    def _add_milestones_maps(self, req, ticket_data, is_newticket):
 
         milestone = ticket_data.get_value_or_default('milestone')
         project   = ticket_data.get_value_or_default('project')
@@ -108 +111 @@
         milestonesForProject = {}
         milestonesForProject[""] = { "Please, select a project!": "" }
 
+        have_ticketadmin = req.perm.has_permission('TICKET_ADMIN')
+
         for project in sorted(allProjects, key=itemgetter(1)):
             milestones = self.__SmpModel.get_milestones_of_project(project[1])
             milestonesForProject[project[1]] = { "": "" }
             for milestone in sorted(milestones, key=itemgetter(0)):
-                milestonesForProject[project[1]][milestone[0]] = milestone[0]
+                ms = milestone[0]
+                is_completed   = self.__SmpModel.is_milestone_completed(ms)
+                hide_milestone = (is_newticket and is_completed) or (not is_newticket and not have_ticketadmin)
+                if not hide_milestone:
+                    milestonesForProject[project[1]][ms] = ms
 
         smp_milestonesForProject = { 'smp_milestonesForProject' : milestonesForProject }
         smp_initialProjectMilestone = { 'smp_initialProjectMilestone' : initialProjectMilestone }

[nenad@…]

It works - TICKET_ADMIN owner can set milestone to closed when modifying existing ticket; new ticket creation offers only open milestones.

[falkb]

Great! I'm still thinking about the permission, I mean if TICKET_ADMIN is the right one. Stay tuned... Hints very welcome (as always). :-)

[nenad@…]

Acutally, it may be better to use other permission, as TICKET_ADMIN is already over-used; for example, TICKET_ADMIN is required to make possible to change ticket milestone, and with it you can see all closed ones, too ... In fact, in the first place problem is that TICKET_ADMIN is required for milestone change.

[falkb]

Actually, it's programmatically quite easy for me to introduce a new plugin permission. See admin.py around line 40:

    def get_permission_actions(self):
        """ Permissions supported by the plugin. """
        return ['PROJECT_ADMIN', 'PROJECT_SETTINGS_VIEW']

One just adds another permission here and it could be used in my patch given in comment:10, though I'm not sure if that's the right way to go.

[nenad@…]

Yes, I understand you, it would make plugin to behave differently than plain Trac.

[falkb]

Are you sure you need TICKET_ADMIN for milestone changes since there is ​T:/TracPermissions#Roadmap with all those MILESTONE_... permissions?

[falkb]

I also see TICKET_CHGPROP in ​T:/TracPermissions#TicketSystem which seems to be the smallest one allowing changes to the ticket fields.

[falkb]

ah forget my comment:17 since we're talking about ticket fields and not managing milestones :)

[nenad@…]

Replying to falkb:

Are you sure you need TICKET_ADMIN for milestone changes since there is ​T:/TracPermissions#Roadmap with all those MILESTONE_... permissions?

...

I also see TICKET_CHGPROP in ​​T:/TracPermissions#TicketSystem which seems to be the smallest one allowing changes to the ticket fields.

Problem is within this patch itself - TICKET_CHGPROP does the job without this patch, with it TICKET_ADMIN is needed. Problem is that with this

hide_milestone = (is_newticket and is_completed) or (not is_newticket and not have_ticketadmin)

we don't take into account case when ticket is not new, milestone is open, and user has TICKET_CHGPROP permission.

[falkb]

I'm confused and cannot follow you anymore. Can you fix this pseudo code to let it correctly set the variable 'hide_milestone'?

if is_newticket:
    hide_milestone = is_completed
else:
    nenad_case = (not completed and req.perm.has_permission('TICKET_CHGPROP'))
    hide_milestone = not have_ticketadmin or nenad_case

[nenad@…]

Sorry for the confusion :) I believe code may look like this:

if is_newticket:
    hide_milestone = is_completed
else:
    hide_milestone = not (have_ticketadmin or (not is_completed and req.perm.has_permission('TICKET_CHGPROP')))

[falkb]

Thanks, now it's clear. :-)

[falkb]

In 14463:

fixes #12201 (thanks to nenad):

• hide closed milestones from newticket form
• hide closed milestones from change ticket form under some circumstances (e.g. missing permissions)