Skip to content
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

Bug/59374 regression missing commit message comments in work packages after upgrade to 1501 #17594

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/components/_index.sass
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
@import "work_packages/activities_tab/journals/new_component"
@import "work_packages/activities_tab/journals/index_component"
@import "work_packages/activities_tab/journals/item_component"
@import "work_packages/activities_tab/journals/revision_component"
@import "work_packages/activities_tab/journals/item_component/details"
@import "work_packages/activities_tab/journals/item_component/add_reactions"
@import "work_packages/activities_tab/journals/item_component/reactions"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
mb: inner_container_margin_bottom
) do
flex_layout(id: insert_target_modifier_id,
data: { test_selector: "op-wp-journals-container" }) do |journals_index_container|
data: { test_selector: "op-wp-journals-container" }) do |journals_index_container|
if empty_state?
journals_index_container.with_row(mt: 2, mb: 3) do
render(
Expand All @@ -22,12 +22,16 @@
end
end

recent_journals.each do |journal|
recent_journals.each do |record|
journals_index_container.with_row do
render(WorkPackages::ActivitiesTab::Journals::ItemComponent.new(
journal:, filter:,
grouped_emoji_reactions: wp_journals_grouped_emoji_reactions[journal.id]
))
if record.is_a?(Changeset)
render(WorkPackages::ActivitiesTab::Journals::RevisionComponent.new(changeset: record, filter:))
else
render(WorkPackages::ActivitiesTab::Journals::ItemComponent.new(
journal: record, filter:,
grouped_emoji_reactions: wp_journals_grouped_emoji_reactions[record.id]
))
end
end
end

Expand All @@ -48,12 +52,16 @@
else
helpers.turbo_frame_tag("work-package-activities-tab-content-older-journals") do
flex_layout do |older_journals_container|
older_journals.each do |journal|
older_journals.each do |record|
older_journals_container.with_row do
render(WorkPackages::ActivitiesTab::Journals::ItemComponent.new(
journal:, filter:,
grouped_emoji_reactions: wp_journals_grouped_emoji_reactions[journal.id]
))
if record.is_a?(Changeset)
render(WorkPackages::ActivitiesTab::Journals::RevisionComponent.new(changeset: record, filter:))
else
render(WorkPackages::ActivitiesTab::Journals::ItemComponent.new(
journal: record, filter:,
grouped_emoji_reactions: wp_journals_grouped_emoji_reactions[record.id]
))
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,48 +63,71 @@
journal_sorting == "desc"
end

def base_journals
work_package
.journals
.includes(
:user,
:customizable_journals,
:attachable_journals,
:storable_journals,
:notifications
)
.reorder(version: journal_sorting)
.with_sequence_version
# Get journals with eager loading
journals = API::V3::Activities::ActivityEagerLoadingWrapper.wrap(
work_package
.journals
.includes(
:user,
:customizable_journals,
:attachable_journals,
:storable_journals,
:notifications
)
.reorder(version: journal_sorting)
.with_sequence_version
)

# Get associated revisions
revisions = work_package.changesets.includes(:user, :repository)

# Combine and sort them by date
if journal_sorting_desc?
(journals + revisions).sort_by do |record|
timestamp = if record.is_a?(API::V3::Activities::ActivityEagerLoadingWrapper)
record.created_at&.to_i
elsif record.is_a?(Changeset)
record.committed_on.to_i
end
[-timestamp, -record.id]
end
else
(journals + revisions).sort_by do |record|
timestamp = if record.is_a?(API::V3::Activities::ActivityEagerLoadingWrapper)
record.created_at&.to_i
elsif record.is_a?(Changeset)
record.committed_on.to_i
end
[timestamp, record.id]
end
end
end

Check notice on line 105 in app/components/work_packages/activities_tab/journals/index_component.rb

View workflow job for this annotation

GitHub Actions / rubocop

[rubocop] app/components/work_packages/activities_tab/journals/index_component.rb#L66-L105 <Metrics/AbcSize>

Assignment Branch Condition size for base_journals is too high. [<6, 31, 10> 33.12/17]
Raw output
app/components/work_packages/activities_tab/journals/index_component.rb:66:9: C: Metrics/AbcSize: Assignment Branch Condition size for base_journals is too high. [<6, 31, 10> 33.12/17]

Check notice on line 105 in app/components/work_packages/activities_tab/journals/index_component.rb

View workflow job for this annotation

GitHub Actions / rubocop

[rubocop] app/components/work_packages/activities_tab/journals/index_component.rb#L66-L105 <Metrics/PerceivedComplexity>

Perceived complexity for base_journals is too high. [13/8]
Raw output
app/components/work_packages/activities_tab/journals/index_component.rb:66:9: C: Metrics/PerceivedComplexity: Perceived complexity for base_journals is too high. [13/8]

def journals
API::V3::Activities::ActivityEagerLoadingWrapper.wrap(base_journals)
base_journals
end

def recent_journals
recent_ones = if journal_sorting_desc?
base_journals.first(MAX_RECENT_JOURNALS)
else
base_journals.last(MAX_RECENT_JOURNALS)
end

API::V3::Activities::ActivityEagerLoadingWrapper.wrap(recent_ones)
if journal_sorting_desc?
base_journals.first(MAX_RECENT_JOURNALS)
else
base_journals.last(MAX_RECENT_JOURNALS)
end
end

def older_journals
older_ones = if journal_sorting_desc?
base_journals.offset(MAX_RECENT_JOURNALS)
else
total = base_journals.count
limit = [total - MAX_RECENT_JOURNALS, 0].max
base_journals.limit(limit)
end

API::V3::Activities::ActivityEagerLoadingWrapper.wrap(older_ones)
if journal_sorting_desc?
base_journals.drop(MAX_RECENT_JOURNALS)
else
base_journals.take(base_journals.size - MAX_RECENT_JOURNALS)
end
end

def journal_with_notes
base_journals.where.not(notes: "")
work_package
.journals
.where.not(notes: "")
end

def wp_journals_grouped_emoji_reactions
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<%=
component_wrapper(class: "work-packages-activities-tab-journals-item-component") do
flex_layout(data: {
test_selector: "op-wp-revision-entry-#{changeset.id}"
}) do |revision_container|
revision_container.with_row do
render(border_box_container(
id: "activity-anchor-r#{changeset.revision}",
padding: :condensed,
"aria-label": I18n.t("activities.work_packages.activity_tab.commented")
)) do |border_box_component|
border_box_component.with_header(px: 2, py: 1, data: { test_selector: "op-revision-header" }) do
flex_layout(align_items: :center, justify_content: :space_between, classes: "work-packages-activities-tab-revision-component--header") do |header_container|
header_container.with_column(flex_layout: true,
classes: "work-packages-activities-tab-journals-item-component--header-start-container ellipsis") do |header_start_container|
header_start_container.with_column(mr: 2) do
if changeset.user
render(Users::AvatarComponent.new(user: changeset.user, show_name: false, size: :mini))
end
end
header_start_container.with_column(mr: 1, flex_layout: true,
classes: "work-packages-activities-tab-journals-item-component--user-name-container hidden-for-desktop") do |user_name_container|
user_name_container.with_row(classes: "work-packages-activities-tab-journals-item-component--user-name ellipsis") do
render_user_name
end
user_name_container.with_row do
render(Primer::Beta::Text.new(font_size: :small, color: :subtle, mr: 1)) do
committed_text = render(Primer::Beta::Link.new(
href: revision_url,
scheme: :secondary,
underline: false,
font_size: :small,
target: "_blank"
)) do
I18n.t("js.label_committed_link", revision_identifier: short_revision)
end
I18n.t("js.label_committed_at",
committed_revision_link: committed_text.html_safe,
date: format_time(changeset.committed_on)).html_safe
end
end
end
header_start_container.with_column(mr: 1,
classes: "work-packages-activities-tab-journals-item-component--user-name ellipsis hidden-for-mobile") do
render_user_name
end
header_start_container.with_column(mr: 1, classes: "hidden-for-mobile") do
render(Primer::Beta::Text.new(font_size: :small, color: :subtle, mr: 1)) do
committed_text = render(Primer::Beta::Link.new(
href: revision_url,
scheme: :secondary,
underline: false,
font_size: :small,
target: "_blank"
)) do
I18n.t("js.label_committed_link", revision_identifier: short_revision)
end
I18n.t("js.label_committed_at",
committed_revision_link: committed_text.html_safe,
date: format_time(changeset.committed_on)).html_safe
end
end
end
end
end
border_box_component.with_body(
classes: "work-packages-activities-tab-journals-item-component--journal-notes-body",
data: { test_selector: "op-revision-notes-body" }
) do
render(Primer::Box.new(mt: 1, classes: "op-uc-container")) do
format_text(changeset, :comments)
end
end
end
end
revision_container.with_row(flex_layout: true, classes: "work-packages-activities-tab-revision-component--stem-line-container") do |stem_line_container|
stem_line_container.with_column(border: :left, classes: "work-packages-activities-tab-revision-component--stem-line")
end
end
end
%>
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
module WorkPackages

Check notice on line 1 in app/components/work_packages/activities_tab/journals/revision_component.rb

View workflow job for this annotation

GitHub Actions / rubocop

[rubocop] app/components/work_packages/activities_tab/journals/revision_component.rb#L1 <Style/FrozenStringLiteralComment>

Missing magic comment `# frozen_string_literal: true`.
Raw output
app/components/work_packages/activities_tab/journals/revision_component.rb:1:1: C: Style/FrozenStringLiteralComment: Missing magic comment `# frozen_string_literal: true`.
module ActivitiesTab
module Journals
class RevisionComponent < ApplicationComponent
include ApplicationHelper
include OpPrimer::ComponentHelpers
include OpTurbo::Streamable

def initialize(changeset:, filter:)
super

@changeset = changeset
@filter = filter
end

private

attr_reader :changeset, :filter

def render?
filter != :only_comments
end

def user_name
if changeset.user
changeset.user.name
else
# Extract name from committer string (format: "name <email>")
changeset.committer.split("<").first.strip
end
end

def revision_url
repository = changeset.repository
project = repository.project

show_revision_project_repository_path(project_id: project.id, rev: changeset.revision)
end

def short_revision
changeset.revision[0..7]
end

def copy_url_action_item(menu)
menu.with_item(label: t("button_copy_link_to_clipboard"),
tag: :button,
content_arguments: {
data: {
action: "click->work-packages--activities-tab--item#copyActivityUrlToClipboard"
}
}) do |item|
item.with_leading_visual_icon(icon: :copy)
end
end

def render_user_name
if changeset.user
render_user_link(changeset.user)
else
render_committer_name(changeset.committer)
end
end

def render_user_link(user)
render(Primer::Beta::Link.new(
href: user_url(user),
target: "_blank",
scheme: :primary,
underline: false,
font_weight: :bold
)) do
changeset.user.name
end
end

def render_committer_name(committer)
render(Primer::Beta::Text.new(font_weight: :bold, mr: 1)) do
committer.gsub(%r{<.+@.+>}, "").strip

Check failure

Code scanning / CodeQL

Incomplete multi-character sanitization High

This string may still contain
<script
, which may cause an HTML element injection vulnerability.

Copilot Autofix AI 5 days ago

To fix the problem, we should ensure that all potentially dangerous HTML tags are removed from the committer string. The best way to achieve this is to use a well-tested sanitization library, such as the sanitize gem, which is designed to handle various edge cases and ensure effective sanitization.

  1. Install the sanitize gem if it is not already included in the project.
  2. Update the render_committer_name method to use the sanitize gem to remove any potentially dangerous HTML tags from the committer string.
Suggested changeset 1
app/components/work_packages/activities_tab/journals/revision_component.rb

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/components/work_packages/activities_tab/journals/revision_component.rb b/app/components/work_packages/activities_tab/journals/revision_component.rb
--- a/app/components/work_packages/activities_tab/journals/revision_component.rb
+++ b/app/components/work_packages/activities_tab/journals/revision_component.rb
@@ -1 +1,3 @@
+require 'sanitize'
+
 module WorkPackages
@@ -77,3 +79,3 @@
           render(Primer::Beta::Text.new(font_weight: :bold, mr: 1)) do
-            committer.gsub(%r{<.+@.+>}, "").strip
+            Sanitize.fragment(committer.gsub(%r{<.+@.+>}, "").strip)
           end
EOF
@@ -1 +1,3 @@
require 'sanitize'

module WorkPackages
@@ -77,3 +79,3 @@
render(Primer::Beta::Text.new(font_weight: :bold, mr: 1)) do
committer.gsub(%r{<.+@.+>}, "").strip
Sanitize.fragment(committer.gsub(%r{<.+@.+>}, "").strip)
end
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjabari-op - this looks sensible, wdyt?

end
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.work-packages-activities-tab-revision-component
&--header
min-height: 32px
&--stem-line-container
min-height: 20px
&--stem-line
margin-left: 19px
Loading
Loading