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

Feature/#160 remove files course overview #191

Merged
merged 22 commits into from
Jan 16, 2020
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
b087164
add prototype for filelist
adrianjost Jan 12, 2020
9060e5e
add download link
adrianjost Jan 12, 2020
6ebf02c
Tests for #155, remove double tests for show_courses
caustt Jan 13, 2020
b2d8ce6
Differntiate student and lecturer files and add scroll bar
caustt Jan 13, 2020
c295dde
Missing test case
caustt Jan 13, 2020
bd4bc23
Tests for #160 #156
caustt Jan 14, 2020
af06371
Implement Deleting of Course Files
caustt Jan 14, 2020
b7b0a0d
Validate right to delete files from courses
caustt Jan 14, 2020
496ff49
Merge branch 'dev' into feature/#160-remove-files-course-overview
caustt Jan 15, 2020
aed30b8
rubocop
caustt Jan 15, 2020
22bd68c
Remove duplicate section
caustt Jan 15, 2020
894ee73
Add lecture file delete validation
caustt Jan 15, 2020
844c42f
Merge branch 'dev' into feature/#160-remove-files-course-overview
adrianjost Jan 16, 2020
62575b5
Update app/controllers/uploaded_files_controller.rb
caustt Jan 16, 2020
f43b8c7
Update app/controllers/uploaded_files_controller.rb
caustt Jan 16, 2020
87fdd2c
Update app/controllers/uploaded_files_controller.rb
caustt Jan 16, 2020
5d9a3a8
Update app/controllers/uploaded_files_controller.rb
caustt Jan 16, 2020
be7a61b
Update app/controllers/uploaded_files_controller.rb
caustt Jan 16, 2020
e9b59a1
Merge branch 'dev' into feature/#160-remove-files-course-overview
adrianjost Jan 16, 2020
eb538bf
Update uploaded_files_controller.rb
caustt Jan 16, 2020
1fdcbab
change back variable scope
caustt Jan 16, 2020
8c746aa
change variable scope
caustt Jan 16, 2020
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
21 changes: 21 additions & 0 deletions app/controllers/uploaded_files_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
class UploadedFilesController < ApplicationController
before_action :authenticate_user!
before_action :validate_destroy_rights, only: [:destroy]

# GET /uploaded_files
def index
Expand All @@ -12,6 +13,17 @@ def new
@uploaded_file.author = current_user
end


# DELETE /uploaded_files/1
def destroy
@uploaded_file.destroy
if @uploaded_file.allowsUpload.class.name == "Course"
redirect_to course_path(@uploaded_file.allowsUpload), notice: "File was successfully deleted."
else
redirect_to course_lecture_path(@uploaded_file.allowsUpload), notice: "File was successfully deleted."
end
end

# POST /uploaded_files
def create
file = uploaded_file_params["attachment"]
Expand Down Expand Up @@ -48,4 +60,13 @@ def show
def uploaded_file_params
params.require(:uploaded_file).permit(:attachment, :lecture)
end

def validate_destroy_rights
@uploaded_file = UploadedFile.find(params[:id])
caustt marked this conversation as resolved.
Show resolved Hide resolved
unless current_user == @uploaded_file.author
unless (@uploaded_file.allowsUpload.class.name == "Course") && (@uploaded_file.allowsUpload.creator_id == current_user.id)
Copy link
Contributor

@adrianjost adrianjost Jan 15, 2020

Choose a reason for hiding this comment

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

a) there is no else case so these two conditions can be merged
b) it would improve readability a lot if you store the condition result in variables with meaningful names and then only compare the variables.

The conditions were difficult to understand for me because of the inverted logic using unless. But I think this is just a personal preference.
Anyway, do I understand them correctly:
Permission is always granted if you are the author/creator of the file. And if it is a Course File and you aren't the creator permission is denied? Doesn't this result in false positives for Lecture files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I only added validaten for course files in this PR.
I changed it now, so it should also validate lecture files and tried to make it more readable.

redirect_to (uploaded_files_url), notice: "You can't delete this file."
end
end
end
end
2 changes: 1 addition & 1 deletion app/views/courses/_filelist.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
</div>
<div>
<% if @current_user == file.author || @current_user == @course.creator %>
<button class="btn btn-sm btn-outline-danger">TODO ADD DELETE ACTION</button>
<%= link_to 'Delete File', file, method: :delete, class: "btn btn-sm btn-outline-danger", data: { confirm: 'Are you sure?' } %>
<% end %>
</div>
</li>
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Rails.application.routes.draw do
resources :uploaded_files, only: [:show, :index, :new, :create]
resources :uploaded_files, only: [:show, :index, :new, :create, :destroy]
get "/courses/:course_id/lectures/current", to: "lectures#current", as: "current_lectures"
post "/courses/:course_id/lectures/start_lecture", to: "lectures#start_lecture", as: "start_lecture"
post "/courses/:course_id/lectures/join_lecture", to: "lectures#join_lecture", as: "join_lecture"
Expand Down
36 changes: 31 additions & 5 deletions spec/controllers/uploaded_files_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,36 @@
end

