PR#1782: Add buttons for "All milestone tags", or "All active milestone tags" - pagure

		`@@ -3656,3 +3656,20 @@`
		`)`

		`return query.first()`
		`+`
		`+`
		`+ def get_active_milestones(session, project):`
		`+ ''' Returns the list of all the active milestones for a given project.`
		`+ '''`
		`+`
		`+ query = session.query(`
		`+ model.Issue.milestone`
		`+ ).filter(`
		`+ model.Issue.project_id == project.id`
		`+ ).filter(`
		`+ model.Issue.status == 'Open'`
		`+ ).filter(`
		`+ model.Issue.milestone.isnot(None)`
		`+ )`
		`+`
		`+ return sorted([item[0] for item in query.distinct()])`

pagure/templates/roadmap.html

file modified

+35 -6

		`@@ -42,7 +42,7 @@`

		`<section id="flags">`
		`<span class="btn-group btn-group-sm issues-tagbar" role="group">`
		`- <span class="oi m-l-2 btn btn-nopad" data-glyph="briefcase" title="Status"></span>`
		`+ <span class="oi m-l-2 btn btn-nopad" title="Status"></span>`
		`<a class="btn {%`
		`if status\|lower in ['open', 'true'] %}btn-primary{%`
		`else %}btn-secondary{%`
		`@@ -51,6 +51,7 @@`
		`username=username,`
		`namespace=repo.namespace,`
		`milestone=requested_stones,`
		`+ all_stones=all_stones,`
		`tag=tags) }}" title="Filter issues by status">Open</a>`
		`<a class="btn {%`
		`if status\|lower in ['closed', 'false'] %}btn-primary{%`
		`@@ -61,6 +62,7 @@`
		`namespace=repo.namespace,`
		`tag=tags,`
		`milestone=requested_stones,`
		`+ all_stones=all_stones,`
		`status='Closed') }}" title="Filter issues by status">Closed</a>`
		`<a class="btn {%`
		`if status\|lower in ['all', 'none'] %}btn-primary{%`
		`@@ -71,24 +73,48 @@`
		`namespace=repo.namespace,`
		`tag=tags,`
		`milestone=requested_stones,`
		`+ all_stones=all_stones,`
		`status='all') }}" title="Filter issues by status">All</a>`
		`</span>`
		`+ <span class="btn-group btn-group-sm issues-tagbar" role="group">`
		`+ <span class="oi m-l-2 btn btn-nopad" title="Status"></span>`
		`+ <a class="btn {%`
		`+ if not all_stones %}btn-primary{%`
		`+ else %}btn-secondary{%`
		`+ endif %} btn-sm" href="{{ url_for('view_roadmap',`
		`+ repo=repo.name,`
		`+ username=username,`
		`+ namespace=repo.namespace,`
		`+ tag=tags,`
		`+ status=status) }}" title="Only display active milestone tags">Active Milestones</a>`
		`+ <a class="btn {%`
		`+ if all_stones %}btn-primary{%`
		`+ else %}btn-secondary{%`
		`+ endif %} btn-sm" href="{{ url_for('view_roadmap',`
		`+ repo=repo.name,`
		`+ username=username,`
		`+ namespace=repo.namespace,`
		`+ tag=tags,`
		`+ all_stones=true,`
		`+ status=status) }}" title="Display all milestone tags">All Milestones</a>`
		`+ </span>`
		`<span class="btn-group btn-group-sm issues-tagbar" role="group" aria-label="Basic example">`
		`<span class="oi m-l-2 btn btn-nopad" data-glyph="target" title="Milestones"></span>`
		`{% for stone in milestones %}`
		`{% if stone in requested_stones %}`
		`- <a class="btn btn-secondary btn-sm {% if stone in requested_stones %}active{% endif %}"`
		`- href="{{ url_for(`
		`+ <a class="btn btn-secondary btn-sm active"`
		`+ href="{{ url_for(`
		`'view_roadmap',`
		`repo=repo.name,`
		`username=username,`
		`namespace=repo.namespace,`
		`tag=tags,`
		`+ all_stones=all_stones,`
		`status=status) }}"`
		`title="Filter issues by milestone">`
		`{{ stone }}</a>`
		`{% else %}`
		`- <a class="btn btn-secondary btn-sm {% if stone in requested_stones%}active{% endif %}"`
		`+ <a class="btn btn-secondary btn-sm"`
		`href="{{ url_for(`
		`'view_roadmap',`
		`repo=repo.name,`
		`@@ -96,6 +122,7 @@`
		`namespace=repo.namespace,`
		`milestone=stone,`
		`tag=tags,`
		`+ all_stones=all_stones,`
		`status=status) }}"`
		`title="Filter issues by milestone">`
		`{{ stone }}</a>`
		`@@ -106,22 +133,24 @@`
		`<span class="oi m-l-2 btn btn-nopad" data-glyph="tag" title="Tags"></span>`
		`{% for tag in tag_list %}`
		`{% if tag in tags %}`
		`- <a class="btn btn-secondary btn-sm {% if tag in tags%}active{% endif %}"`
		`+ <a class="btn btn-secondary btn-sm {% if tag in tags %}active{% endif %}"`
		`href="{{ url_for('view_roadmap',`
		`repo=repo.name,`
		`username=username,`
		`namespace=repo.namespace,`
		`milestone=requested_stones,`
		`+ all_stones=all_stones,`
		`status=status) }}"`
		`title="Filter issues by tag">`
		`{% else %}`
		`- <a class="btn btn-secondary btn-sm {% if tag in tags%}active{%endif%}"`
		`+ <a class="btn btn-secondary btn-sm {% if tag in tags %}active{% endif %}"`
		`href="{{ url_for('view_roadmap',`
		`repo=repo.name,`
		`username=username,`
		`namespace=repo.namespace,`
		`status=status,`
		`tag=tag,`
		`+ all_stones=all_stones,`
		`milestone=requested_stones) }}"`
		`title="Filter issues by tag">`
		`{% endif %}`

