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

Add confirm delete progress admin page #42170

Merged
merged 2 commits into from
Aug 31, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 15 additions & 8 deletions dashboard/app/controllers/admin_users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def manual_pass
redirect_to :manual_pass_form
end

# get /admin/user_progress
# GET /admin/user_progress
def user_progress_form
user_identifier = params[:user_identifier]
script_offset = params[:script_offset] || 0 # Not currently exposed in admin UI but can be manually added to URL
Expand All @@ -130,16 +130,23 @@ def user_progress_form
@user_scripts = UserScript.
where(user_id: @target_user.id).
order(updated_at: :desc).
limit(5).
limit(100).
offset(script_offset)
end
end

if @user_scripts
script_ids = @user_scripts.pluck(:script_id)
@user_levels = UserLevel.
where(user_id: @target_user.id, script_id: script_ids).
order(updated_at: :desc)
end
# GET /admin/delete_progress
# This page is linked from /admin/user_progress to confirm that the admin
# wants to delete progress and to capture additional information. It expects
# user_id and script_id to be passed in as parameters.
def delete_progress_form
params.require([:user_id, :script_id])

@target_user = User.find(params[:user_id])
@script = Script.get_from_cache(params[:script_id])
@user_level_count = UserLevel.
where(user_id: @target_user.id, script_id: @script.id).
count
end

# get /admin/permissions
Expand Down
24 changes: 24 additions & 0 deletions dashboard/app/views/admin_users/delete_progress_form.html.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
%h1 Delete User Progress

%p You are about to <strong>permanently</strong> delete progress (level status and source code) for:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also mention other records that will be deleted as side effects like teacher feedback (I forget if there are others)?


= form_tag(delete_progress_path, method: :post) do
=hidden_field_tag :user_id, @target_user.id
User:
%strong= @target_user.username
%br
=hidden_field_tag :script_id, @script.id
Script:
%strong= @script.name
%br
Levels affected:
%strong= @user_level_count
%br
%br
Reason:
%br
=text_area_tag :reason, '', placeholder: 'required'
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the placeholder says 'required', do we want to disable the submit button if no reason was given? I'm also wondering if it may be useful to record the id of the admin that deleted the progress?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good suggestion but I think I want to keep the haml as simple as possible for now (similar to the other admin pages). I will run through this scenario where the admin does not provide a reason once everything is hooked up end-to-end and make sure that the experience is reasonable for an admin. If it's really bad, I'll add this.

Yes, the id of the admin (the current logged in user) will be recorded in the firehose event.

%br
%br
=submit_tag 'Delete Progress'
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 a cancel button, or do you think it's obvious that you can just hit the browser back button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this and it didn't seem like other similar admin pages had a cancel button or link so I left it out?


29 changes: 4 additions & 25 deletions dashboard/app/views/admin_users/user_progress_form.html.haml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
%h1 User Progress

%p This page allows an admin to view recent progress for a user and reset progress for all levels in a script. All times in UTC.
%p This page allows an admin to view recent progress for a user and delete progress for all levels in a script. All times in UTC.

:ruby
def format_time(datetime)
Expand Down Expand Up @@ -31,7 +31,7 @@
%td= format_time(@target_user.current_sign_in_at)
%td= format_time(@target_user.created_at)

%h2 Scripts with most recent progress (sorted by updated_at, up to 5 shown)
%h2 Scripts with most recent progress (sorted by updated_at, up to 100 shown)
%p Data represents rows from the user_scripts table
%table.table.table-hover.table-condensed
%thead
Expand All @@ -42,6 +42,7 @@
%th Completed at
%th Assigned at
%th Last progress at
%th
%tbody
- @user_scripts.each do |user_script|
%tr
Expand All @@ -52,26 +53,4 @@
%td= format_time(user_script.completed_at)
%td= format_time(user_script.assigned_at)
%td= format_time(user_script.last_progress_at)

