From d86ac11695ea0761be191051b2bfda18df1c9299 Mon Sep 17 00:00:00 2001
From: Akanksha <akanksha_mishra01@yahoo.com>
Date: Oct 06 2018 07:52:37 +0000
Subject: Fix bug in update_milestones


Added a distinct "name" attribute with loop index to milestone names,
for a consistent mapping to the corresponding dates and active status elements

Closes Issue #3716: Changing order of milestones in roadmaps does not change date

---

diff --git a/pagure/templates/settings.html b/pagure/templates/settings.html
index 176458a..558d1bb 100644
--- a/pagure/templates/settings.html
+++ b/pagure/templates/settings.html
@@ -1276,11 +1276,17 @@ $('.extend-form').click(function(e) {
       var _b2 = $(form.find('.milestone_order_bottom'));
       _b2.attr('data-stone', (idx + 1))
 
-      var _d = form.find('input[name=milestone_date_' + idx + ']');
-      $(_d).attr('name', 'milestone_date_' + (idx + 1 ));
+      var _idx = form.find('input[name=milestones]');
+      $(_idx).attr('value', (idx + 1 ));
 
-      var _a = form.find('input[name=active_milestone_' + idx + ']');
-      $(_a).attr('name', 'active_milestone_' + (idx + 1 ));
+      var _n = form.find('input[name=milestone_' + idx + '_name]');
+      $(_n).attr('name', 'milestone_' + (idx + 1 ) + '_name');
+
+      var _d = form.find('input[name=milestone_' + idx + '_date]');
+      $(_d).attr('name', 'milestone_' + (idx + 1 ) + '_date');
+
+      var _a = form.find('input[name=milestone_' + idx + '_active]');
+      $(_a).attr('name', 'milestone_' + (idx + 1 ) + '_active');
       $(_a).prop('checked', true);
   } else if (tgt == '#milestones_show'){
     var _el = $('.milestone_inactive')
diff --git a/pagure/templates/settings_milestones.html b/pagure/templates/settings_milestones.html
index 6ee133b..2c997aa 100644
--- a/pagure/templates/settings_milestones.html
+++ b/pagure/templates/settings_milestones.html
@@ -33,12 +33,13 @@
       <div class="row p-t-1 milestone{%if milestone and
       not repo.milestones[milestone]['active'] %} milestone_inactive {%
       endif %}" id="milestone_{{ loop.index }}">
+        <input type="hidden" name="milestones" value="{{ loop.index }}">
         <div class="col-sm-4 p-r-0">
-          <input type="text" name="milestones"
+          <input type="text" name="milestone_{{ loop.index }}_name"
             value="{{ milestone }}" size="3" class="form-control"/>
         </div>
         <div class="col-sm-4 p-r-0">
-          <input type="text" name="milestone_date_{{ loop.index }}"
+          <input type="text" name="milestone_{{ loop.index }}_date"
             value="{{ repo.milestones[milestone]['date']
                 if milestone and repo.milestones[milestone]['date'] is not none
             }}" class="form-control"/>
@@ -50,8 +51,8 @@
                 data-stone="{{ loop.index }}"></span>
         </div>
         <div class="col-sm-1 p-r-0" >
-            <input type="checkbox" name="active_milestone_{{ loop.index
-            }}"{% if milestone and repo.milestones[milestone]['active']
+            <input type="checkbox" name="milestone_{{ loop.index
+            }}_active"{% if milestone and repo.milestones[milestone]['active']
             %} checked{% endif %} />
         </div>
       </div>
diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py
index b9fb770..e6969aa 100644
--- a/pagure/ui/repo.py
+++ b/pagure/ui/repo.py
@@ -1516,41 +1516,45 @@ def update_milestones(repo, username=None, namespace=None):
     error = False
     if form.validate_on_submit():
         redirect = flask.request.args.get("from")
-        milestones = [
-            w.strip() for w in flask.request.form.getlist("milestones")
-        ]
+        milestones = flask.request.form.getlist("milestones")
+        miles = {}
+        keys = []
 