pagure/ui/issues.py

file modified

+10 -2

		`@@ -713,6 +713,8 @@`
		`status = flask.request.args.get('status', 'Open')`
		`milestones = flask.request.args.getlist('milestone', None)`
		`tags = flask.request.args.getlist('tag', None)`
		`+ all_stones = flask.request.args.get('all_stones')`
		`+ active_milestones = flask.request.args.getlist('active_milestones', None)`

		`repo = flask.g.repo`

		`@@ -729,6 +731,7 @@`
		`private = None`

		`all_milestones = sorted(list(repo.milestones.keys()))`
		`+ active_milestones = pagure.lib.get_active_milestones(SESSION, repo)`

		`issues = pagure.lib.search_issues(`
		`SESSION,`
		`@@ -736,7 +739,7 @@`
		`milestones=milestones or all_milestones,`
		`tags=tags,`
		`private=private,`
		`- status=status if status.lower()!= 'all' else None,`
		`+ status=status if status.lower() != 'all' else None,`
		`)`

		`# Change from a list of issues to a dict of milestone/issues`
		`@@ -771,6 +774,10 @@`
		`cnt = len(all_milestones)`
		`all_milestones.insert(cnt, all_milestones.pop(index))`

		`+ milestones_list = active_milestones`
		`+ if all_stones:`
		`+ milestones_list = all_milestones`
		`+`
		`return flask.render_template(`
		`'roadmap.html',`
		`select='issues',`
		`@@ -778,7 +785,8 @@`
		`username=username,`
		`tag_list=tag_list,`
		`status=status,`
		`- milestones=all_milestones,`
		`+ all_stones=all_stones,`
		`+ milestones=milestones_list,`
		`requested_stones=milestones,`
		`issues=milestone_issues,`
		`tags=tags,`

tests/test_pagure_flask_ui_roadmap.py

file modified

+3 -2

		`@@ -487,7 +487,7 @@`
		`output.data.count(u'<span class="label label-default">#'), 4)`

		`# test the roadmap view for all milestones`
		`- output = self.app.get('/test/roadmap?status=All')`
		`+ output = self.app.get('/test/roadmap?all_stones=True&status=All')`
		`self.assertEqual(output.status_code, 200)`
		`self.assertIn(u'<th>v1.0', output.data)`
		`self.assertIn(u'<th>v2.0', output.data)`
		`@@ -510,7 +510,8 @@`
		`output.data.count(u'<span class="label label-default">#'), 0)`

		`# test the roadmap view for a specific milestone - closed`
		`- output = self.app.get('/test/roadmap?milestone=v1.0&status=All')`
		`+ output = self.app.get(`
		`+ '/test/roadmap?milestone=v1.0&status=All&all_stones=True')`
		`self.assertEqual(output.status_code, 200)`
		`self.assertIn(u'<th>v1.0', output.data)`
		`self.assertEqual(`

