From 9f0083a96c03ec22b1d9442a9c7530899e633301 Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Sun, 5 Oct 2014 00:53:44 -0500 Subject: [PATCH 1/4] Add task lists to issues and merge requests Make the Markdown parser recognize "[x]" or "[ ]" at the beginning of a list item and turn it into a checkbox input. Users who can modify the issue or MR can toggle the checkboxes directly or edit the Markdown to manage the tasks. Task status is also displayed in the MR and issue lists. --- app/assets/javascripts/issue.js.coffee | 24 +++++++++ .../javascripts/merge_request.js.coffee | 23 +++++++++ app/assets/stylesheets/generic/common.scss | 3 ++ app/assets/stylesheets/generic/lists.scss | 4 ++ app/controllers/projects/issues_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 2 +- app/models/concerns/taskable.rb | 51 +++++++++++++++++++ app/models/issue.rb | 1 + app/models/merge_request.rb | 1 + app/services/issues/update_service.rb | 14 ++++- app/services/merge_requests/update_service.rb | 8 ++- app/views/projects/issues/_issue.html.haml | 4 ++ app/views/projects/issues/show.html.haml | 2 +- .../merge_requests/_merge_request.html.haml | 4 +- .../merge_requests/show/_mr_box.html.haml | 2 +- lib/gitlab/markdown.rb | 24 +++++++++ lib/redcarpet/render/gitlab_html.rb | 6 ++- 17 files changed, 167 insertions(+), 8 deletions(-) create mode 100644 app/models/concerns/taskable.rb diff --git a/app/assets/javascripts/issue.js.coffee b/app/assets/javascripts/issue.js.coffee index 36935a0a159..f2b531fb2b1 100644 --- a/app/assets/javascripts/issue.js.coffee +++ b/app/assets/javascripts/issue.js.coffee @@ -6,4 +6,28 @@ class Issue $(".issue-box .inline-update").on "change", "#issue_assignee_id", -> $(this).submit() + if $("a.btn-close").length + $("li.task-list-item input:checkbox").prop("disabled", false) + + $(".task-list-item input:checkbox").on "click", -> + is_checked = $(this).prop("checked") + if $(this).is(":checked") + state_event = "task_check" + else + state_event = "task_uncheck" + + mr_url = $("form.edit-issue").first().attr("action") + mr_num = mr_url.match(/\d+$/) + task_num = 0 + $("li.task-list-item input:checkbox").each( (index, e) => + if e == this + task_num = index + 1 + ) + + $.ajax + type: "PATCH" + url: mr_url + data: "issue[state_event]=" + state_event + + "&issue[task_num]=" + task_num + @Issue = Issue diff --git a/app/assets/javascripts/merge_request.js.coffee b/app/assets/javascripts/merge_request.js.coffee index 4c9f20ae6fa..203c721c30c 100644 --- a/app/assets/javascripts/merge_request.js.coffee +++ b/app/assets/javascripts/merge_request.js.coffee @@ -17,6 +17,8 @@ class MergeRequest disableButtonIfEmptyField '#commit_message', '.accept_merge_request' + if $("a.close-mr-link").length + $("li.task-list-item input:checkbox").prop("disabled", false) # Local jQuery finder $: (selector) -> @@ -72,6 +74,27 @@ class MergeRequest this.$('.remove_source_branch_in_progress').hide() this.$('.remove_source_branch_widget.failed').show() + this.$(".task-list-item input:checkbox").on "click", -> + is_checked = $(this).prop("checked") + if $(this).is(":checked") + state_event = "task_check" + else + state_event = "task_uncheck" + + mr_url = $("form.edit-merge_request").first().attr("action") + mr_num = mr_url.match(/\d+$/) + task_num = 0 + $("li.task-list-item input:checkbox").each( (index, e) => + if e == this + task_num = index + 1 + ) + + $.ajax + type: "PATCH" + url: mr_url + data: "merge_request[state_event]=" + state_event + + "&merge_request[task_num]=" + task_num + activateTab: (action) -> this.$('.merge-request-tabs li').removeClass 'active' this.$('.tab-content').hide() diff --git a/app/assets/stylesheets/generic/common.scss b/app/assets/stylesheets/generic/common.scss index 803219a2e86..cd2f4e45e3c 100644 --- a/app/assets/stylesheets/generic/common.scss +++ b/app/assets/stylesheets/generic/common.scss @@ -356,3 +356,6 @@ table { font-size: 42px; } +.task-status { + margin-left: 10px; +} diff --git a/app/assets/stylesheets/generic/lists.scss b/app/assets/stylesheets/generic/lists.scss index d347ab2c2e4..2653bfbf831 100644 --- a/app/assets/stylesheets/generic/lists.scss +++ b/app/assets/stylesheets/generic/lists.scss @@ -122,3 +122,7 @@ ul.bordered-list { } } } + +li.task-list-item { + list-style-type: none; +} diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 9e7a55b23fd..c6d526f05c5 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -152,7 +152,7 @@ class Projects::IssuesController < Projects::ApplicationController def issue_params params.require(:issue).permit( :title, :assignee_id, :position, :description, - :milestone_id, :state_event, label_ids: [] + :milestone_id, :state_event, :task_num, label_ids: [] ) end end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index e13773d6465..20a733b10e1 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -250,7 +250,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController params.require(:merge_request).permit( :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch, :milestone_id, - :state_event, :description, label_ids: [] + :state_event, :description, :task_num, label_ids: [] ) end end diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb new file mode 100644 index 00000000000..410e8dc820b --- /dev/null +++ b/app/models/concerns/taskable.rb @@ -0,0 +1,51 @@ +# Contains functionality for objects that can have task lists in their +# descriptions. Task list items can be added with Markdown like "* [x] Fix +# bugs". +# +# Used by MergeRequest and Issue +module Taskable + TASK_PATTERN_MD = /^(? *[*-] *)\[(?[ xX])\]/.freeze + TASK_PATTERN_HTML = /^
  • \[(?[ xX])\]/.freeze + + # Change the state of a task list item for this Taskable. Edit the object's + # description by finding the nth task item and changing its checkbox + # placeholder to "[x]" if +checked+ is true, or "[ ]" if it's false. + # Note: task numbering starts with 1 + def update_nth_task(n, checked) + index = 0 + check_char = checked ? 'x' : ' ' + + # Do this instead of using #gsub! so that ActiveRecord detects that a field + # has changed. + self.description = self.description.gsub(TASK_PATTERN_MD) do |match| + index += 1 + case index + when n then "#{$LAST_MATCH_INFO[:bullet]}[#{check_char}]" + else match + end + end + + save + end + + # Return true if this object's description has any task list items. + def tasks? + description && description.match(TASK_PATTERN_MD) + end + + # Return a string that describes the current state of this Taskable's task + # list items, e.g. "20 tasks (12 done, 8 unfinished)" + def task_status + return nil unless description + + num_tasks = 0 + num_done = 0 + + description.scan(TASK_PATTERN_MD) do + num_tasks += 1 + num_done += 1 unless $LAST_MATCH_INFO[:checked] == ' ' + end + + "#{num_tasks} tasks (#{num_done} done, #{num_tasks - num_done} unfinished)" + end +end diff --git a/app/models/issue.rb b/app/models/issue.rb index 13152fdf94e..8a9e969248c 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -23,6 +23,7 @@ require 'file_size_validator' class Issue < ActiveRecord::Base include Issuable include InternalId + include Taskable ActsAsTaggableOn.strict_case_match = true diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e0358c1889c..7c525b02f48 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -25,6 +25,7 @@ require Rails.root.join("lib/static_model") class MergeRequest < ActiveRecord::Base include Issuable + include Taskable include InternalId belongs_to :target_project, foreign_key: :target_project_id, class_name: "Project" diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index a0e57144435..5b2746ffecf 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -8,9 +8,14 @@ module Issues Issues::ReopenService.new(project, current_user, {}).execute(issue) when 'close' Issues::CloseService.new(project, current_user, {}).execute(issue) + when 'task_check' + issue.update_nth_task(params[:task_num].to_i, true) + when 'task_uncheck' + issue.update_nth_task(params[:task_num].to_i, false) end - if params.present? && issue.update_attributes(params.except(:state_event)) + if params.present? && issue.update_attributes(params.except(:state_event, + :task_num)) issue.reset_events_cache if issue.previous_changes.include?('milestone_id') @@ -28,5 +33,12 @@ module Issues issue end + + private + + def update_task(issue, params, checked) + issue.update_nth_task(params[:task_num].to_i, checked) + params.except!(:task_num) + end end end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 6e416a0080c..fc26619cd17 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -17,9 +17,15 @@ module MergeRequests MergeRequests::ReopenService.new(project, current_user, {}).execute(merge_request) when 'close' MergeRequests::CloseService.new(project, current_user, {}).execute(merge_request) + when 'task_check' + merge_request.update_nth_task(params[:task_num].to_i, true) + when 'task_uncheck' + merge_request.update_nth_task(params[:task_num].to_i, false) end - if params.present? && merge_request.update_attributes(params.except(:state_event)) + if params.present? && merge_request.update_attributes( + params.except(:state_event, :task_num) + ) merge_request.reset_events_cache if merge_request.previous_changes.include?('milestone_id') diff --git a/app/views/projects/issues/_issue.html.haml b/app/views/projects/issues/_issue.html.haml index e089b5fa1cf..b125706781c 100644 --- a/app/views/projects/issues/_issue.html.haml +++ b/app/views/projects/issues/_issue.html.haml @@ -26,6 +26,10 @@ %span %i.fa.fa-clock-o = issue.milestone.title + - if issue.tasks? + %span.task-status + = issue.task_status + .pull-right %small updated #{time_ago_with_tooltip(issue.updated_at, 'bottom', 'issue_update_ago')} diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index 4c1ea098d98..e1849b3f8b8 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -48,7 +48,7 @@ .description .wiki = preserve do - = markdown @issue.description + = markdown(@issue.description, parse_tasks: true) .context %cite.cgray = render partial: 'issue_context', locals: { issue: @issue } diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index 647e8873e9e..1ee2e1bdae8 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -27,7 +27,9 @@ %span %i.fa.fa-clock-o = merge_request.milestone.title - + - if merge_request.tasks? + %span.task-status + = merge_request.task_status .pull-right %small updated #{time_ago_with_tooltip(merge_request.updated_at, 'bottom', 'merge_request_updated_ago')} diff --git a/app/views/projects/merge_requests/show/_mr_box.html.haml b/app/views/projects/merge_requests/show/_mr_box.html.haml index f1aaba2010d..7e5a4eda508 100644 --- a/app/views/projects/merge_requests/show/_mr_box.html.haml +++ b/app/views/projects/merge_requests/show/_mr_box.html.haml @@ -18,7 +18,7 @@ .description .wiki = preserve do - = markdown @merge_request.description + = markdown(@merge_request.description, parse_tasks: true) .context %cite.cgray diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 709a74fe21e..17512a51658 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -33,6 +33,11 @@ module Gitlab attr_reader :html_options + def gfm_with_tasks(text, project = @project, html_options = {}) + text = gfm(text, project, html_options) + parse_tasks(text) + end + # Public: Parse the provided text with GitLab-Flavored Markdown # # text - the source text @@ -265,5 +270,24 @@ module Gitlab ) link_to("#{prefix_text}##{identifier}", url, options) end + + # Turn list items that start with "[ ]" into HTML checkbox inputs. + def parse_tasks(text) + li_tag = '
  • ' + unchecked_box = '' + checked_box = unchecked_box.sub(/\/>$/, 'checked="checked" />') + + # Regexp captures don't seem to work when +text+ is an + # ActiveSupport::SafeBuffer, hence the `String.new` + String.new(text).gsub(Taskable::TASK_PATTERN_HTML) do + checked = $LAST_MATCH_INFO[:checked].downcase == 'x' + + if checked + "#{li_tag}#{checked_box}" + else + "#{li_tag}#{unchecked_box}" + end + end + end end end diff --git a/lib/redcarpet/render/gitlab_html.rb b/lib/redcarpet/render/gitlab_html.rb index bb225f1acd8..c3378d6a18f 100644 --- a/lib/redcarpet/render/gitlab_html.rb +++ b/lib/redcarpet/render/gitlab_html.rb @@ -47,6 +47,10 @@ class Redcarpet::Render::GitlabHTML < Redcarpet::Render::HTML unless @template.instance_variable_get("@project_wiki") || @project.nil? full_document = h.create_relative_links(full_document) end - h.gfm(full_document) + if @options[:parse_tasks] + h.gfm_with_tasks(full_document) + else + h.gfm(full_document) + end end end From fecd9c0cb22eda0a208042c568d8d833a1bd6292 Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Sun, 5 Oct 2014 21:17:28 -0500 Subject: [PATCH 2/4] Add specs for tasks Add tests for the new task list functionality in the Markdown parser and in issues and merge requests. --- spec/helpers/gitlab_markdown_helper_spec.rb | 101 +++++++++++++++++++- spec/models/issue_spec.rb | 4 + spec/models/merge_request_spec.rb | 4 + spec/support/taskable_shared_examples.rb | 42 ++++++++ 4 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 spec/support/taskable_shared_examples.rb diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 73b3d91e96e..15033f07432 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -616,7 +616,7 @@ describe GitlabMarkdownHelper do end end - describe "markdwon for empty repository" do + describe 'markdown for empty repository' do before do @project = empty_project @repository = empty_project.repository @@ -652,4 +652,103 @@ describe GitlabMarkdownHelper do helper.render_wiki_content(@wiki) end end + + describe '#gfm_with_tasks' do + before(:all) do + @source_text_asterisk = <(txt){ subject.description = txt } } end + + it_behaves_like 'a Taskable' do + let(:subject) { create :issue } + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c40f75290ed..7b0d261d72f 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -119,4 +119,8 @@ describe MergeRequest do let(:backref_text) { "merge request !#{subject.iid}" } let(:set_mentionable_text) { ->(txt){ subject.title = txt } } end + + it_behaves_like 'a Taskable' do + let(:subject) { create :merge_request, :simple } + end end diff --git a/spec/support/taskable_shared_examples.rb b/spec/support/taskable_shared_examples.rb new file mode 100644 index 00000000000..42252675683 --- /dev/null +++ b/spec/support/taskable_shared_examples.rb @@ -0,0 +1,42 @@ +# Specs for task state functionality for issues and merge requests. +# +# Requires a context containing: +# let(:subject) { Issue or MergeRequest } +shared_examples 'a Taskable' do + before do + subject.description = < Date: Sun, 5 Oct 2014 22:04:58 -0500 Subject: [PATCH 3/4] Update docs for task lists --- CHANGELOG | 1 + doc/markdown/markdown.md | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 857a5bc9234..14bb88f22a5 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -18,6 +18,7 @@ v 7.4.0 - Add Pushover service integration (Sullivan Senechal) - Add select field type for services options (Sullivan Senechal) - Add cross-project references to the Markdown parser (Vinnie Okada) + - Add task lists to issue and merge request descriptions (Vinnie Okada) v 7.3.2 - Fix creating new file via web editor diff --git a/doc/markdown/markdown.md b/doc/markdown/markdown.md index 5c095ed1487..6d96da76ad7 100644 --- a/doc/markdown/markdown.md +++ b/doc/markdown/markdown.md @@ -10,6 +10,7 @@ * [Code and Syntax Highlighting](#code-and-syntax-highlighting) * [Emoji](#emoji) * [Special GitLab references](#special-gitlab-references) +* [Task lists](#task-lists) **[Standard Markdown](#standard-markdown)** @@ -183,6 +184,18 @@ GFM also recognizes references to commits, issues, and merge requests in other p - namespace/project!123 : for merge requests - namespace/project@1234567 : for commits +## Task Lists + +You can add task lists to merge request and issue descriptions to keep track of to-do items. To create a task, add an unordered list to the description in an issue or merge request, formatted like so: + +```no-highlight +* [x] Completed task +* [ ] Unfinished task + * [x] Nested task +``` + +Task lists can only be created in descriptions, not in titles or comments. Task item state can be managed by editing the description's Markdown or by clicking the rendered checkboxes. + # Standard Markdown ## Headers From 31bc42de57b3cfd7bf068df06d15372307b8661b Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Mon, 6 Oct 2014 01:59:03 -0500 Subject: [PATCH 4/4] Add Spinach tests for task lists --- features/project/issues/issues.feature | 33 ++++++++++++++++++++++++ features/project/merge_requests.feature | 33 ++++++++++++++++++++++++ features/steps/project/issues/issues.rb | 26 +++++++++++++++++++ features/steps/project/merge_requests.rb | 14 ++++++++++ features/steps/shared/markdown.rb | 30 +++++++++++++++++++++ features/steps/shared/note.rb | 14 ++++++++++ features/steps/shared/paths.rb | 20 ++++++++++++++ 7 files changed, 170 insertions(+) diff --git a/features/project/issues/issues.feature b/features/project/issues/issues.feature index ae6a03ce865..e989569ccd4 100644 --- a/features/project/issues/issues.feature +++ b/features/project/issues/issues.feature @@ -126,3 +126,36 @@ Feature: Project Issues When I click label 'bug' And I should see "Release 0.4" in issues And I should not see "Tweet control" in issues + + Scenario: Issue description should render task checkboxes + Given project "Shop" has "Tasks-open" open issue with task markdown + When I visit issue page "Tasks-open" + Then I should see task checkboxes in the description + + @javascript + Scenario: Issue notes should not render task checkboxes + Given project "Shop" has "Tasks-open" open issue with task markdown + When I visit issue page "Tasks-open" + And I leave a comment with task markdown + Then I should not see task checkboxes in the comment + + # Task status in issues list + + Scenario: Issues list should display task status + Given project "Shop" has "Tasks-open" open issue with task markdown + When I visit project "Shop" issues page + Then I should see the task status for issue "Tasks-open" + + # Toggling task items + + @javascript + Scenario: Task checkboxes should be enabled for an open issue + Given project "Shop" has "Tasks-open" open issue with task markdown + When I visit issue page "Tasks-open" + Then Task checkboxes should be enabled + + @javascript + Scenario: Task checkboxes should be disabled for a closed issue + Given project "Shop" has "Tasks-closed" closed issue with task markdown + When I visit issue page "Tasks-closed" + Then Task checkboxes should be disabled diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index f8dccc15c0e..bad83191371 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -96,6 +96,16 @@ Feature: Project Merge Requests And I leave a comment with a header containing "Comment with a header" Then The comment with the header should not have an ID + Scenario: Merge request description should render task checkboxes + Given project "Shop" has "MR-task-open" open MR with task markdown + When I visit merge request page "MR-task-open" + Then I should see task checkboxes in the description + + Scenario: Merge request notes should not render task checkboxes + Given project "Shop" has "MR-task-open" open MR with task markdown + When I visit merge request page "MR-task-open" + Then I should not see task checkboxes in the comment + # Toggling inline comments @javascript @@ -157,3 +167,26 @@ Feature: Project Merge Requests And I leave a comment like "Line is wrong" on line 39 of the second file And I click Side-by-side Diff tab Then I should see comments on the side-by-side diff page + + # Task status in issues list + + @now + Scenario: Merge requests list should display task status + Given project "Shop" has "MR-task-open" open MR with task markdown + When I visit project "Shop" merge requests page + Then I should see the task status for merge request "MR-task-open" + + # Toggling task items + + @javascript + Scenario: Task checkboxes should be enabled for an open merge request + Given project "Shop" has "MR-task-open" open MR with task markdown + When I visit merge request page "MR-task-open" + Then Task checkboxes should be enabled + + @javascript + Scenario: Task checkboxes should be disabled for a closed merge request + Given project "Shop" has "MR-task-open" open MR with task markdown + And I visit merge request page "MR-task-open" + And I click link "Close" + Then Task checkboxes should be disabled diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb index b55b3c6c8a2..26c3c7c14bc 100644 --- a/features/steps/project/issues/issues.rb +++ b/features/steps/project/issues/issues.rb @@ -153,6 +153,32 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps author: project.users.first) end + step 'project "Shop" has "Tasks-open" open issue with task markdown' do + desc_text = <