-        for cnt, milestone in enumerate(milestones):
-            if milestone.strip() and milestones.count(milestone) != 1:
-                flask.flash(
-                    "Milestone %s is present %s times"
-                    % (milestone, milestones.count(milestone)),
-                    "error",
+        for idx in milestones:
+
+            milestone = flask.request.form.get(
+                "milestone_%s_name" % (idx), None
+            )
+
+            date = flask.request.form.get(
+                "milestone_%s_date" % (idx), None
+            )
+
+            active = (
+                True
+                if flask.request.form.get(
+                    "milestone_%s_active" % (idx)
                 )
-                error = True
-                break
+                else False
+            )
 
-        keys = []
-        if not error:
-            miles = {}
-            for cnt in range(len(milestones)):
-                active = (
-                    True
-                    if flask.request.form.get(
-                        "active_milestone_%s" % (cnt + 1)
+            if milestone and milestone.strip():
+                milestone = milestone.strip()
+                if milestone in miles:
+                    flask.flash(
+                        "Milestone %s is present multiple times" % milestone,
+                        "error",
                     )
-                    else False
-                )
-                date = flask.request.form.get(
-                    "milestone_date_%s" % (cnt + 1), None
-                )
+                    error = True
+                    break
 
-                if milestones[cnt].strip():
-                    miles[milestones[cnt]] = {
-                        "date": date.strip() if date else None,
-                        "active": active,
-                    }
-                    keys.append(milestones[cnt])
+                miles[milestone] = {
+                    "date": date.strip() if date else None,
+                    "active": active,
+                }
+                keys.append(milestone)
+
+        if not error:
             try:
                 repo.milestones = miles
                 repo.milestones_keys = keys
diff --git a/tests/test_pagure_flask_ui_repo.py b/tests/test_pagure_flask_ui_repo.py
index 12a4f39..4f1916e 100644
--- a/tests/test_pagure_flask_ui_repo.py
+++ b/tests/test_pagure_flask_ui_repo.py
@@ -1434,12 +1434,13 @@ class PagureFlaskRepotests(tests.Modeltests):
             self.assertIn(
             '''<div id="milestones">
       <div class="row p-t-1 milestone" id="milestone_1">
+        <input type="hidden" name="milestones" value="1">
         <div class="col-sm-4 p-r-0">
-          <input type="text" name="milestones"
+          <input type="text" name="milestone_1_name"
             value="" size="3" class="form-control"/>
         </div>
         <div class="col-sm-4 p-r-0">
-          <input type="text" name="milestone_date_1"
+          <input type="text" name="milestone_1_date"
             value="" class="form-control"/>
         </div>
         <div class="col-sm-2 p-r-0" >
@@ -1449,7 +1450,7 @@ class PagureFlaskRepotests(tests.Modeltests):
                 data-stone="1"></span>
         </div>
         <div class="col-sm-1 p-r-0" >
-            <input type="checkbox" name="active_milestone_1" />
+            <input type="checkbox" name="milestone_1_active" />
         </div>
       </div>''', output_text)
 
diff --git a/tests/test_pagure_flask_ui_repo_milestones.py b/tests/test_pagure_flask_ui_repo_milestones.py
index b57dd7d..a7e83c8 100644
--- a/tests/test_pagure_flask_ui_repo_milestones.py
+++ b/tests/test_pagure_flask_ui_repo_milestones.py
@@ -68,12 +68,13 @@ class PagureFlaskRepoMilestonestests(tests.Modeltests):
             self.assertIn(
             '''<div id="milestones">
       <div class="row p-t-1 milestone" id="milestone_1">
+        <input type="hidden" name="milestones" value="1">
         <div class="col-sm-4 p-r-0">
-          <input type="text" name="milestones"
+          <input type="text" name="milestone_1_name"
             value="" size="3" class="form-control"/>
         </div>
         <div class="col-sm-4 p-r-0">
-          <input type="text" name="milestone_date_1"
+          <input type="text" name="milestone_1_date"
             value="" class="form-control"/>
         </div>
         <div class="col-sm-2 p-r-0" >
@@ -83,7 +84,7 @@ class PagureFlaskRepoMilestonestests(tests.Modeltests):
                 data-stone="1"></span>
         </div>
         <div class="col-sm-1 p-r-0" >
-            <input type="checkbox" name="active_milestone_1" />
+            <input type="checkbox" name="milestone_1_active" />
         </div>
       </div>''', output.get_data(as_text=True))
 
diff --git a/tests/test_pagure_flask_ui_roadmap.py b/tests/test_pagure_flask_ui_roadmap.py
index d7cd731..96791a9 100644
--- a/tests/test_pagure_flask_ui_roadmap.py
+++ b/tests/test_pagure_flask_ui_roadmap.py
@@ -177,7 +177,8 @@ class PagureFlaskRoadmaptests(tests.Modeltests):
 
             data = {
                 'milestones': 1,
-                'milestone_dates': 'Tomorrow',
+                'milestone_1_name': '1',
+                'milestone_1_date': 'Tomorrow',
             }
             output = self.app.post(
                 '/test/update/milestones', data=data, follow_redirects=True)
@@ -195,7 +196,8 @@ class PagureFlaskRoadmaptests(tests.Modeltests):
 
             data = {
                 'milestones': 1,
-                'milestone_dates': 'Tomorrow',
+                'milestone_1_name': '1',
+                'milestone_date': '',
                 'csrf_token': csrf_token,
             }
             output = self.app.post(
@@ -216,9 +218,11 @@ class PagureFlaskRoadmaptests(tests.Modeltests):
             self.assertEqual(repo.milestones, {'1': {'active': False, 'date': None}})
 
             data = {
-                'milestones': ['v1.0', 'v2.0'],
-                'milestone_dates_1': 'Tomorrow',
-                'milestone_dates_2': '',
+                'milestones': [1, 2],
+                'milestone_1_name': 'v1.0',
+                'milestone_2_name': 'v2.0',
+                'milestone_1_date': 'Tomorrow',
+                'milestone_2_date': '',
                 'csrf_token': csrf_token,
             }
             output = self.app.post(
@@ -238,17 +242,19 @@ class PagureFlaskRoadmaptests(tests.Modeltests):
             self.assertEqual(
                 repo.milestones,
                 {
-                    'v1.0': {'active': False, 'date': None},
+                    'v1.0': {'active': False, 'date': 'Tomorrow'},
                     'v2.0': {'active': False, 'date': None}
                 }
             )
 
             # Check error - less milestones than dates
             data = {
-                'milestones': ['v1.0', 'v2.0'],
-                'milestone_date_1': 'Tomorrow',
-                'milestone_date_2': 'Next week',
-                'milestone_date_3': 'Next Year',
+                'milestones': [1, 2],
+                'milestone_1_name': 'v1.0',
+                'milestone_2_name': 'v2.0',
+                'milestone_1_date': 'Tomorrow',
+                'milestone_2_date': 'Next week',
+                'milestone_3_date': 'Next Year',
                 'csrf_token': csrf_token,
             }
             output = self.app.post(
@@ -274,10 +280,13 @@ class PagureFlaskRoadmaptests(tests.Modeltests):
 
             # Check error - Twice the same milestone
             data = {
-                'milestones': ['v1.0', 'v2.0', 'v2.0'],
-                'milestone_date_1': 'Tomorrow',
-                'milestone_date_2': 'Next week',
-                'milestone_date_3': 'Next Year',
+                'milestones': [1, 2, 3],
+                'milestone_1_name': 'v1.0',
+                'milestone_2_name': 'v2.0',
+                'milestone_3_name': 'v2.0',
+                'milestone_1_date': 'Tomorrow',
+                'milestone_2_date': 'Next week',
+                'milestone_3_date': 'Next Year',
                 'csrf_token': csrf_token,
             }
             output = self.app.post(
@@ -291,7 +300,8 @@ class PagureFlaskRoadmaptests(tests.Modeltests):
                 '<h5 class="pl-2 font-weight-bold text-muted">'
                 'Project Settings</h5>\n', output_text)
             self.assertIn(
-                'Milestone v2.0 is present 2 times',
+                'Milestone v2.0 is present multiple times',
+                'Milestone v2.0 is present multiple times',
                 output_text)
             # Check the result of the action -- Milestones un-changed
             self.session.commit()
@@ -306,10 +316,13 @@ class PagureFlaskRoadmaptests(tests.Modeltests):
 
             # Check error - Twice the same date
             data = {
-                'milestones': ['v1.0', 'v2.0', 'v3.0'],
-                'milestone_date_1': 'Tomorrow',
-                'milestone_date_2': 'Next week',
-                'milestone_date_3': 'Next week',
+                'milestones': [1, 2, 3],
+                'milestone_1_name': 'v1.0',
+                'milestone_2_name': 'v2.0',
+                'milestone_3_name': 'v3.0',
+                'milestone_1_date': 'Tomorrow',
+                'milestone_2_date': 'Next week',
+                'milestone_3_date': 'Next week',
                 'csrf_token': csrf_token,
             }
             output = self.app.post(
@@ -383,8 +396,11 @@ class PagureFlaskRoadmaptests(tests.Modeltests):
                 'name="csrf_token" type="hidden" value="')[1].split('">')[0]
 
             data = {
-                'milestones': ['v1.0', 'v2.0'],
-                'milestone_dates': ['', ''],
+                'milestones': [1, 2],
+                'milestone_1_name': 'v1.0',
+                'milestone_2_name': 'v2.0',
+                'milestone_1_date': '',
+                'milestone_2_date': '',
                 'csrf_token': csrf_token,
             }
             output = self.app.post(
@@ -435,13 +451,16 @@ class PagureFlaskRoadmaptests(tests.Modeltests):
 
             # Create an unplanned milestone
             data = {
-                'milestones': ['v1.0', 'v2.0', 'unplanned'],
-                'milestone_date_1': 'Tomorrow',
-                'milestone_date_2': '',
-                'milestone_date_3': '',
-                'active_milestone_1': True,
-                'active_milestone_2': True,
-                'active_milestone_3': True,
+                'milestones': [1, 2, 3],
+                'milestone_1_name': 'v1.0',
+                'milestone_2_name': 'v2.0',
+                'milestone_3_name': 'unplanned',
+                'milestone_1_date': 'Tomorrow',
+                'milestone_2_date': '',
+                'milestone_3_date': '',
+                'milestone_1_active': True,
+                'milestone_2_active': True,
+                'milestone_3_active': True,
                 'csrf_token': csrf_token,
             }
             output = self.app.post(