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

Activity filters #1207

Open
wants to merge 18 commits into
base: develop
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
5 changes: 2 additions & 3 deletions CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
[v#.#.#] ([month] [YYYY])
- [entity]:
- [future tense verb] [feature]
- Activity: Filter activities by user, type, and date
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Activity: Filter activities by user, type, and date
- Activities: Filter activities by user, type, and date

- Upgraded gems:
- [gem]
- Bugs fixes:
Expand Down Expand Up @@ -70,7 +69,7 @@ v4.11.0 (January 2024)
- Qualys: Add support for the output for Qualys WAS API 3.13 and later
- Security Fixes:
- Low: Authenticated (author) information disclosure
- After a user has been removed from a project, they may still get
- After a user has been removed from a project, they may still get
notifications for Issues they were subscribed to, resulting in the
disclosure of Issue titles.
- Low: Authenticated (author) information disclosure in the output console of upload manager
Expand Down
15 changes: 15 additions & 0 deletions app/assets/javascripts/tylium/pages/activities.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,19 @@ document.addEventListener('turbolinks:load', function() {
}
});
}

$('[data-behavior~=activity-filters] select').change(function () {
$(this).closest('form').submit();
});

MattBudz marked this conversation as resolved.
Show resolved Hide resolved
$('[data-behavior~=activity-filters] input[type="date"]').change(function () {
const dateValue = $(this).val();
const periodStart = $('[data-behavior~=since]').attr('min');
if (
(dateValue !== '' && !isNaN(Date.parse(dateValue))) &&
(Date.parse(dateValue) >= Date.parse(periodStart))
) {
$(this).closest('form').submit();
}
});
});
40 changes: 36 additions & 4 deletions app/controllers/activities_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ class ActivitiesController < AuthenticatedController