it "stores the file only if valid file provided" do
old_count = UploadedFile.count
@file["uploaded_file"].except!("attachment")
post :create, params: @file
expect(response).to_not redirect_to(uploaded_files_url)
expect(UploadedFile.count).to eq(old_count)
old_count = UploadedFile.count
@file["uploaded_file"].except!("attachment")
post :create, params: @file
expect(response).to_not redirect_to(uploaded_files_url)
expect(UploadedFile.count).to eq(old_count)
end

context "having a file uploaded to a course as a student" do
before :each do
@student = FactoryBot.create(:user, :student)
@lecturer = FactoryBot.create(:user, :lecturer)
@course = FactoryBot.create(:course, creator: @lecturer)
@lecture = FactoryBot.create(:lecture, lecturer: @lecturer, course: @course)
@lecturer_file = FactoryBot.create(:uploaded_file, author: @student, allowsUpload_id: @course.id, allowsUpload_type: "Course")
sign_in @student
end

it "can be deleted by the owner" do
expect { post :destroy, params: { id: @lecturer_file[:id] } }.to change(UploadedFile, :count).by(-1)
end

it "can be deleted by the course owner" do
sign_in @lecturer
expect { post :destroy, params: { id: @lecturer_file[:id] } }.to change(UploadedFile, :count).by(-1)
end

it "can't be deleted by someone else" do
@other_lecturer = FactoryBot.create(:user, :lecturer)
sign_in @other_lecturer
expect { post :destroy, params: { id: @lecturer_file[:id] } }.to_not change(UploadedFile, :count)
end
end
end
35 changes: 35 additions & 0 deletions spec/features/courses/show_course_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -111,5 +111,40 @@
expect(page.body).to eql @lecturer_file.data
expect(page.response_headers["Content-Type"]).to eql @lecturer_file.content_type
end

it "should have delete links for every file" do
@student = FactoryBot.create(:user, :student)
@lecturer_file = FactoryBot.create(:uploaded_file, author: @lecturer, allowsUpload_id: @course.id, allowsUpload_type: "Course")
@student_file = FactoryBot.create(:uploaded_file, author: @student, allowsUpload_id: @course.id, allowsUpload_type: "Course")
visit(course_path(@course))
@delete_link_student_file = find_link("Delete File", href: uploaded_file_path(@student_file))
@delete_link_lecturer_file =find_link("Delete File", href: uploaded_file_path(@lecturer_file))
expect { @delete_link_student_file.click }.to change(UploadedFile, :count).by(-1)
expect { @delete_link_lecturer_file.click }.to change(UploadedFile, :count).by(-1)
end
end

context "being signed in as a student" do
before(:each) do
@lecturer = FactoryBot.create(:user, :lecturer)
@course = FactoryBot.create(:course, creator: @lecturer)
@lecture = FactoryBot.create(:lecture, lecturer: @lecturer, course: @course)
@student = FactoryBot.create(:user, :student)
@course.join_course(@student)
sign_in @student
end

it "should only have delete links for his own files" do
@other_student = FactoryBot.create(:user, :student)
@other_student_file = FactoryBot.create(:uploaded_file, author: @other_student, allowsUpload_id: @course.id, allowsUpload_type: "Course")
@lecturer_file = FactoryBot.create(:uploaded_file, author: @lecturer, allowsUpload_id: @course.id, allowsUpload_type: "Course")
@student_file = FactoryBot.create(:uploaded_file, author: @student, allowsUpload_id: @course.id, allowsUpload_type: "Course")
visit(course_path(@course))
expect(page).to have_selector("a[href='#{uploaded_file_path(@student_file)}'][data-method='delete']")
expect(page).to_not have_selector("a[href='#{uploaded_file_path(@lecturer_file)}'][data-method='delete']")
expect(page).to_not have_selector("a[href='#{uploaded_file_path(@other_student_file)}'][data-method='delete']")
@delete_link = find_link("Delete File", href: uploaded_file_path(@student_file))
expect { @delete_link.click }.to change(UploadedFile, :count).by(-1)
end
end
end