%h2 All levels with progress in the above scripts (sorted by updated_at)
%p Data represents rows from user_levels table
%table.table.table-hover.table-condensed
%thead
%th Id
%th Level path
%th Result
%th Att.
%th Time(s)
%th Created at
%th Updated at
%tbody
- @user_levels.each do |user_level|
%tr
%td= user_level.id
- script_level_path = build_script_level_path(user_level.script_level)
%td= link_to script_level_path, script_level_path
%td= user_level.best_result
%td= user_level.attempts
%td= user_level.time_spent
%td= format_time(user_level.created_at)
%td= format_time(user_level.updated_at)
%td= link_to 'Delete...', delete_progress_form_path(user_id: @target_user.id, script_id: user_script.script_id)
2 changes: 2 additions & 0 deletions dashboard/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,8 @@
post '/admin/studio_person_split', to: 'admin_users#studio_person_split', as: 'studio_person_split'
post '/admin/studio_person_add_email_to_emails', to: 'admin_users#studio_person_add_email_to_emails', as: 'studio_person_add_email_to_emails'
get '/admin/user_progress', to: 'admin_users#user_progress_form', as: 'user_progress_form'
get '/admin/delete_progress', to: 'admin_users#delete_progress_form', as: 'delete_progress_form'
post '/admin/delete_progress', to: 'admin_users#delete_progress', as: 'delete_progress'
get '/census/review', to: 'census_reviewers#review_reported_inaccuracies', as: 'review_reported_inaccuracies'
post '/census/review', to: 'census_reviewers#create'

Expand Down
46 changes: 34 additions & 12 deletions dashboard/test/controllers/admin_users_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -233,27 +233,49 @@ class AdminUsersControllerTest < ActionController::TestCase
user = @not_admin
script1 = create(:script, :with_levels, levels_count: 2)
script2 = create(:script, :with_levels, levels_count: 1)
level1 = script1.script_levels.first.level
level2 = script1.script_levels.second.level
level3 = script2.script_levels.first.level

UserScript.create! user: user, script: script1
UserScript.create! user: user, script: script2
UserLevel.create! user: user, script: script1, level: level1, best_result: 100
UserLevel.create! user: user, script: script1, level: level2, best_result: 100
UserLevel.create! user: user, script: script2, level: level3, best_result: 100
UserScript.create!(user: user, script: script1)
UserScript.create!(user: user, script: script2)

sign_in @admin
post :user_progress_form, params: {user_identifier: @not_admin.id.to_s}

# page has 3 tables:
# page has 2 tables:
# table 1 - user information (1 row)
# table 2 - script progress (2 rows)
# table 3 - level progress (3 rows)
assert_select "table", 3
assert_select "table", 2
assert_select "table:nth-of-type(1) tbody tr", 1
assert_select "table:nth-of-type(2) tbody tr", 2
assert_select "table:nth-of-type(3) tbody tr", 3
end

test "delete_progress_form returns error if not admin" do
sign_in @not_admin
get :delete_progress_form, params: {user_id: @user.id, script_id: @script.id}
assert_response :forbidden
end

test 'delete_progress_form returns correct data' do
sign_in @admin

user = create(:student, username: 'test_student')
script = create(:script, :with_levels, levels_count: 3)
level1 = script.script_levels.first.level
level2 = script.script_levels.second.level
level3 = script.script_levels.third.level

UserScript.create!(user: user, script: script)
UserLevel.create!(user: user, script: script, level: level1, best_result: 0)
UserLevel.create!(user: user, script: script, level: level2, best_result: 10)
UserLevel.create!(user: user, script: script, level: level3, best_result: 100)

get :delete_progress_form, params: {user_id: user.id, script_id: script.id}
assert_response :ok
assert_select "strong" do |elements|
assert_equal 4, elements.length
assert_equal user.username, elements[1].text
assert_equal script.name, elements[2].text
assert_equal 3, elements[3].text.to_i # count of user_level rows
end
end

generate_admin_only_tests_for :permissions_form
Expand Down