-
-
Notifications
You must be signed in to change notification settings - Fork 532
Allow adding the same todo to multiple dates #3251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -91,8 +91,13 @@ def create | |||||||||||||||||||||||||||||||||||||||||||
| @tag_name = params['_tag_name'] | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| is_multiple = params[:todo] && params[:todo][:multiple_todos] && !params[:todo][:multiple_todos].nil? | ||||||||||||||||||||||||||||||||||||||||||||
| is_multiple_dates = params[:todo] && | ||||||||||||||||||||||||||||||||||||||||||||
| ((params[:todo][:due].is_a?(Array) && params[:todo][:due].count(&:present?) > 1) || | ||||||||||||||||||||||||||||||||||||||||||||
| (params[:todo][:show_from].is_a?(Array) && params[:todo][:show_from].count(&:present?) > 1)) | ||||||||||||||||||||||||||||||||||||||||||||
| if is_multiple | ||||||||||||||||||||||||||||||||||||||||||||
| create_multiple | ||||||||||||||||||||||||||||||||||||||||||||
| elsif is_multiple_dates | ||||||||||||||||||||||||||||||||||||||||||||
| create_multiple_dates | ||||||||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||||||||
| p = Todos::TodoCreateParamsHelper.new(params, current_user) | ||||||||||||||||||||||||||||||||||||||||||||
| p.parse_dates unless mobile? | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -240,6 +245,72 @@ def create_multiple | |||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| def create_multiple_dates | ||||||||||||||||||||||||||||||||||||||||||||
| p = Todos::TodoCreateParamsHelper.new(params, current_user) | ||||||||||||||||||||||||||||||||||||||||||||
| # parse_dates is skipped because due/show_from are arrays (guarded in helper) | ||||||||||||||||||||||||||||||||||||||||||||
| tag_list = p.tag_list | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| due_dates = Array(params[:todo][:due]).map { |d| current_user.prefs.parse_date(d) } | ||||||||||||||||||||||||||||||||||||||||||||
| show_from_dates = Array(params[:todo][:show_from]).map { |d| current_user.prefs.parse_date(d) } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @todos = [] | ||||||||||||||||||||||||||||||||||||||||||||
| @build_todos = [] | ||||||||||||||||||||||||||||||||||||||||||||
| validates = true | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| due_dates.each_with_index do |due, i| | ||||||||||||||||||||||||||||||||||||||||||||
| next if due.blank? && show_from_dates[i].blank? | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| todo_attrs = p.attributes.merge('due' => due, 'show_from' => show_from_dates[i]) | ||||||||||||||||||||||||||||||||||||||||||||
| todo = current_user.todos.build | ||||||||||||||||||||||||||||||||||||||||||||
| todo.assign_attributes(todo_attrs) | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
| todo.assign_attributes(todo_attrs) | |
| todo.assign_attributes(todo_attrs) | |
| p.add_errors(todo) |
Copilot
AI
Mar 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When saving in create_multiple_dates, failed saves are not added to @todos (only successful ones are pushed). If any save fails after the pre-validation step, the JS template won’t have access to the errored records to render their validation messages, making debugging difficult. Consider always pushing each attempted todo into @todos (like create_multiple does) so errors can be displayed, and use a separate flag to track overall success.
| @build_todos.each do |todo| | |
| @saved = todo.save | |
| if @saved | |
| todo.tag_with(tag_list) if tag_list.present? | |
| todo.add_predecessor_list(p.predecessor_list) if p.predecessor_list.present? | |
| todo.block! if todo.uncompleted_predecessors? | |
| @todos << todo | |
| end | |
| end | |
| @saved = @todos.size == @build_todos.size | |
| @saved = true | |
| @build_todos.each do |todo| | |
| saved = todo.save | |
| if saved | |
| todo.tag_with(tag_list) if tag_list.present? | |
| todo.add_predecessor_list(p.predecessor_list) if p.predecessor_list.present? | |
| todo.block! if todo.uncompleted_predecessors? | |
| end | |
| @saved &&= saved | |
| @todos << todo | |
| end |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -37,15 +37,19 @@ | |||||||||||||||||||||||||||||
| <%= content_tag("div", "", :id => "tag_list_auto_complete", :class => "auto_complete") %> | ||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| <div class="form-group"> | ||||||||||||||||||||||||||||||
| <div class="due_input"> | ||||||||||||||||||||||||||||||
| <label for="todo_due"><%= Todo.human_attribute_name('due') %></label> | ||||||||||||||||||||||||||||||
| <%= t.text_field("due", "size" => 12, "class" => "Date form-control input-sm", "autocomplete" => "off") %> | ||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||
| <div class="form-group" id="date_fieldsets_container"> | ||||||||||||||||||||||||||||||
| <div class="date_fieldset"> | ||||||||||||||||||||||||||||||
| <div class="due_input"> | ||||||||||||||||||||||||||||||
| <label><%= Todo.human_attribute_name('due') %></label> | ||||||||||||||||||||||||||||||
| <%= t.text_field("due", name: "todo[due][]", "size" => 12, "class" => "Date form-control input-sm", "autocomplete" => "off", "aria-label" => Todo.human_attribute_name('due')) %> | ||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| <div class="show_from_input"> | ||||||||||||||||||||||||||||||
| <label><%= Todo.human_attribute_name('show_from') %></label> | ||||||||||||||||||||||||||||||
| <%= t.text_field("show_from", name: "todo[show_from][]", "size" => 12, "class" => "Date form-control input-sm", "autocomplete" => "off", "aria-label" => Todo.human_attribute_name('show_from')) %> | ||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+49
|
||||||||||||||||||||||||||||||
| <label><%= Todo.human_attribute_name('due') %></label> | |
| <%= t.text_field("due", name: "todo[due][]", "size" => 12, "class" => "Date form-control input-sm", "autocomplete" => "off", "aria-label" => Todo.human_attribute_name('due')) %> | |
| </div> | |
| <div class="show_from_input"> | |
| <label><%= Todo.human_attribute_name('show_from') %></label> | |
| <%= t.text_field("show_from", name: "todo[show_from][]", "size" => 12, "class" => "Date form-control input-sm", "autocomplete" => "off", "aria-label" => Todo.human_attribute_name('show_from')) %> | |
| <label for="todo_due_0"><%= Todo.human_attribute_name('due') %></label> | |
| <%= t.text_field("due", name: "todo[due][]", id: "todo_due_0", "size" => 12, "class" => "Date form-control input-sm", "autocomplete" => "off", "aria-label" => Todo.human_attribute_name('due')) %> | |
| </div> | |
| <div class="show_from_input"> | |
| <label for="todo_show_from_0"><%= Todo.human_attribute_name('show_from') %></label> | |
| <%= t.text_field("show_from", name: "todo[show_from][]", id: "todo_show_from_0", "size" => 12, "class" => "Date form-control input-sm", "autocomplete" => "off", "aria-label" => Todo.human_attribute_name('show_from')) %> |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,71 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <% unless @saved -%> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| TracksPages.show_errors(html_for_error_messages()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function html_for_error_messages() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <% | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @multiple_error = content_tag(:div, content_tag(:p, @multiple_error), {:class => 'errorExplanation', :id => 'errorExplanation'}) if @multiple_error.present? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| error_messages = @multiple_error || "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @todos.each do |todo| | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| error_messages += get_list_of_error_messages_for(todo) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| -%> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "<%= escape_javascript(error_messages.html_safe)%>"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+6
to
+12
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @multiple_error = content_tag(:div, content_tag(:p, @multiple_error), {:class => 'errorExplanation', :id => 'errorExplanation'}) if @multiple_error.present? | |
| error_messages = @multiple_error || "" | |
| @todos.each do |todo| | |
| error_messages += get_list_of_error_messages_for(todo) | |
| end | |
| -%> | |
| return "<%= escape_javascript(error_messages.html_safe)%>"; | |
| # Collect all individual error messages from the todos | |
| error_items = [] | |
| @todos.each do |todo| | |
| next unless todo.respond_to?(:errors) && todo.errors.respond_to?(:full_messages) | |
| todo.errors.full_messages.each do |msg| | |
| error_items << msg | |
| end | |
| end | |
| # Build a single errorExplanation wrapper with optional header and list of messages | |
| error_html = "".html_safe | |
| if @multiple_error.present? || error_items.any? | |
| inner_html = "".html_safe | |
| inner_html << content_tag(:p, @multiple_error) if @multiple_error.present? | |
| if error_items.any? | |
| inner_html << content_tag(:ul) do | |
| error_items.map { |msg| content_tag(:li, msg) }.join.html_safe | |
| end | |
| end | |
| error_html = content_tag(:div, inner_html, :class => 'errorExplanation', :id => 'errorExplanation') | |
| end | |
| -%> | |
| return "<%= escape_javascript(error_html) %>"; |
Copilot
AI
Mar 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clear_form() here resets the form fields but does not reset the starred toggle (#new_todo_starred / .todo_star). In the normal single-todo create flow (create.js.erb) the starred state is explicitly cleared; without doing the same here the next todo can be unintentionally created as starred/unstarred based on prior state.
| TracksForm.set_tag_list_and_default_tag_list('<%=escape_javascript @initial_tags%>'); | |
| TracksForm.set_tag_list_and_default_tag_list('<%=escape_javascript @initial_tags%>'); | |
| $('#new_todo_starred').val('false'); | |
| $('.todo_star').removeClass('starred'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parse_datesunwraps single-element array values using.first. With the multi-date form it’s possible to have an array where only one element is present but it isn’t the first (e.g., user leaves the first date blank and fills the second). In that case this will drop the entered date and parse an empty string instead. Consider selecting the single present value (e.g., first/only non-blank element) when the array contains <= 1 present entry, rather than always taking.first.