DEV: Use discourse tags instead of "labels"#19
Conversation
SamSaffron
force-pushed
the
labels-to-tags
branch
from
2bac3b9 to
b89052d
Compare
martin-brennan
left a comment
martin-brennan
left a comment
There was a problem hiding this comment.
Looks pretty good so far, I just pushed some spec improvements. @SamSaffron I made comments on stuff you added too here I think, not sure which is yours and which is from @jordanvidrine
| .group_by(&:topic_id) | ||
| end | ||
|
|
||
| def preload_floater_card_tags(cards) |
There was a problem hiding this comment.
We should only do this when SiteSetting.tagging_enabled is true, otherwise return {}
| normalized_tag_ids = | ||
| raw_values | ||
| .filter_map do |value| | ||
| next if value.blank? | ||
| value.is_a?(String) ? Integer(value, 10) : Integer(value) | ||
| rescue ArgumentError, TypeError | ||
| raise Discourse::InvalidParameters.new( | ||
| I18n.t( | ||
| "discourse_kanban.errors.unknown_tag_ids", | ||
| tag_ids: raw_values.join(","), | ||
| ), | ||
| ) | ||
| end | ||
| .uniq |
There was a problem hiding this comment.
This ruby is a bit weird, should be more like this:
| normalized_tag_ids = | |
| raw_values | |
| .filter_map do |value| | |
| next if value.blank? | |
| value.is_a?(String) ? Integer(value, 10) : Integer(value) | |
| rescue ArgumentError, TypeError | |
| raise Discourse::InvalidParameters.new( | |
| I18n.t( | |
| "discourse_kanban.errors.unknown_tag_ids", | |
| tag_ids: raw_values.join(","), | |
| ), | |
| ) | |
| end | |
| .uniq | |
| normalized_tag_ids = | |
| raw_values | |
| .compact | |
| .map(&:to_i) | |
| .uniq |
This will get rid of blank values early, and we don't need this odd way of converting to integer
| normalized_tag_ids = Array(tag_ids).map(&:to_i).reject(&:zero?).uniq | ||
| return [] if normalized_tag_ids.blank? |
There was a problem hiding this comment.
Should use normalize_tag_ids! here
| normalized_tag_ids = Array(tag_ids).map(&:to_i).reject(&:zero?).uniq | ||
| return [] if normalized_tag_ids.blank? |
There was a problem hiding this comment.
Should use normalize_tag_ids! here
| def tags | ||
| return [] if object.topic? | ||
|
|
||
| ordered_tags.map { |tag| CardTagSerializer.new(tag, root: false, scope: scope).as_json } | ||
| end |
There was a problem hiding this comment.
A nicer way to do this would be to use has_many :tags, embed: :object, serializer: CardTagSerializer at the top of this serializer, then adding changing this method like so:
| def tags | |
| return [] if object.topic? | |
| ordered_tags.map { |tag| CardTagSerializer.new(tag, root: false, scope: scope).as_json } | |
| end | |
| def tags | |
| object.topic? ? [] : ordered_tags | |
| end |
| guardian:, | ||
| params: card_mutation_params.to_h.merge("board_id" => params[:board_id]), | ||
| constraint_fix: constraint_fix_params, | ||
| **service_params.deep_merge( |
There was a problem hiding this comment.
| **service_params.deep_merge( | |
| service_params.deep_merge( |
** not necessary, the service_params is a hash not kwargs
| params: raw_params.merge("board_id" => params[:board_id], "id" => params[:id]), | ||
| raw_card_params: raw_params, | ||
| constraint_fix: constraint_fix_params, | ||
| **service_params.deep_merge( |
There was a problem hiding this comment.
| **service_params.deep_merge( | |
| service_params.deep_merge( |
** not necessary, the service_params is a hash not kwargs
| board_id: params[:board_id], | ||
| column_id: params[:column_id], | ||
| }, | ||
| **service_params.deep_merge( |
There was a problem hiding this comment.
| **service_params.deep_merge( | |
| service_params.deep_merge( |
** not necessary, the service_params is a hash not kwargs
| board_id: params[:board_id], | ||
| id: params[:id], | ||
| }, | ||
| **service_params.deep_merge(params: { board_id: params[:board_id], id: params[:id] }), |
There was a problem hiding this comment.
| **service_params.deep_merge(params: { board_id: params[:board_id], id: params[:id] }), | |
| service_params.deep_merge( |
** not necessary, the service_params is a hash not kwargs
| params: card_mutation_params.to_h.merge("board_id" => params[:board_id]), | ||
| constraint_fix: constraint_fix_params, | ||
| **service_params.deep_merge( | ||
| params: card_mutation_params.to_h.merge("board_id" => params[:board_id]), |
There was a problem hiding this comment.
We are inconsistent with merging string key hashes and symbol key hashes like DestroyCard, we should standardize on the latter
Fixes a few small issues as well.
SamSaffron
force-pushed
the
labels-to-tags
branch
from
df4655b to
477e28d
Compare
3 participants
Replace custom labels system with Discourse's native tag chooser — cards now store tags instead of freeform labels, which carry over to the composer when promoting a card to a topic.
cards-with-tags.mp4