diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b7806741..4744417cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,8 +57,6 @@ All notable changes to this project will be documented in this file. See [standa * revert to doubtfire local image for unit tests ([73fcbe3](https://github.com/macite/doubtfire-deploy/commit/73fcbe3f5adb603253033e7b126502a5d3c006f1)) * skip unit tests and linting for documentation updates ([2503fe6](https://github.com/macite/doubtfire-deploy/commit/2503fe61468f8ebe37e54ccb1d0cc2a11387949b)) -### [7.0.23](https://github.com/macite/doubtfire-deploy/compare/v8.0.0-2...v7.0.23) (2024-03-22) - ## [8.0.0-5](https://github.com/macite/doubtfire-deploy/compare/v8.0.0-4...v8.0.0-5) (2024-05-02) diff --git a/Gemfile.lock b/Gemfile.lock index afc55cce0..58ca5d592 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,36 +1,36 @@ GEM remote: https://rubygems.org/ specs: - Ascii85 (1.1.0) - actioncable (7.1.3.2) - actionpack (= 7.1.3.2) - activesupport (= 7.1.3.2) + Ascii85 (1.1.1) + actioncable (7.1.3.3) + actionpack (= 7.1.3.3) + activesupport (= 7.1.3.3) nio4r (~> 2.0) websocket-driver (>= 0.6.1) zeitwerk (~> 2.6) - actionmailbox (7.1.3.2) - actionpack (= 7.1.3.2) - activejob (= 7.1.3.2) - activerecord (= 7.1.3.2) - activestorage (= 7.1.3.2) - activesupport (= 7.1.3.2) + actionmailbox (7.1.3.3) + actionpack (= 7.1.3.3) + activejob (= 7.1.3.3) + activerecord (= 7.1.3.3) + activestorage (= 7.1.3.3) + activesupport (= 7.1.3.3) mail (>= 2.7.1) net-imap net-pop net-smtp - actionmailer (7.1.3.2) - actionpack (= 7.1.3.2) - actionview (= 7.1.3.2) - activejob (= 7.1.3.2) - activesupport (= 7.1.3.2) + actionmailer (7.1.3.3) + actionpack (= 7.1.3.3) + actionview (= 7.1.3.3) + activejob (= 7.1.3.3) + activesupport (= 7.1.3.3) mail (~> 2.5, >= 2.5.4) net-imap net-pop net-smtp rails-dom-testing (~> 2.2) - actionpack (7.1.3.2) - actionview (= 7.1.3.2) - activesupport (= 7.1.3.2) + actionpack (7.1.3.3) + actionview (= 7.1.3.3) + activesupport (= 7.1.3.3) nokogiri (>= 1.8.5) racc rack (>= 2.2.4) @@ -38,35 +38,35 @@ GEM rack-test (>= 0.6.3) rails-dom-testing (~> 2.2) rails-html-sanitizer (~> 1.6) - actiontext (7.1.3.2) - actionpack (= 7.1.3.2) - activerecord (= 7.1.3.2) - activestorage (= 7.1.3.2) - activesupport (= 7.1.3.2) + actiontext (7.1.3.3) + actionpack (= 7.1.3.3) + activerecord (= 7.1.3.3) + activestorage (= 7.1.3.3) + activesupport (= 7.1.3.3) globalid (>= 0.6.0) nokogiri (>= 1.8.5) - actionview (7.1.3.2) - activesupport (= 7.1.3.2) + actionview (7.1.3.3) + activesupport (= 7.1.3.3) builder (~> 3.1) erubi (~> 1.11) rails-dom-testing (~> 2.2) rails-html-sanitizer (~> 1.6) - activejob (7.1.3.2) - activesupport (= 7.1.3.2) + activejob (7.1.3.3) + activesupport (= 7.1.3.3) globalid (>= 0.3.6) - activemodel (7.1.3.2) - activesupport (= 7.1.3.2) - activerecord (7.1.3.2) - activemodel (= 7.1.3.2) - activesupport (= 7.1.3.2) + activemodel (7.1.3.3) + activesupport (= 7.1.3.3) + activerecord (7.1.3.3) + activemodel (= 7.1.3.3) + activesupport (= 7.1.3.3) timeout (>= 0.4.0) - activestorage (7.1.3.2) - actionpack (= 7.1.3.2) - activejob (= 7.1.3.2) - activerecord (= 7.1.3.2) - activesupport (= 7.1.3.2) + activestorage (7.1.3.3) + actionpack (= 7.1.3.3) + activejob (= 7.1.3.3) + activerecord (= 7.1.3.3) + activesupport (= 7.1.3.3) marcel (~> 1.0) - activesupport (7.1.3.2) + activesupport (7.1.3.3) base64 bigdecimal concurrent-ruby (~> 1.0, >= 1.0.2) @@ -90,7 +90,7 @@ GEM erubi (>= 1.0.0) rack (>= 0.9.0) rouge (>= 1.0.0) - bigdecimal (3.1.7) + bigdecimal (3.1.8) bindata (2.5.0) bootsnap (1.18.3) msgpack (~> 1.2) @@ -107,7 +107,7 @@ GEM code_analyzer (0.5.5) sexp_processor coderay (1.1.3) - concurrent-ruby (1.2.3) + concurrent-ruby (1.3.1) connection_pool (2.4.1) crack (1.0.0) bigdecimal @@ -130,7 +130,7 @@ GEM diff-lcs (1.5.1) docile (1.4.0) domain_name (0.6.20240107) - dotenv (3.1.1) + dotenv (3.1.2) drb (2.2.1) dry-core (1.0.1) concurrent-ruby (~> 1.0) @@ -159,7 +159,7 @@ GEM factory_bot_rails (6.4.3) factory_bot (~> 6.4) railties (>= 5.0.0) - faker (3.3.1) + faker (3.4.1) i18n (>= 1.8.11, < 2) faraday (2.9.0) faraday-net_http (>= 2.0, < 3.2) @@ -167,7 +167,7 @@ GEM faraday (>= 1, < 3) faraday-net_http (3.1.0) net-http - ffi (1.16.3) + ffi (1.17.0-aarch64-linux-gnu) fugit (1.11.0) et-orbi (~> 1, >= 1.2.11) raabro (~> 1.4) @@ -183,7 +183,7 @@ GEM grape-entity (1.0.1) activesupport (>= 3.0.0) multi_json (>= 1.3.2) - grape-swagger (2.0.3) + grape-swagger (2.1.0) grape (>= 1.7, < 3.0) rack-test (~> 2) grape-swagger-rails (0.5.0) @@ -192,15 +192,15 @@ GEM hashery (2.1.2) hirb (0.7.3) http-accept (1.7.0) - http-cookie (1.0.5) + http-cookie (1.0.6) domain_name (~> 0.5) - i18n (1.14.4) + i18n (1.14.5) concurrent-ruby (~> 1.0) icalendar (2.10.1) ice_cube (~> 0.16) ice_cube (0.16.4) io-console (0.7.2) - irb (1.13.0) + irb (1.13.1) rdoc (>= 4.0.0) reline (>= 0.4.2) jaro_winkler (1.5.6) @@ -231,9 +231,9 @@ GEM marcel (1.0.4) mime-types (3.5.2) mime-types-data (~> 3.2015) - mime-types-data (3.2024.0305) + mime-types-data (3.2024.0507) mini_mime (1.1.5) - minitest (5.22.3) + minitest (5.23.1) minitest-around (0.5.0) minitest (~> 5.0) minitest-rails (7.1.0) @@ -251,7 +251,7 @@ GEM mysql2 (0.5.6) net-http (0.4.1) uri - net-imap (0.4.10) + net-imap (0.4.12) date net-protocol net-ldap (0.19.0) @@ -262,15 +262,13 @@ GEM net-smtp (0.5.0) net-protocol netrc (0.11.0) - nio4r (2.7.1) - nokogiri (1.16.4-aarch64-linux) - racc (~> 1.4) - nokogiri (1.16.4-x86_64-linux) + nio4r (2.7.3) + nokogiri (1.16.5-aarch64-linux) racc (~> 1.4) observer (0.1.2) orm_adapter (0.5.0) parallel (1.24.0) - parser (3.3.1.0) + parser (3.3.2.0) ast (~> 2.4.1) racc pdf-reader (2.12.0) @@ -280,15 +278,15 @@ GEM ruby-rc4 ttfunk pkg-config (1.5.6) - prism (0.27.0) + prism (0.29.0) psych (5.1.2) stringio public_suffix (5.0.5) puma (6.4.2) nio4r (~> 2.0) raabro (1.4.0) - racc (1.7.3) - rack (3.0.10) + racc (1.8.0) + rack (3.0.11) rack-accept (0.4.5) rack (>= 0.4) rack-cors (2.0.2) @@ -300,20 +298,20 @@ GEM rackup (2.1.0) rack (>= 3) webrick (~> 1.8) - rails (7.1.3.2) - actioncable (= 7.1.3.2) - actionmailbox (= 7.1.3.2) - actionmailer (= 7.1.3.2) - actionpack (= 7.1.3.2) - actiontext (= 7.1.3.2) - actionview (= 7.1.3.2) - activejob (= 7.1.3.2) - activemodel (= 7.1.3.2) - activerecord (= 7.1.3.2) - activestorage (= 7.1.3.2) - activesupport (= 7.1.3.2) + rails (7.1.3.3) + actioncable (= 7.1.3.3) + actionmailbox (= 7.1.3.3) + actionmailer (= 7.1.3.3) + actionpack (= 7.1.3.3) + actiontext (= 7.1.3.3) + actionview (= 7.1.3.3) + activejob (= 7.1.3.3) + activemodel (= 7.1.3.3) + activerecord (= 7.1.3.3) + activestorage (= 7.1.3.3) + activesupport (= 7.1.3.3) bundler (>= 1.15.0) - railties (= 7.1.3.2) + railties (= 7.1.3.3) rails-dom-testing (2.2.0) activesupport (>= 5.0.0) minitest @@ -331,9 +329,9 @@ GEM json require_all (~> 3.0) ruby-progressbar - railties (7.1.3.2) - actionpack (= 7.1.3.2) - activesupport (= 7.1.3.2) + railties (7.1.3.3) + actionpack (= 7.1.3.3) + activesupport (= 7.1.3.3) irb rackup (>= 1.0.0) rake (>= 12.2) @@ -342,18 +340,18 @@ GEM rainbow (3.1.1) rake (13.2.1) rb-fsevent (0.11.2) - rb-inotify (0.10.1) + rb-inotify (0.11.1) ffi (~> 1.0) rbs (2.8.4) rbtree (0.4.6) - rdoc (6.6.3.1) + rdoc (6.7.0) psych (>= 4.0.0) redis (5.2.0) redis-client (>= 0.22.0) - redis-client (0.22.1) + redis-client (0.22.2) connection_pool - regexp_parser (2.9.0) - reline (0.5.4) + regexp_parser (2.9.2) + reline (0.5.8) io-console (~> 0.5) require_all (3.0.0) responders (3.1.1) @@ -366,8 +364,9 @@ GEM netrc (~> 0.8) reverse_markdown (2.1.1) nokogiri - rexml (3.2.6) - rmagick (5.5.0) + rexml (3.2.8) + strscan (>= 3.0.9) + rmagick (6.0.1) observer (~> 0.1) pkg-config (~> 1.4) roo (2.7.1) @@ -378,7 +377,7 @@ GEM roo (>= 2.0.0, < 3) spreadsheet (> 0.9.0) rouge (4.2.1) - rubocop (1.63.4) + rubocop (1.64.1) json (~> 2.3) language_server-protocol (>= 3.17.0) parallel (~> 1.10) @@ -394,15 +393,15 @@ GEM rubocop-faker (1.1.0) faker (>= 2.12.0) rubocop (>= 0.82.0) - rubocop-rails (2.24.1) + rubocop-rails (2.25.0) activesupport (>= 4.2.0) rack (>= 1.1) rubocop (>= 1.33.0, < 2.0) rubocop-ast (>= 1.31.1, < 2.0) ruby-filemagic (0.7.3) - ruby-lsp (0.16.6) + ruby-lsp (0.17.1) language_server-protocol (~> 3.17.0) - prism (>= 0.23.0, < 0.28) + prism (>= 0.29.0, < 0.30) sorbet-runtime (>= 0.5.10782) ruby-ole (1.2.13.1) ruby-progressbar (1.13.0) @@ -446,7 +445,7 @@ GEM thor (~> 1.0) tilt (~> 2.0) yard (~> 0.9, >= 0.9.24) - sorbet-runtime (0.5.11367) + sorbet-runtime (0.5.11409) sorted_set (1.0.3) rbtree set (~> 1.0) @@ -461,6 +460,7 @@ GEM activesupport (>= 5.2) sprockets (>= 3.0.0) stringio (3.1.0) + strscan (3.1.0) tca_client (1.0.4) typhoeus (~> 1.0, >= 1.0.1) tcp_timeout (0.1.1) @@ -477,7 +477,7 @@ GEM uri (0.13.0) warden (1.2.9) rack (>= 2.0.9) - webmock (3.23.0) + webmock (3.23.1) addressable (>= 2.8.0) crack (>= 0.3.2) hashdiff (>= 0.4.0, < 2.0.0) @@ -486,11 +486,10 @@ GEM websocket-extensions (>= 0.1.0) websocket-extensions (0.1.5) yard (0.9.36) - zeitwerk (2.6.13) + zeitwerk (2.6.15) PLATFORMS aarch64-linux - x86_64-linux DEPENDENCIES better_errors @@ -549,7 +548,7 @@ DEPENDENCIES webmock RUBY VERSION - ruby 3.1.3p185 + ruby 3.1.4p223 BUNDLED WITH - 2.4.9 + 2.4.15 diff --git a/app/api/entities/unit_entity.rb b/app/api/entities/unit_entity.rb index f1865d015..4ac79cb15 100644 --- a/app/api/entities/unit_entity.rb +++ b/app/api/entities/unit_entity.rb @@ -5,11 +5,11 @@ class UnitEntity < Grape::Entity end def is_staff?(my_role) - Role.teaching_staff_ids.include?(my_role.id) unless my_role.nil? + [Role.tutor_id, Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? end - def is_admin_staff?(my_role) - Role.admin_staff_ids.include?(my_role.id) unless my_role.nil? + def can_read_unit_config?(my_role) + [Role.convenor_id, Role.admin_id, Role.auditor_id].include?(my_role.id) unless my_role.nil? end expose :code @@ -36,7 +36,7 @@ def is_admin_staff?(my_role) expose :active - expose :overseer_image_id, unless: :summary_only, if: lambda { |unit, options| is_admin_staff?(options[:my_role]) } + expose :overseer_image_id, unless: :summary_only, if: lambda { |unit, options| can_read_unit_config?(options[:my_role]) } expose :assessment_enabled, unless: :summary_only expose :auto_apply_extension_before_deadline, unless: :summary_only, if: lambda { |unit, options| is_staff?(options[:my_role]) } diff --git a/app/api/learning_alignment_api.rb b/app/api/learning_alignment_api.rb index 2ea49366c..0a8900bb9 100644 --- a/app/api/learning_alignment_api.rb +++ b/app/api/learning_alignment_api.rb @@ -229,7 +229,7 @@ class LearningAlignmentApi < Grape::API get '/units/:unit_id/learning_alignments/class_details' do unit = Unit.find(params[:unit_id]) - unless authorise?(current_user, unit, :provide_feedback) + unless authorise?(current_user, unit, :get_students) error!({ error: 'You are not authorised to access these task alignments.' }, 403) end diff --git a/app/api/projects_api.rb b/app/api/projects_api.rb index 91d7c513f..dc3cbafc6 100644 --- a/app/api/projects_api.rb +++ b/app/api/projects_api.rb @@ -56,7 +56,7 @@ class ProjectsApi < Grape::API if authorise? current_user, project, :trigger_week_end project.trigger_week_end(current_user) else - error!({ error: "You are not authorised to perform this action for Project with id=#{params[:id]}" }, 403) + error!({ error: "You are not authorised to perform this action" }, 403) end else error!({ error: "Invalid trigger - #{params[:trigger]} unknown" }, 403) @@ -64,28 +64,28 @@ class ProjectsApi < Grape::API # If we are only updating the campus elsif params[:campus_id].present? unless authorise? current_user, project, :change_campus - error!({ error: "You cannot change the campus for project #{params[:id]}" }, 403) + error!({ error: "You cannot change the campus for this student" }, 403) end for_student = false project.campus_id = params[:campus_id] == -1 ? nil : params[:campus_id] project.save! elsif !params[:enrolled].nil? unless authorise? current_user, project.unit, :change_project_enrolment - error!({ error: "You cannot change the enrolment for project #{params[:id]}" }, 403) + error!({ error: "You cannot change the enrolment for this student" }, 403) end for_student = false project.enrolled = params[:enrolled] project.save elsif !params[:target_grade].nil? unless authorise? current_user, project, :change - error!({ error: "You do not have permissions to change Project with id=#{params[:id]}" }, 403) + error!({ error: "You do not have permissions to change this student" }, 403) end project.target_grade = params[:target_grade] project.save elsif !params[:submitted_grade].nil? unless authorise? current_user, project, :change - error!({ error: "You do not have permissions to change Project with id=#{params[:id]}" }, 403) + error!({ error: "You do not have permissions to change this student" }, 403) end if project.portfolio_exists? error!({ error: "You cannot change your submitted grade after portfolio submission" }, 403) @@ -95,7 +95,7 @@ class ProjectsApi < Grape::API project.save elsif !params[:grade].nil? unless authorise? current_user, project, :assess - error!({ error: "You do not have permissions to assess Project with id=#{params[:id]}" }, 403) + error!({ error: "You do not have permissions to assess this student" }, 403) end if params[:grade_rationale].nil? @@ -120,7 +120,7 @@ class ProjectsApi < Grape::API return elsif !params[:compile_portfolio].nil? unless authorise? current_user, project, :change - error!({ error: "You do not have permissions to change Project with id=#{params[:id]}" }, 403) + error!({ error: "You do not have permissions to change this student" }, 403) end # if someone changes this setting manually, clear the autogenerated status diff --git a/app/api/students_api.rb b/app/api/students_api.rb index 29b9e573b..455cb69dd 100644 --- a/app/api/students_api.rb +++ b/app/api/students_api.rb @@ -16,7 +16,7 @@ class StudentsApi < Grape::API get '/students' do unit = Unit.find(params[:unit_id]) - if (authorise? current_user, unit, :get_students) || (authorise? current_user, User, :admin_units) + if authorise? current_user, unit, :get_students result = if params[:withdrawn].nil? || (!params[:withdrawn].nil? && !params[:withdrawn]) unit.student_query(true) else diff --git a/app/api/submission/portfolio_api.rb b/app/api/submission/portfolio_api.rb index b58dd6ac3..3b0182ec5 100644 --- a/app/api/submission/portfolio_api.rb +++ b/app/api/submission/portfolio_api.rb @@ -5,6 +5,7 @@ class PortfolioApi < Grape::API helpers GenerateHelpers helpers AuthenticationHelpers helpers AuthorisationHelpers + helpers FileStreamHelper before do authenticated? @@ -95,7 +96,7 @@ class PortfolioApi < Grape::API content_type 'application/pdf' env['api.format'] = :binary - File.read(evidence_loc) + stream_file evidence_loc end # get # "Retrieve portfolios for a unit" done using controller diff --git a/app/api/submission/portfolio_evidence_api.rb b/app/api/submission/portfolio_evidence_api.rb index 42fa799c8..ff7176821 100644 --- a/app/api/submission/portfolio_evidence_api.rb +++ b/app/api/submission/portfolio_evidence_api.rb @@ -5,6 +5,7 @@ class PortfolioEvidenceApi < Grape::API helpers GenerateHelpers helpers AuthenticationHelpers helpers AuthorisationHelpers + helpers FileStreamHelper include LogHelper def self.logger @@ -109,9 +110,8 @@ def self.logger # Set download headers... content_type 'application/pdf' - env['api.format'] = :binary - File.read(evidence_loc) + stream_file evidence_loc end # get desc "Request for a task's documents to be re-processed to recreate the task's PDF" diff --git a/app/api/task_definitions_api.rb b/app/api/task_definitions_api.rb index 8e5414aee..03536c9ef 100644 --- a/app/api/task_definitions_api.rb +++ b/app/api/task_definitions_api.rb @@ -6,6 +6,7 @@ class TaskDefinitionsApi < Grape::API helpers FileHelper helpers MimeCheckHelpers helpers Submission::GenerateHelpers + helpers FileStreamHelper before do authenticated? @@ -456,7 +457,7 @@ class TaskDefinitionsApi < Grape::API end get '/units/:unit_id/task_definitions/:task_def_id/tasks' do unit = Unit.find(params[:unit_id]) - unless authorise? current_user, unit, :provide_feedback + unless authorise? current_user, unit, :get_students error!({ error: 'Not authorised to access tasks for this unit' }, 403) end @@ -557,8 +558,7 @@ class TaskDefinitionsApi < Grape::API end content_type 'application/pdf' - env['api.format'] = :binary - File.read(path) + stream_file path end desc 'Download the task resources' @@ -585,8 +585,7 @@ class TaskDefinitionsApi < Grape::API end header['Access-Control-Expose-Headers'] = 'Content-Disposition' - env['api.format'] = :binary - File.read(path) + stream_file path end desc 'Download the task assessment resources' @@ -613,7 +612,6 @@ class TaskDefinitionsApi < Grape::API end header['Access-Control-Expose-Headers'] = 'Content-Disposition' - env['api.format'] = :binary - File.read(path) + stream_file path end end diff --git a/app/api/tasks_api.rb b/app/api/tasks_api.rb index dc4531757..10e45917e 100644 --- a/app/api/tasks_api.rb +++ b/app/api/tasks_api.rb @@ -3,6 +3,7 @@ class TasksApi < Grape::API helpers AuthenticationHelpers helpers AuthorisationHelpers + helpers FileStreamHelper before do authenticated? @@ -227,9 +228,8 @@ class TasksApi < Grape::API # Set download headers... content_type 'application/octet-stream' - env['api.format'] = :binary # Return the file data - File.read(file_loc) + stream_file file_loc end end diff --git a/app/api/unit_roles_api.rb b/app/api/unit_roles_api.rb index cc1bd2d3a..b59a69f96 100644 --- a/app/api/unit_roles_api.rb +++ b/app/api/unit_roles_api.rb @@ -13,7 +13,7 @@ class UnitRolesApi < Grape::API optional :active_only, type: Boolean, desc: 'Show only active roles' end get '/unit_roles' do - return [] unless authorise? current_user, User, :act_tutor + return [] unless authorise? current_user, User, :get_unit_roles result = UnitRole.includes(:unit).where(unit_roles: { user_id: current_user.id }) @@ -29,7 +29,7 @@ class UnitRolesApi < Grape::API unit_role = UnitRole.find(params[:id]) unless (authorise? current_user, unit_role.unit, :employ_staff) || (authorise? current_user, User, :admin_units) - error!({ error: "Couldn't find UnitRole with id=#{params[:id]}" }, 403) + error!({ error: "You do not have permission to perform this action" }, 403) end unit_role.destroy! diff --git a/app/api/units_api.rb b/app/api/units_api.rb index 4dbc09cd1..bbdc93fca 100644 --- a/app/api/units_api.rb +++ b/app/api/units_api.rb @@ -26,7 +26,7 @@ class UnitsApi < Grape::API optional :include_in_active, type: Boolean, desc: 'Include units that are not active' end get '/units' do - unless authorise? current_user, User, :convene_units + unless authorise? current_user, User, :get_all_units error!({ error: 'Unable to list units' }, 403) end @@ -190,10 +190,14 @@ class UnitsApi < Grape::API :allow_student_change_tutorial, ) - # Identify main convenor - main_convenor_user = unit_parameters[:main_convenor_user_id].present? ? User.find(unit_parameters[:main_convenor_user_id]) : main_convenor_user = current_user + # Identify main convenor - ensure they have the correct role + main_convenor_user = unit_parameters[:main_convenor_user_id].present? ? User.find(unit_parameters[:main_convenor_user_id]) : current_user - unless authorise? current_user, User, :convene_units + unless main_convenor_user.present? + error!({ error: 'Main convenor user not found' }, 403) + end + + unless authorise? main_convenor_user, User, :convene_units error!({ error: 'Main convenor is not authorised to manage units' }, 403) end @@ -259,7 +263,7 @@ class UnitsApi < Grape::API get '/units/:id/feedback' do unit = Unit.find(params[:id]) - unless authorise? current_user, unit, :provide_feedback + unless authorise? current_user, unit, :get_students error!({ error: 'Not authorised to provide feedback for this unit' }, 403) end @@ -271,7 +275,7 @@ class UnitsApi < Grape::API get '/units/:id/tasks/inbox' do unit = Unit.find(params[:id]) - unless authorise? current_user, unit, :provide_feedback + unless authorise? current_user, unit, :get_students error!({ error: 'Not authorised to provide feedback for this unit' }, 403) end diff --git a/app/api/users_api.rb b/app/api/users_api.rb index 30f055df7..ffcf6a42f 100644 --- a/app/api/users_api.rb +++ b/app/api/users_api.rb @@ -21,7 +21,7 @@ class UsersApi < Grape::API desc 'Get user' get '/users/:id', requirements: { id: /[0-9]*/ } do user = User.eager_load(:role).find(params[:id]) - unless (user.id == current_user.id) || (authorise? current_user, User, :admin_users) + unless (user.id == current_user.id) || (authorise? current_user, User, :get_user) error!({ error: "Cannot find User with id #{params[:id]}" }, 403) end @@ -30,7 +30,7 @@ class UsersApi < Grape::API desc 'Get convenors' get '/users/convenors' do - unless authorise? current_user, User, :convene_units + unless authorise? current_user, User, :get_staff_list error!({ error: 'Cannot list convenors - not authorised' }, 403) end @@ -39,7 +39,7 @@ class UsersApi < Grape::API desc 'Get tutors' get '/users/tutors' do - unless authorise? current_user, User, :convene_units + unless authorise? current_user, User, :get_staff_list error!({ error: 'Cannot list tutors - not authorised' }, 403) end @@ -55,7 +55,7 @@ class UsersApi < Grape::API optional :email, type: String, desc: 'New email address for user' optional :student_id, type: String, desc: 'New student_id for user' optional :nickname, type: String, desc: 'New nickname for user' - optional :system_role, type: String, desc: 'New role for user [Admin, Convenor, Tutor, Student]' + optional :system_role, type: String, desc: 'New role for user [Admin, Auditor, Convenor, Tutor, Student]' optional :receive_task_notifications, type: Boolean, desc: 'Allow user to be sent task notifications' optional :receive_portfolio_notifications, type: Boolean, desc: 'Allow user to be sent portfolio notifications' optional :receive_feedback_notifications, type: Boolean, desc: 'Allow user to be sent feedback notifications' @@ -114,7 +114,14 @@ class UsersApi < Grape::API if new_role.nil? error!({ error: "No such role name #{user_parameters[:role]}" }, 403) end - action = new_role.id > old_role.id ? :promote_user : :demote_user + + if old_role == Role.auditor + action - :promote_user + elsif new_role == Role.auditor + action = old_role == Role.student ? :promote_user : :demote_user + else + action = new_role.id > old_role.id ? :promote_user : :demote_user + end # current user not authorised to peform action with new role? unless authorise? current_user, User, action, User.get_change_role_perm_fn, [old_role.to_sym, new_role.to_sym] @@ -141,7 +148,7 @@ class UsersApi < Grape::API optional :student_id, type: String, desc: 'New student_id for user' requires :username, type: String, desc: 'New username for user' requires :nickname, type: String, desc: 'New nickname for user' - requires :system_role, type: String, desc: 'New system role for user [Admin, Convenor, Tutor, Student]' + requires :system_role, type: String, desc: 'New system role for user [Admin, Auditor, Convenor, Tutor, Student]' end end post '/users' do diff --git a/app/controllers/portfolio_downloads_controller.rb b/app/controllers/portfolio_downloads_controller.rb index f8ee4498e..a11fdb65e 100644 --- a/app/controllers/portfolio_downloads_controller.rb +++ b/app/controllers/portfolio_downloads_controller.rb @@ -25,7 +25,7 @@ def index unit = Unit.find(params[:id]) - unless authorise? current_user, unit, :provide_feedback + unless authorise? current_user, unit, :get_students error!({ error: "Not authorised to download portfolios for unit '#{params[:id]}'" }, 401) end diff --git a/app/controllers/task_downloads_controller.rb b/app/controllers/task_downloads_controller.rb index 600389554..d30120955 100644 --- a/app/controllers/task_downloads_controller.rb +++ b/app/controllers/task_downloads_controller.rb @@ -25,7 +25,7 @@ def index unit = Unit.find(params[:id]) - unless authorise? current_user, unit, :provide_feedback + unless authorise? current_user, unit, :get_students error!({ error: "Not authorised to download tasks for unit '#{params[:id]}'" }, 401) end diff --git a/app/controllers/task_submission_pdfs_controller.rb b/app/controllers/task_submission_pdfs_controller.rb index dbd20449e..5a183d9a3 100644 --- a/app/controllers/task_submission_pdfs_controller.rb +++ b/app/controllers/task_submission_pdfs_controller.rb @@ -25,8 +25,8 @@ def index unit = Unit.find(params[:id]) - unless authorise? current_user, unit, :provide_feedback - error!({ error: "Not authorised to download student PDFs for unit '#{params[:id]}'" }, 401) + unless authorise? current_user, unit, :get_students + error!({ error: "Not authorised to download student PDFs for unit '#{params[:id]}'" }, 401) end td = unit.task_definitions.find(params[:task_def_id]) diff --git a/app/helpers/file_stream_helper.rb b/app/helpers/file_stream_helper.rb new file mode 100644 index 000000000..c0d9d7898 --- /dev/null +++ b/app/helpers/file_stream_helper.rb @@ -0,0 +1,52 @@ +module FileStreamHelper + # Extract part of the contents so that is can be streamed to the client + # file_path is the path to the file to be streamed + # this will set the headers and return the content + def stream_file(file_path) + # Work out what part to return + file_size = File.size(file_path) + begin_point = 0 + end_point = file_size - 1 + + # Was it asked for just a part of the file? + if request.headers['Range'] + # indicate partial content + status 206 + + # extract part desired from the content + if request.headers['Range'] =~ /bytes=(\d+)-(\d*)/ + begin_point = Regexp.last_match(1).to_i + end_point = Regexp.last_match(2).to_i if Regexp.last_match(2).present? + end + + end_point = file_size - 1 unless end_point < file_size - 1 + + # Limit to 10MB chunks + if end_point - begin_point + 1 > 10_485_760 + end_point = begin_point + 10_485_760 + end + elsif file_size > 10_485_760 + # If the file is large, only return the first 1MB + status 206 + begin_point = 0 + end_point = 10_485_760 + else + sendfile file_path + + return + end + + # Return the requested content + content_length = end_point - begin_point + 1 + header['Content-Range'] = "bytes #{begin_point}-#{end_point}/#{file_size}" + header['Content-Length'] = content_length.to_s + header['Accept-Ranges'] = 'bytes' + + env['api.format'] = :binary + + # Read the binary data and return + body = File.binread(file_path, content_length, begin_point) + end + + module_function :stream_file +end diff --git a/app/models/group.rb b/app/models/group.rb index db4f766d8..e075c04b4 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -58,12 +58,17 @@ def self.permissions :can_exceed_capacity, :move_tutorial ] + # What can auditors do with groups? + auditor_role_permissions = [ + :get_members + ] # What can nil users do with groups? nil_role_permissions = [] # Return permissions hash { admin: admin_role_permissions, + auditor: auditor_role_permissions, convenor: convenor_role_permissions, tutor: tutor_role_permissions, student: student_role_permissions, diff --git a/app/models/group_set.rb b/app/models/group_set.rb index 3d2fea097..b4731579c 100644 --- a/app/models/group_set.rb +++ b/app/models/group_set.rb @@ -37,6 +37,7 @@ def self.permissions # Return permissions hash { + admin: convenor_role_permissions, convenor: convenor_role_permissions, tutor: tutor_role_permissions, student: student_role_permissions, diff --git a/app/models/project.rb b/app/models/project.rb index b41b7b81b..c0770cf87 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -63,8 +63,16 @@ def self.permissions :assess, :change_campus ] - # What can convenors do with projects? - convenor_role_permissions = [] + # What can admins do with projects? + admin_role_permissions = [ + :get, + :get_submission + ] + # What can auditors do with projects? + auditor_role_permissions = [ + :get, + :get_submission + ] # What can nil users do with projects? nil_role_permissions = [] @@ -72,6 +80,8 @@ def self.permissions { student: student_role_permissions, tutor: tutor_role_permissions, + admin: admin_role_permissions, + auditor: auditor_role_permissions, nil: nil_role_permissions } end @@ -222,6 +232,8 @@ def main_convenor_user def user_role(user) if user == student then :student elsif user.present? && unit.tutors.where(id: user.id).count != 0 then :tutor + elsif user.present? && user.role.id == Role.admin_id then :admin + elsif user.present? && user.role.id == Role.auditor_id then :auditor else nil end end diff --git a/app/models/role.rb b/app/models/role.rb index c6ef76270..d7ef0bd37 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -8,14 +8,6 @@ def self.find(id) end end - def self.admin_staff_ids - [self.convenor_id, self.admin_id] - end - - def self.teaching_staff_ids - [self.tutor_id, self.convenor_id, self.admin_id] - end - def self.student Role.find(student_id) end @@ -32,6 +24,10 @@ def self.admin Role.find(admin_id) end + def self.auditor + Role.find(auditor_id) + end + def to_sym name.downcase.to_sym end @@ -66,10 +62,16 @@ def self.admin_id 4 end + def self.auditor_id + 5 + end + def self.with_name(name) case name when /[Aa]dmin/ admin + when /[Aa]uditor/ + auditor when /[Cc]onvenor/ convenor when /[Tt]utor/ diff --git a/app/models/task.rb b/app/models/task.rb index 97a8db50b..e75815f90 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -49,6 +49,20 @@ def self.permissions :assess_extension, :request_extension ] + # What can admins do with tasks? + admin_role_permissions = [ + :get, + :get_submission, + :view_plagiarism, + :get_discussion + ] + # What can auditors do with tasks? + auditor_role_permissions = [ + :get, + :get_submission, + :view_plagiarism, + :get_discussion + ] # What can nil users do with tasks? nil_role_permissions = [] @@ -57,6 +71,8 @@ def self.permissions student: student_role_permissions, tutor: tutor_role_permissions, convenor: convenor_role_permissions, + admin: admin_role_permissions, + auditor: auditor_role_permissions, nil: nil_role_permissions } end diff --git a/app/models/unit.rb b/app/models/unit.rb index 9c94adb42..175e62c79 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -69,6 +69,13 @@ def self.permissions :exceed_capacity ] + # What can auditors do with units? + auditor_role_permissions = [ + :get_unit, + :get_students, + :download_stats, + ] + # What can other users do with units? nil_role_permissions = [] @@ -78,6 +85,7 @@ def self.permissions tutor: tutor_role_permissions, convenor: convenor_role_permissions, admin: admin_role_permissions, + auditor: auditor_role_permissions, nil: nil_role_permissions } end @@ -89,6 +97,10 @@ def role_for(user) Role.tutor elsif active_projects.where('projects.user_id=:id', id: user.id).count == 1 Role.student + elsif user.has_auditor_capability? && + start_date >= Date.today - Doubtfire::Application.config.auditor_unit_access_years && + end_date < DateTime.now + Role.auditor elsif user.has_admin_capability? Role.admin else @@ -317,8 +329,12 @@ def student_tasks def self.for_user_admin(user) if user.has_admin_capability? Unit.all + elsif user.has_auditor_capability? + # Limit range of units that the auditor has access to + earliest_unit_start_date = Date.today - Doubtfire::Application.config.auditor_unit_access_years + Unit.all.where('start_date >= :earliest_unit_start_date AND end_date < :today', earliest_unit_start_date: earliest_unit_start_date, today: DateTime.now) else - Unit.joins(:unit_roles).where('unit_roles.user_id = :user_id and unit_roles.role_id = :convenor_role', user_id: user.id, convenor_role: Role.convenor.id) + Unit.joins(:unit_roles).where('unit_roles.user_id = :user_id AND unit_roles.role_id = :convenor_role', user_id: user.id, convenor_role: Role.convenor.id) end end @@ -1634,7 +1650,7 @@ def import_task_files_from_zip(zip_file) if (File.extname(file.name) == '.pdf') || (File.extname(file.name) == '.zip') found = false # sort task definitions by longest abbreviation to ensure longest matches - task_definitions.sort_by{ |td| -td.abbreviation.size }.each do |td| + task_definitions.sort_by { |td| -td.abbreviation.size }.each do |td| next unless /^#{td.abbreviation}/ =~ file_name file.extract ("#{task_path}#{FileHelper.sanitized_filename(td.abbreviation)}#{File.extname(file.name)}") { true } diff --git a/app/models/user.rb b/app/models/user.rb index 262b5d028..6e016badf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -147,9 +147,10 @@ def token_for_text?(a_token) validate :can_change_to_role?, if: :will_save_change_to_role_id? # Queries - scope :tutors, -> { joins(:role).where('roles.id = :tutor_role or roles.id = :convenor_role or roles.id = :admin_role', tutor_role: Role.tutor_id, convenor_role: Role.convenor_id, admin_role: Role.admin_id) } - scope :convenors, -> { joins(:role).where('roles.id = :convenor_role or roles.id = :admin_role', convenor_role: Role.convenor_id, admin_role: Role.admin_id) } - scope :admins, -> { joins(:role).where('roles.id = :admin_role', admin_role: Role.admin_id) } + scope :tutors, -> { joins(:role).where('roles.id = :tutor_role or roles.id = :convenor_role or roles.id = :admin_role or roles.id = :auditor_role', tutor_role: Role.tutor_id, convenor_role: Role.convenor_id, admin_role: Role.admin_id, auditor_role: Role.auditor_id) } + scope :convenors, -> { joins(:role).where('roles.id = :convenor_role or roles.id = :admin_role or roles.id = :auditor_role', convenor_role: Role.convenor_id, admin_role: Role.admin_id, auditor_role: Role.auditor_id) } + scope :admins, -> { joins(:role).where('roles.id = :admin_role or roles.id = :auditor_role', admin_role: Role.admin_id, auditor_role: Role.auditor_id) } + scope :auditors, -> { joins(:role).where('roles.id = :auditor_role', auditor_role: Role.auditor_id) } def self.teaching(unit) User.joins(:unit_roles).where('unit_roles.unit_id = :unit_id and ( unit_roles.role_id = :tutor_role_id or unit_roles.role_id = :convenor_role_id) ', unit_id: unit.id, tutor_role_id: Role.tutor_id, convenor_role_id: Role.convenor_id) @@ -167,7 +168,7 @@ def can_change_to_role? fail_if_in_unit_role = [Role.tutor, Role.convenor] if new_role == Role.student fail_if_in_unit_role = [Role.convenor] if new_role == Role.tutor - fail_if_in_unit_role = [] if new_role == Role.admin || new_role == Role.convenor + fail_if_in_unit_role = [] if new_role == Role.admin || new_role == Role.convenor || new_role == Role.auditor for check_role in fail_if_in_unit_role do if unit_roles.where('role_id = :role_id', role_id: check_role.id).count > 0 @@ -192,15 +193,19 @@ def has_admin_capability? role_id == Role.admin_id end + def has_auditor_capability? + role_id == Role.auditor_id + end + def self.get_change_role_perm_fn lambda do |role, perm_hash, other| from_role = other[0] to_role = other[1] - (chg_roles = perm_hash[:change_role]) && - (role_hash = chg_roles[role]) && - (from_role_hash = role_hash[from_role]) && - from_role_hash[to_role] + (chg_roles = perm_hash[:change_role]) && # get the change role hash - ensure it exists + (role_hash = chg_roles[role]) && # get the role from... + (from_role_hash = role_hash[from_role]) && # get the role to from within the role from + from_role_hash[to_role] # the permissions allowed end end @@ -213,34 +218,46 @@ def self.permissions change_role_permissions = { # The current_user's role is an Administrator admin: { - # User being assigned is an admin? + # User being changed is an admin? # An admin current_user can demote them to either a student, tutor or convenor admin: { student: [:demote_user], tutor: [:demote_user], - convenor: [:demote_user] }, - # User being assigned is a convenor? - # An admin current_user can demote them to student or tutor + convenor: [:demote_user], + auditor: [:demote_user] }, + # User being changed is an auditor? + # An admin current_user can demote them to either a student, tutor or convenor + # An admin current_user can promote them to an admin + auditor: { student: [:promote_user], + tutor: [:promote_user], + convenor: [:promote_user], + admin: [:promote_user] }, + # User being changed is a convenor? + # An admin current_user can demote them to student or tutor or auditor # An admin current_user can promote them to an admin convenor: { student: [:demote_user], tutor: [:demote_user], - admin: [:promote_user] }, - # User being assigned is a tutor? - # An admin current_user can demote them to a student - # An admin current_user can promote them to a convenor or admin + admin: [:promote_user], + auditor: [:demote_user] }, + # User being changed is a tutor? + # An admin current_user can demote them to a student or auditor + # An admin current_user can promote them to a convenor, admin or auditor tutor: { student: [:demote_user], convenor: [:promote_user], - admin: [:promote_user] }, + admin: [:promote_user], + auditor: [:demote_user] }, # User being assigned is a student? - # An admin current_user can promote them to a tutor, convenor or admin + # An admin current_user can promote them to a tutor, convenor, admin or auditor student: { tutor: [:promote_user], convenor: [:promote_user], - admin: [:promote_user] }, + admin: [:promote_user], + auditor: [:promote_user] }, # User being assigned has no role? # An admin current_user can create user to any role nil: { student: [:create_user], tutor: [:create_user], convenor: [:create_user], - admin: [:create_user] } + admin: [:create_user], + auditor: [:create_user] } }, # The current_user's role is a Convenor convenor: { @@ -260,26 +277,47 @@ def self.permissions # What can admins do with users? admin_role_permissions = [ :create_user, - :upload_csv, :list_users, + :update_user, + :get_user, + + :upload_csv, :download_system_csv, :download_unit_csv, - :update_user, + :create_unit, - :act_tutor, :admin_units, - :admin_users, :convene_units, - :download_stats, + :get_all_units, + :get_staff_list, + :rollover, + + :get_unit_roles, + :handle_teaching_period, :handle_campuses, :handle_activity_types, + :get_teaching_periods, - :rollover, + :admin_overseer, :use_overseer ] + # What can auditors do with users? + auditor_role_permissions = [ + :list_users, + :get_staff_list, + + :get_unit_roles, + :get_all_units, + + :audit_units, + + :get_teaching_periods, + :use_overseer + ] + # What can convenors do with users? convenor_role_permissions = [ :promote_user, @@ -290,16 +328,16 @@ def self.permissions :upload_csv, :download_unit_csv, :create_unit, - :act_tutor, + :get_unit_roles, :convene_units, - :download_stats, + :get_staff_list, :get_teaching_periods, :use_overseer ] # What can tutors do with users? tutor_role_permissions = [ - :act_tutor, + :get_unit_roles, :download_unit_csv, :get_teaching_periods ] @@ -314,6 +352,7 @@ def self.permissions { change_role: change_role_permissions, admin: admin_role_permissions, + auditor: auditor_role_permissions, convenor: convenor_role_permissions, tutor: tutor_role_permissions, student: student_role_permissions diff --git a/app/views/portfolio/portfolio_pdf.pdf.erb b/app/views/portfolio/portfolio_pdf.pdf.erb index e93df81ff..039c8036d 100644 --- a/app/views/portfolio/portfolio_pdf.pdf.erb +++ b/app/views/portfolio/portfolio_pdf.pdf.erb @@ -240,21 +240,22 @@ No Tutor <% if File.exist? task.portfolio_evidence_path # add task evidence to the document list for annotation extraction - document_list.append(task.portfolio_evidence_path) unless @is_retry + max_pages = FileHelper.pages_in_pdf(task.portfolio_evidence_path) - # embed in its entirety if more than 30 pages, otherwise embed page by page - if FileHelper.pages_in_pdf(task.portfolio_evidence_path) > 30 - %> -\includepdf[pages={1-},fitpaper]{<%= task.portfolio_evidence_path %>} - <% else %> - <% for page_idx in 1..FileHelper.pages_in_pdf(task.portfolio_evidence_path) do %> + # Limit to 100 pages + if max_pages > 100 + max_pages = 100 + else + document_list.append(task.portfolio_evidence_path) unless @is_retry + end + + for page_idx in 1..max_pages do %> \includepdf[pages={<%= page_idx %>-<%= page_idx %>},fitpaper]{<%= task.portfolio_evidence_path %>} - <% end # end for - end # end if many pages +<% end # end for end # end if file exists -end # portfolio tasks %> +end # portfolio tasks do -<% # generate lua calls for newpax and insert it in the preamble for annotation extraction +# generate lua calls for newpax and insert it in the preamble for annotation extraction content_for :preamble_newpax do %> \directlua { diff --git a/app/views/shared/_file.pdf.erb b/app/views/shared/_file.pdf.erb index b78c3e457..036b4b263 100644 --- a/app/views/shared/_file.pdf.erb +++ b/app/views/shared/_file.pdf.erb @@ -18,9 +18,10 @@ end %> -<% if file_type == 'document' %> - <% - for page_idx in 1..FileHelper.pages_in_pdf(file_name) do %> +<% if file_type == 'document' + max_pages = [100,FileHelper.pages_in_pdf(file_name)].min + + for page_idx in 1..max_pages do %> \includepdf[pages={<%= page_idx %>-<%= page_idx %>},fitpaper]{<%= file_name %>} <% end # end for %> diff --git a/config/application.rb b/config/application.rb index d2449dd24..df21df01b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -38,6 +38,12 @@ class Application < Rails::Application credentials.secret_key_aaf = ENV.fetch('DF_SECRET_KEY_AAF', Rails.env.production? ? nil : 'secretsecret12345') credentials.secret_key_moss = ENV.fetch('DF_SECRET_KEY_MOSS', nil) + # Limit number of pdf generators to run at once + config.pdfgen_max_processes = ENV['DF_MAX_PDF_GEN_PROCESSES'] || 2 + + # Date range for auditors to view + config.auditor_unit_access_years = ENV.fetch('DF_AUDITOR_UNIT_ACCESS_YEARS', 2).years + # ==> Institution settings # Institution YAML and ENV (override) config load config.institution = YAML.load_file("#{Rails.root}/config/institution.yml").with_indifferent_access diff --git a/config/environments/development.rb b/config/environments/development.rb index 0645f4a8c..0b4ebb164 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -43,6 +43,13 @@ config.cache_store = :null_store end + # Ensure cache is cleared on reload + unless Rails.application.config.cache_classes + Rails.autoloaders.main.on_unload do |klass, _abspath| + Rails.cache.clear + end + end + # Store uploaded files on the local file system (see config/storage.yml for options). # config.active_storage.service = :local diff --git a/db/migrate/20240528223908_add_auditor_role.rb b/db/migrate/20240528223908_add_auditor_role.rb new file mode 100644 index 000000000..88776569f --- /dev/null +++ b/db/migrate/20240528223908_add_auditor_role.rb @@ -0,0 +1,23 @@ +class AddAuditorRole < ActiveRecord::Migration[7.0] + # Add the auditor role + def up + return if Role.where(name: 'Auditor').first.present? + return if Role.where(id: 5).first.present? + + role = Role.create( + name: 'Auditor', + description: 'Auditors are able to view units but not change details.' + ) + + role.id = 5 + role.save + end + + # Remove the auditor role + def down + auditor_role = Role.where(id: 5).first + return unless auditor_role.present? + + auditor_role.destroy + end +end diff --git a/db/schema.rb b/db/schema.rb index 3a6368943..6672e53cc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_01_05_055902) do +ActiveRecord::Schema[7.0].define(version: 2024_05_28_223908) do create_table "activity_types", charset: "utf8", collation: "utf8_unicode_ci", force: :cascade do |t| t.string "name", null: false t.string "abbreviation", null: false diff --git a/lib/helpers/database_populator.rb b/lib/helpers/database_populator.rb index 1d95526f3..1d197a60c 100644 --- a/lib/helpers/database_populator.rb +++ b/lib/helpers/database_populator.rb @@ -288,6 +288,7 @@ def generate_fixed_data # Define fixed user data here @user_data = { acain: { first_name: "Andrew", last_name: "Cain", nickname: "Macite", role_id: Role.admin_id }, + aauditor: { first_name: "Auditor", last_name: "Auditor", nickname: "Auditor", role_id: Role.auditor_id }, aconvenor: { first_name: "Clinton", last_name: "Woodward", nickname: "The Giant", role_id: Role.convenor_id }, ajones: { first_name: "Allan", last_name: "Jones", nickname: "P-Jiddy", role_id: Role.admin_id }, rwilson: { first_name: "Reuben", last_name: "Wilson", nickname: "Reubs", role_id: Role.convenor_id }, @@ -568,7 +569,7 @@ def generate_tasks_for_unit(unit, unit_details) if (File.exist? csv_to_import) && (File.exist? zip_to_import) echo "----> CSV file found, importing tasks from #{csv_to_import} \n" - result = unit.import_tasks_from_csv File.open(csv_to_import) + result = unit.import_tasks_from_csv(File.open(csv_to_import)) unless result[:errors].empty? raise("----> Task import from CSV failed with the following errors: #{result[:errors]} \n") end @@ -578,9 +579,11 @@ def generate_tasks_for_unit(unit, unit_details) unless result[:errors].empty? raise("----> Task files import failed with the following errors: #{result[:errors]} \n") end + unless result[:ignored].empty? echo "----> Task files import ignored the following files: #{result[:ignored]} \n" end + return end diff --git a/lib/tasks/generate_pdfs.rake b/lib/tasks/generate_pdfs.rake index bf9d2c9af..c20c30e80 100644 --- a/lib/tasks/generate_pdfs.rake +++ b/lib/tasks/generate_pdfs.rake @@ -50,8 +50,9 @@ namespace :submission do end def should_run? - # Only run if there are not more than 4 processes running - old_executing(true).count < 4 + # Only run if there are not more than 3 processes running + max_processes = Rails.configuration.pdfgen_max_processes || 2 + old_executing(true).count < max_processes end def clean_up_failed_runs @@ -103,13 +104,15 @@ namespace :submission do # Rescue any projects that were orphaned by a previous process Project.where('NOT portfolio_generation_pid IS NULL').group(:portfolio_generation_pid).select('MIN(portfolio_generation_pid) as pid').map { |r| r['pid'] }.each do |pid| - next if pid == Process.pid + next if is_process_running?(pid) + # That process is not running... so pick up portfolios here Project.where(portfolio_generation_pid: pid).update_all(portfolio_generation_pid: Process.pid) end # Secure portfolios Project.where(compile_portfolio: true, portfolio_generation_pid: nil) + .limit(10) .update_all(portfolio_generation_pid: Process.pid) # Clean up any old failed runs - now after I have the files I need :) diff --git a/lib/tasks/init.rake b/lib/tasks/init.rake index c00ead3d4..8368440f3 100644 --- a/lib/tasks/init.rake +++ b/lib/tasks/init.rake @@ -10,7 +10,8 @@ namespace :db do { name: 'Student', description: "Students are able to be enrolled into units, and to submit progress for their unit projects." }, { name: 'Tutor', description: "Tutors are able to supervise tutorial classes and provide feedback to students, they may also be students in other units" }, { name: 'Convenor', description: "Convenors are able to create and manage units, as well as act as tutors and students." }, - { name: 'Admin', description: "Admin are able to create convenors, and act as convenors, tutors, and students in units." } + { name: 'Admin', description: "Admin are able to create convenors, and act as convenors, tutors, and students in units." }, + { name: 'Auditor', description: "Auditors are able to view only everything an admin can." } ] roles.each do |role| diff --git a/test/api/group_sets_api_test.rb b/test/api/group_sets_api_test.rb index bcc002e17..2d8c2778f 100644 --- a/test/api/group_sets_api_test.rb +++ b/test/api/group_sets_api_test.rb @@ -38,7 +38,7 @@ def test_post_add_a_new_groupset_to_a_unit_with_authorization # A groupSet we want to save new_group_set = FactoryBot.build(:group_set) - # Create a unit + # Create a unit new_unit = FactoryBot.create(:unit) # Data that we want to post @@ -58,11 +58,11 @@ def test_post_add_a_new_groupset_to_a_unit_with_authorization response_keys = %w(name allow_students_to_create_groups allow_students_to_manage_groups keep_groups_in_same_class) response_group_set = GroupSet.find(last_response_body['id']) assert_json_matches_model(response_group_set, last_response_body, response_keys) - assert_equal new_unit.id,response_group_set.unit.id - assert_equal new_group_set.name,response_group_set.name - assert_equal new_group_set.allow_students_to_create_groups,response_group_set.allow_students_to_create_groups - assert_equal new_group_set.allow_students_to_manage_groups,response_group_set.allow_students_to_manage_groups - assert_equal new_group_set.keep_groups_in_same_class,response_group_set.keep_groups_in_same_class + assert_equal new_unit.id, response_group_set.unit.id + assert_equal new_group_set.name, response_group_set.name + assert_equal new_group_set.allow_students_to_create_groups, response_group_set.allow_students_to_create_groups + assert_equal new_group_set.allow_students_to_manage_groups, response_group_set.allow_students_to_manage_groups + assert_equal new_group_set.keep_groups_in_same_class, response_group_set.keep_groups_in_same_class end def test_post_add_a_group_to_a_group_set_of_a_unit_without_authorization @@ -83,12 +83,12 @@ def test_post_add_a_group_to_a_group_set_of_a_unit_without_authorization unit_id: new_unit.id, group_set_id: new_group_set.id, group: { - name:new_group.name, - tutorial_id:new_tutorial.id + name: new_group.name, + tutorial_id: new_tutorial.id } } - add_auth_header_for user: User.first + add_auth_header_for user: User.find_by(username: 'student_1') # perform the POST post_json "/api/units/#{new_unit.id}/group_sets/#{new_group_set.id}/groups", data_to_post @@ -144,7 +144,7 @@ def test_get_all_groups_in_unit_without_authorization new_unit = new_group.group_set.unit get "/api/units/#{new_unit.id}/group_sets/#{new_group.group_set_id}/groups" - + # Check error code when an unauthorized user tries to get groups in a unit assert_equal 419, last_response.status, last_response_body end @@ -160,10 +160,10 @@ def test_get_all_groups_in_unit_with_authorization get "/api/units/#{new_unit.id}/group_sets/#{new_group.group_set_id}/groups" - #check returning number of groups + # check returning number of groups assert_equal new_group.group_set.groups.count, last_response_body.count - #Check response + # Check response response_keys = %w(id name) last_response_body.each do | data | grp = Group.find(data['id']) @@ -182,7 +182,7 @@ def test_get_groups_in_a_group_set_without_authorization # Obtain the unit from the group new_unit = new_group.group_set.unit - add_auth_header_for user: User.first + add_auth_header_for user: User.find_by(username: 'student_1') get "/api/units/#{new_unit.id}/group_sets/#{new_group_set.id}/groups" # Check error code @@ -214,7 +214,7 @@ def test_get_groups_in_a_group_set_with_authorization end assert_equal 200, last_response.status end - + def test_groups_unlocked_upon_creation unit = FactoryBot.create :unit @@ -247,7 +247,7 @@ def test_groups_lockable_only_by_staff group = Group.create!({group_set: group_set, name: 'test_groups_lockable_only_by_staff', tutorial: unit.tutorials.first }) group.save! group.add_member(unit.active_projects[0]) - + url = "api/units/#{unit.id}/group_sets/#{group_set.id}/groups/#{group.id}" lock_data = { group: { locked: true } } unlock_data = { group: { locked: false } } diff --git a/test/api/projects_api_test.rb b/test/api/projects_api_test.rb index 9e9dc0ff0..fda4b41b4 100644 --- a/test/api/projects_api_test.rb +++ b/test/api/projects_api_test.rb @@ -6,6 +6,7 @@ class ProjectsApiTest < ActiveSupport::TestCase include Rack::Test::Methods include TestHelpers::AuthHelper include TestHelpers::JsonHelper + include TestHelpers::TestFileHelper def app Rails.application @@ -34,7 +35,6 @@ def test_get_projects_with_streams_match assert_equal 1, last_response_body.count, last_response_body end - def test_projects_returns_correct_number_of_projects user = FactoryBot.create(:user, :student, enrol_in: 2) @@ -143,4 +143,53 @@ def test_submitted_grade_cant_change_after_submission assert_not_equal user.projects.find(project.id).submitted_grade, 1 assert_equal 403, last_response.status end + + def test_download_portfolio + project = FactoryBot.create(:project) + unit = project.unit + + project.portfolio_production_date = Time.zone.now + project.save + + `fallocate -l 10M #{project.portfolio_path}` + + assert File.exist?(project.portfolio_path) + assert project.portfolio_exists? + + data_to_put = { + as_attachment: true + } + + add_auth_header_for(user: project.student) + + get "/api/submission/project/#{project.id}/portfolio", data_to_put + assert_equal 200, last_response.status + assert last_response.header['Content-Disposition'].starts_with?('attachment; filename=') + assert last_response.header['Access-Control-Expose-Headers'] == 'Content-Disposition' + assert last_response.header['Content-Type'] == 'application/pdf' + assert 10_485_760, last_response.length + + `fallocate -l 11M #{project.portfolio_path}` + get "/api/submission/project/#{project.id}/portfolio", data_to_put + assert_equal 206, last_response.status + assert 10_485_760, last_response.length + + data_to_put = { + as_attachment: false + } + + add_auth_header_for(user: project.student) + header 'range', 'bytes=1000-1500' + + get "/api/submission/project/#{project.id}/portfolio", data_to_put + assert 500, last_response.length + assert_equal 206, last_response.status + assert_nil last_response.header['Content-Disposition'] + assert_nil last_response.header['Access-Control-Expose-Headers'] + assert last_response.header['Content-Type'] == 'application/pdf' + + unit.destroy! + ensure + FileUtils.rm_f(project.portfolio_path) + end end diff --git a/test/api/tasks_api_test.rb b/test/api/tasks_api_test.rb index b76e467ea..1e84f3f3c 100644 --- a/test/api/tasks_api_test.rb +++ b/test/api/tasks_api_test.rb @@ -295,12 +295,12 @@ def test_convenors_tutors_can_pin_and_unpin_tasks_students_admins_cannot # Student tries to pin task post "/api/tasks/#{task.id}/pin" - assert_equal last_response.status, 403 + assert_equal 403, last_response.status add_auth_header_for user: admin # Admin tries to pin task post "/api/tasks/#{task.id}/pin" - assert_equal last_response.status, 403 + assert_equal 403, last_response.status end def test_convenors_tutors_can_pin_tasks_of_their_units_only diff --git a/test/api/units_api_test.rb b/test/api/units_api_test.rb index 2fd2cf7d2..bdd73fc84 100644 --- a/test/api/units_api_test.rb +++ b/test/api/units_api_test.rb @@ -168,8 +168,7 @@ def test_add_tutorial_to_unit # Test GET for getting all units def test_units_get - - # Add username and auth_token to Header + # Add username and auth_token to Header - aadmin add_auth_header_for(user: User.first) get '/api/units' @@ -191,9 +190,69 @@ def test_units_get assert_equal expected_unit.end_date.to_date, actual_unit['end_date'].to_date end + def test_permissions_on_get + aadmin = User.find_by(username: 'aadmin') + aauditor = FactoryBot.create :user, :auditor + aconvenor = User.find_by(username: 'aconvenor') + atutor = User.find_by(username: 'atutor') + astudent = User.find_by(username: 'astudent') + + Doubtfire::Application.config.auditor_unit_access_years = 2.years + + # Test auditor can get all - except old + start_before = DateTime.now - Doubtfire::Application.config.auditor_unit_access_years - 1.year + start_inside = DateTime.now - Doubtfire::Application.config.auditor_unit_access_years + 1.week + + end_inside = DateTime.now - 1.week + end_after = DateTime.now + 1.week + + test_unit_before = FactoryBot.create :unit, start_date: start_before, end_date: end_inside, with_students: false, task_count: 0 + test_unit_inside = FactoryBot.create :unit, start_date: start_inside, end_date: end_inside, with_students: false, task_count: 0 + test_unit_after = FactoryBot.create :unit, start_date: start_inside, end_date: end_after, with_students: false, task_count: 0 + + total_units = Unit.count + + # Test admin can get all + add_auth_header_for(user: aadmin) + get '/api/units', { include_in_active: true } + assert_equal 200, last_response.status + assert_equal total_units, last_response_body.count + + assert last_response_body.map { |r| r['id'] }.include? test_unit_inside.id + assert last_response_body.map { |r| r['id'] }.include? test_unit_before.id + assert last_response_body.map { |r| r['id'] }.include? test_unit_after.id + + add_auth_header_for(user: aauditor) + get '/api/units', { include_in_active: true } + assert_equal 200, last_response.status + + assert last_response_body.map { |r| r['id'] }.include? test_unit_inside.id + refute last_response_body.map { |r| r['id'] }.include? test_unit_before.id + refute last_response_body.map { |r| r['id'] }.include? test_unit_after.id + + # Test convenor can not get all + add_auth_header_for(user: aconvenor) + get '/api/units' + assert_equal 403, last_response.status + + # Test tutor can not get all + add_auth_header_for(user: atutor) + get '/api/units' + assert_equal 403, last_response.status + + # Test students cannot get all + add_auth_header_for(user: astudent) + get '/api/units' + assert_equal 403, last_response.status + + # Test no auth cannot get all + clear_auth_header + get '/api/units' + assert_equal 419, last_response.status + end + # Test GET for getting a specific unit by id def test_units_get_by_id - # Add username and auth_token to Header add_auth_header_for(user: User.first) @@ -419,10 +478,8 @@ def test_put_update_unit_dates assert_equal new_end.to_i, Unit.first.end_date.to_i end - # Test GET for getting a specific unit by invalid id def test_fail_units_get_by_id - # Add username and auth_token to Header add_auth_header_for(user: User.first) @@ -431,10 +488,10 @@ def test_fail_units_get_by_id end def test_put_update_unit_custom_token() - unit= Unit.first - token='abcdef' + unit = Unit.first + token = 'abcdef' data_to_put = { - unit: unit + unit: unit } # Add username and auth_token to Header @@ -445,16 +502,16 @@ def test_put_update_unit_custom_token() end def test_put_update_unit_empty_token - unit= Unit.first + unit = Unit.first data_to_put = { - unit: unit + unit: unit } # Add username and auth_token to Header add_auth_header_for(user: User.first) # Override header for empty string - header 'auth_token','' + header 'auth_token', '' put_json '/api/units/1', data_to_put assert_equal 419, last_response.status @@ -489,7 +546,7 @@ def test_update_main_convenor end def test_draft_learning_summary_upload_requirements - unit = FactoryBot.create :unit, student_count:1, task_count:0 + unit = FactoryBot.create :unit, student_count: 1, task_count: 0 task_def_code = FactoryBot.create(:task_definition, unit: unit, upload_requirements: [{'key' => 'file0','name' => 'Code file','type' => 'code'}]) task_def_doc = FactoryBot.create(:task_definition, unit: unit, upload_requirements: [{'key' => 'file0','name' => 'Draft learning summary','type' => 'document'}]) task_def_doc_code = FactoryBot.create(:task_definition, unit: unit, upload_requirements: [{'key' => 'file0','name' => 'Draft learning summary','type' => 'document'}, {'key' => 'file1','name' => 'Code file','type' => 'code'}]) diff --git a/test/factories/users_factory.rb b/test/factories/users_factory.rb index 50a7fc53f..d706bf031 100644 --- a/test/factories/users_factory.rb +++ b/test/factories/users_factory.rb @@ -41,5 +41,9 @@ role { Role.convenor } end + trait :auditor do + role { Role.auditor } + end + end end diff --git a/test/helpers/auth_helper.rb b/test/helpers/auth_helper.rb index b81a7e4da..42537449b 100644 --- a/test/helpers/auth_helper.rb +++ b/test/helpers/auth_helper.rb @@ -39,8 +39,13 @@ def add_auth_header_for(user: User.first, username: nil, auth_token: nil) end end + def clear_auth_header + header 'username', nil + header 'auth_token', nil + end module_function :auth_token module_function :add_auth_header_for + module_function :clear_auth_header end end diff --git a/test/models/role_test.rb b/test/models/role_test.rb new file mode 100644 index 000000000..183d7906c --- /dev/null +++ b/test/models/role_test.rb @@ -0,0 +1,94 @@ +require "test_helper" + +class ProjectModelTest < ActiveSupport::TestCase + include TestHelpers::TestFileHelper + + # Test that there are 5 roles + def test_five_roles + assert_equal 5, Role.count + end + + # Test the student role + def test_student_role + role = Role.student + assert_equal 'Student', role.name + end + + # Test the tutor role + def test_tutor_role + role = Role.tutor + assert_equal 'Tutor', role.name + end + + # Test the convenor role + def test_convenor_role + role = Role.convenor + assert_equal 'Convenor', role.name + end + + # Test the admin role + def test_admin_role + role = Role.admin + assert_equal 'Admin', role.name + end + + # Test the auditor role + def test_auditor_role + role = Role.auditor + assert_equal 'Auditor', role.name + end + + # Test the to_sym method + def test_to_sym + role = Role.student + assert_equal :student, role.to_sym + + role = Role.tutor + assert_equal :tutor, role.to_sym + + role = Role.convenor + assert_equal :convenor, role.to_sym + + role = Role.admin + assert_equal :admin, role.to_sym + + role = Role.auditor + assert_equal :auditor, role.to_sym + end + + # Test with_name + def test_with_name + role = Role.with_name('Student') + assert_equal 'Student', role.name + + role = Role.with_name('Tutor') + assert_equal 'Tutor', role.name + + role = Role.with_name('Convenor') + assert_equal 'Convenor', role.name + + role = Role.with_name('Admin') + assert_equal 'Admin', role.name + + role = Role.with_name('Auditor') + assert_equal 'Auditor', role.name + + role = Role.with_name('asdf') + assert_nil role + + role = Role.with_name('student') + assert_equal 'Student', role.name + + role = Role.with_name('tutor') + assert_equal 'Tutor', role.name + + role = Role.with_name('convenor') + assert_equal 'Convenor', role.name + + role = Role.with_name('admin') + assert_equal 'Admin', role.name + + role = Role.with_name('auditor') + assert_equal 'Auditor', role.name + end +end diff --git a/test/models/unit_model_test.rb b/test/models/unit_model_test.rb index 5e4a40ee9..5ac3abe08 100644 --- a/test/models/unit_model_test.rb +++ b/test/models/unit_model_test.rb @@ -85,13 +85,13 @@ def test_sync_unit end def test_import_tasks_worked - @unit.import_tasks_from_csv File.open(Rails.root.join('test_files',"#{@unit.code}-Tasks.csv")) + @unit.import_tasks_from_csv File.open(Rails.root.join('test_files', "#{@unit.code}-Tasks.csv")) assert_equal 37, @unit.task_definitions.count, 'imported all task definitions' end def test_import_task_files - @unit.import_tasks_from_csv File.open(Rails.root.join('test_files',"#{@unit.code}-Tasks.csv")) - @unit.import_task_files_from_zip Rails.root.join('test_files',"#{@unit.code}-Tasks.zip") + @unit.import_tasks_from_csv File.open(Rails.root.join('test_files', "#{@unit.code}-Tasks.csv")) + @unit.import_task_files_from_zip Rails.root.join('test_files', "#{@unit.code}-Tasks.zip") @unit.task_definitions.each do |td| assert File.exist?(td.task_sheet), "#{td.abbreviation} task sheet missing" @@ -110,8 +110,8 @@ def test_import_task_files end def test_rollover_of_task_files - @unit.import_tasks_from_csv File.open(Rails.root.join('test_files',"#{@unit.code}-Tasks.csv")) - @unit.import_task_files_from_zip Rails.root.join('test_files',"#{@unit.code}-Tasks.zip") + @unit.import_tasks_from_csv File.open(Rails.root.join('test_files', "#{@unit.code}-Tasks.csv")) + @unit.import_task_files_from_zip Rails.root.join('test_files', "#{@unit.code}-Tasks.zip") unit2 = @unit.rollover TeachingPeriod.find(2), nil, nil