mreynolds commented 7 years ago

I still want to improve the page behaviour when selecting milestones tags and issue tags...

1 new commit added

Improve how the milestone tags are dispalyed when filtering is applied

7 years ago

mreynolds commented 7 years ago

This is ready for review

pingou commented on line 5 of pagure/templates/roadmap.html 7 years ago

No more briefcase?

pingou commented 7 years ago

This is looking quite far from what I had in mind, I'll need to test it

mreynolds commented 7 years ago

This is looking quite far from what I had in mind, I'll need to test it

Please do, I am quite happy with how it turned out. And yes I removed the briefcase on purpose. I think it makes the page cleaner - you'll see.

mreynolds commented 7 years ago

This is the best scenario to see the potential of this change:

Create two issue tags: "foo_tag", and "foo2_tag"
Create 3 milestones: milestone-1,..,milestone-3
Create two issues, one with milestone-1, the other with milestone-3
Add "foo_tag" to one of the issues

Then goto the roadmap page, and change between "active" and "all" milestones. See how the milestone tags change. Then select the foo_tag, and bounce between all the the page options: different milestones, status, "active/all milestones", the issue tags, etc.

rebased

7 years ago

pingou commented 7 years ago

@mreynolds I've tried a slightly different approach, would you mind testing it?

I also found that it speeds thing up a little with selecting a single milestone while 'All milestones' are shown

pingou commented 7 years ago

Looks like it missed my second path, so here it is:

That one is purely aesthetic :)

pingou commented 7 years ago

I didn't dare to push these changes directly to your branch as I feared it may break a little (though from my current tests it seems not to)

mreynolds commented 7 years ago

Sorry your patches break everything :(

Milestone tags get incorrectly reset when when clicking on issue tags or individual milestone tags. Also, clicking on "Active Milestones" does not reset the button (All Milestones is always active)

pingou commented 7 years ago

Also, clicking on "Active Milestones" does not reset the button

Ok fixed that one

Milestone tags get incorrectly reset when when clicking on issue tags or individual milestone tags

That one I don't quite follow :(

pingou commented 7 years ago

This is the patch fixing the toggling

mreynolds commented 7 years ago

Also, clicking on "Active Milestones" does not reset the button

Ok fixed that one

Milestone tags get incorrectly reset when when clicking on issue tags or individual milestone tags

That one I don't quite follow :(

Yeah this is the most important part. Once you select "active milestones" the milestone tag list must NOT change regardless of what milestone tag or issue tag you select. So with your fix I select active milestones, then if I click on the "milestone-1" tag then all the milestone tags appears (active and non active) - bad. Same when I click on an issue tag, it brings in all the milestone tags.

Getting this part to work correctly was the hardest part of my fix ;)

pingou commented 7 years ago

Ok, I found the error, it's something that was at first working, I changed it and introduced a bug, here is the fix:

diff --git a/ pagure/ui/issues.py b/ pagure/ui/issues.py
index 0d18fe9..f4fba3f 100644
--- a/ pagure/ui/issues.py      
+++ b/ pagure/ui/issues.py      
@@ -713,7 +713,7 @@ def view_roadmap(repo, username=None, namespace=None):
     status = flask.request.args.get('status', 'Open')
     milestones = flask.request.args.getlist('milestone', None)
     tags = flask.request.args.getlist('tag', None)
-    all_stones = flask.request.args.get('all_stones', False)
+    all_stones = flask.request.args.get('all_stones')
     active_milestones = flask.request.args.getlist('active_milestones', None)

EDIT: The reason behind the bug: this None is being converted into 'None' in the template and ends up making if all_stones return True because a string (not empty) returns True, so it ends up being as if we asked for all the milestones instead of just the active ones

Edited 7 years ago by pingou

mreynolds commented 7 years ago

Well this patch fixes the milestone and issue tags, but it caused another regression. Now there are no issues displayed regardless if I choose active or all milestones

mreynolds commented 7 years ago

Correction - it is working correctly :)

pingou commented 7 years ago

You scared me man! Watch out for these things, I almost die here :D

Ok, let's push the changes and review them here as well :)

4 new commits added