def index
@page = params[:page].present? ? params[:page].to_i : 1
@users_for_select = current_project.activities.map(&:user).union(current_project.authors.enabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to include users that have no activities?

Copy link
Contributor

Choose a reason for hiding this comment

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

With the first part of this query we're also including users who have activities in the project even if they're no longer on the project. Do we want to include these?


activities = current_project.activities
.includes(:trackable)
.order(created_at: :desc)
.page(@page)
activities = current_project.activities.includes(:trackable)
activities = filter_activities(activities)
activities = activities.order(created_at: :desc).page(@page)

@activities_groups = activities.group_by do |activity|
activity.created_at.strftime(Activity::ACTIVITIES_STRFTIME_FORMAT)
Expand All @@ -30,4 +30,36 @@ def poll
format.js
end
end

private

def filtering_params
Copy link
Contributor

Choose a reason for hiding this comment

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

we're not using this anywhere but probably a good idea

params.permit(:user_id, :trackable_type, :since, :before)
end

def filter_activities(activities)
if params[:since].present? && valid_date?(params[:since])
activities = activities.since(DateTime.parse(params[:since]).beginning_of_day)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need beginning_of_day since DateTime.parse on a date like 2024-08-01 will return beginning of day: Thu, 01 Aug 2024 00:00:00 +0000

end

if params[:before].present? && valid_date?(params[:before])
activities = activities.before(DateTime.parse(params[:before]).end_of_day)
end

if params[:user_id].present?
activities = activities.where(user_id: params[:user_id])
end

if params[:trackable_type].present?
activities = activities.where(trackable_type: params[:trackable_type])
end

activities
end

def valid_date?(date_string)
Date.parse(date_string)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will allow dates in the future. Do we want to prevent this?

rescue ArgumentError
false
end
end
3 changes: 3 additions & 0 deletions app/models/activity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ def project=(new_project); end
includes(:trackable).order('activities.created_at DESC').limit(20)
end

scope :since, -> (period_start) { where('created_at >= ?', period_start) }
scope :before, -> (period_end) { where('created_at <= ?', period_end) }

# -- Callbacks ------------------------------------------------------------

# Cast action to a string so the 'inclusion' validation works with symbols
Expand Down
29 changes: 29 additions & 0 deletions app/views/activities/_filters.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<%= form_with url: project_activities_path(current_project), method: :get, remote: true, class: 'd-flex flex-xl-row flex-column gap-3', data: { behavior: 'activity-filters' } do |f| %>
<div class="d-flex flex-column flex-sm-row w-100 gap-3">
<div class="w-100">
<%= f.label :user_id, class: 'form-label' %>
<%= f.select :user_id, options_from_collection_for_select(@users_for_select, :id, :name, params[:user_id]), { include_blank: 'All users' }, { class: 'form-select' } %>
</div>
<div class="w-100">
<%= f.label :trackable_type, class: 'form-label' %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we label this something more user-friendly?

<%= f.select :trackable_type, options_from_collection_for_select(current_project.activities.pluck(:trackable_type).uniq, :to_s, :demodulize, params[:trackable_type] ), { include_blank: 'All types', selected: params[:type] }, { class: 'form-select' } %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get the collection of trackable types in the controller?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<%= f.select :trackable_type, options_from_collection_for_select(current_project.activities.pluck(:trackable_type).uniq, :to_s, :demodulize, params[:trackable_type] ), { include_blank: 'All types', selected: params[:type] }, { class: 'form-select' } %>
<%= f.select :trackable_type, options_from_collection_for_select(current_project.activities.pluck(:trackable_type).uniq, :to_s, :demodulize, params[:trackable_type] ), { include_blank: 'All types' }, { class: 'form-select' } %>

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we split this across multiple lines? ditto for the other fields

</div>
</div>

<div class="w-100">
<span class="form-label mb-2 d-block">Date range</span>
<div class="d-flex flex-column flex-sm-row gap-3 w-100">
<div class="input-group">
<%= f.date_field :since, max: params[:before] || Date.today, min: "01-01-2016", placeholder: 'From date', value: params[:since], class: 'form-control', data: { behavior: 'since' } %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for the min date choice?

<%= f.label :since, class: 'visually-hidden' %>
<span class="input-group-text">to</span>
<%= f.label :before, class: 'visually-hidden' %>
<%= f.date_field :before, max: Date.today, min: params[:since], placeholder: 'To date', value: params[:before] || Date.today, class: 'form-control' %>
</div>

<%= link_to project_activities_path(current_project.id), class: 'text-error-hover text-nowrap d-flex align-items-center' do %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<%= link_to project_activities_path(current_project.id), class: 'text-error-hover text-nowrap d-flex align-items-center' do %>
<%= link_to project_activities_path(current_project), class: 'text-error-hover text-nowrap d-flex align-items-center' do %>

<i class="fa-solid fa-xmark me-2"></i> Clear filters
<% end %>
</div>
</div>
<% end %>
4 changes: 4 additions & 0 deletions app/views/activities/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
</nav>
<% end %>

<div class="content-container mt-3 py-3">
<%= render 'activities/filters' %>
</div>

<div class="content-container mt-3 <%= 'pt-3' if @activities_groups.any? %>">
<% if @activities_groups.any? %>
<div class="container">
Expand Down
109 changes: 100 additions & 9 deletions spec/features/activity_pages/activity_pages_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,38 @@
context 'as authenticated user' do
describe 'index page', js: true do
let(:trackable) { create(:issue) }
let(:trackable_card) { create(:card) }
let(:user) { create(:user) }
let(:second_user) { create(:user) }
let(:project) { current_project }

skliask marked this conversation as resolved.
Show resolved Hide resolved
let(:create_activities) do
50.times do
activity = Activity.create(
user: user,
trackable_type: trackable.class,
trackable_id: trackable.id,
action: 'update',
created_at: Time.current - ((1..5).to_a.sample.days)
)
end
create_list(:activity, 35,
user: user,
trackable_type: trackable.class,
trackable_id: trackable.id,
action: 'update',
project: project,
created_at: Time.current - 1.month
)

create_list(:activity, 10,
user: second_user,
trackable_type: trackable_card.class,
trackable_id: trackable_card.id,
action: 'update',
project: project,
created_at: Time.current
)

create_list(:activity, 5,
user: user,
trackable_type: trackable_card.class,
trackable_id: trackable_card.id,
action: 'update',
project: project,
created_at: Time.current - 2.days
)
end

before do
Expand Down Expand Up @@ -75,6 +95,77 @@
end
end
end

describe 'filters' do
aapomm marked this conversation as resolved.
Show resolved Hide resolved
context 'valid filter options' do
it 'has user filter' do
expect(page).to have_selector('#user_id', count: 1)
end

it 'by user' do
visit project_activities_path(current_project, user_id: second_user.id)
expect(all('.activity').count).to eq(10)
end

it 'has type filter' do
expect(page).to have_selector('#trackable_type', count: 1)
end

it 'by trackable_type' do
visit project_activities_path(current_project, trackable_type: 'Card')
expect(page).to have_selector('.activity', count: 15)
end

it 'has daterange filter' do
expect(page).to have_selector('#since', count: 1)
expect(page).to have_selector('#before', count: 1)
end

it 'combined filter works' do
visit project_activities_path(current_project, trackable_type: 'Card', user_id: second_user.id)
expect(page).to have_selector('.activity', count: 10)
end

it 'filters by date' do
since = Time.current - 3.days
before = Time.current

visit project_activities_path(current_project, since: since, before: before)
expect(page).to have_selector('.activity', count: 15)
end

it 'filters for single day' do
visit project_activities_path(current_project, since: Time.current, before: Time.current)
expect(page).to have_selector('.activity', count: 10)
end
end

context 'invalid filter options' do
context 'invalid dates' do
it 'returns no activities' do
since = Time.current + 3.days
before = Time.current

visit project_activities_path(current_project, since: since, before: before)
expect(page).to_not have_selector('.activity')
end
end

context 'invalid trackable type' do
it 'returns no activities' do
visit project_activities_path(current_project, trackable_type: 'InvalidType')
expect(page).to_not have_selector('.activity')
end
end

context 'invalid user' do
it 'returns no activities' do
visit project_activities_path(current_project, user_id: -1)
expect(page).to_not have_selector('.activity')
end
end
end
end
end
end
end
38 changes: 34 additions & 4 deletions spec/models/activity_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,48 @@

it { should validate_inclusion_of(:action).in_array %i[create update destroy] }

describe "#trackable=" do
context "when passed an Issue" do
it "sets trackable_type as Issue, not Note" do
describe '#trackable=' do
context 'when passed an Issue' do
it 'sets trackable_type as Issue, not Note' do
# Default Rails behaviour is to set trackable_type to 'Note' when you
# pass an Issue, meaning that it gets loaded as a Note, not an Issue,
# when you call #trackable later.
issue = create(:issue)
activity = create(:activity, trackable: issue)
expect(activity.trackable_type).to eq "Issue"
expect(activity.trackable_type).to eq 'Issue'
expect(activity.reload.trackable).to eq issue
end
end
end

describe 'filtering' do
before do
card = create(:card)
issue = create(:issue)
@user = create(:user)
activity = create(:activity, trackable: issue, user: @user, created_at: DateTime.now.beginning_of_year)
activity2 = create(:activity, trackable: card, user: @user, created_at: DateTime.now.beginning_of_year)
activity3 = create(:activity, trackable: card, created_at: DateTime.now.beginning_of_year - 1.year)
end

describe '#since' do
context 'when passed a valid date' do
it 'returns activities within the date' do
year_start = DateTime.now.beginning_of_year

expect(Activity.since(year_start).count).to eq 2
end
end
end

describe '#before' do
context 'when passed a valid date' do
it 'returns activities within the date' do
period_end = DateTime.now.end_of_day - 1.year

expect(Activity.before(period_end).count).to eq 1
end
end
end
end
end
Loading