E2603: Implement ViewSubmissions frontend#312
Open
CamBhat wants to merge 13 commits into
Open
Conversation
Danger has errored[!] Invalid Generated by 🚫 Danger |
CamBhat
changed the title
Author
|
Moved to draft to work through final project: Intending on
|
Introduce GET /submitted_content/:id/view_submissions in SubmittedContentController. The action finds the assignment by id (returns 404 if missing), builds a submissions array grouped by team (includes team id, name, members with full_name and email, and placeholder github/links/files), and returns assignment metadata with submissions in JSON (200 OK). Uses teams_users.includes(:user) to preload users.
Remove the AssignmentsController#view_submissions action (GET /assignments/:id/view_submissions). The deleted method built a submissions payload by enumerating assignment.teams and their members (including full_name and email, with empty github/links/files). This cleans up the controller by removing the now-unnecessary endpoint and its response construction.
Define additional collection routes for submitted_content (download, list_files, remove_hyperlink, submit_file, submit_hyperlink, folder_action) and move the existing view_submissions route into a member block. This restructures the routes to properly scope actions under the submitted_content resource and removes the previous standalone get route.
Delete the commented placeholder and the resources :submitted_content block from config/routes.rb. This removes the submitted_content collection routes (download, list_files, remove_hyperlink, submit_file, submit_hyperlink, folder_action), cleaning up the routing file—these endpoints appear deprecated or have been moved elsewhere.
Populate the view_submissions response with actual SubmissionRecord data instead of empty arrays. The controller now queries SubmissionRecord for a team+assignment, maps 'hyperlink' records to link objects (id, url, display_name, name, type='Hyperlink', modified) and 'file' records to file objects (id, basename as display_name/name, ext as type uppercased, modified). Add seed data to create sample hyperlink and file SubmissionRecord entries per team: sample URLs and file metadata are used to create records (and call team.submit_hyperlink where appropriate) with randomized created_at timestamps to help testing and demos.
Use external URLs for file records and generate internal download URLs only for local files. In SubmittedContentController, detect content starting with 'http' and expose it as the file URL; otherwise build an encoded internal download path. Update db/seeds.rb to use external sample file URLs (instead of local submission paths), store those URLs in SubmissionRecord.content, and fix assignment_id to use the team's parent_id. These changes allow seeding with real remote sample files and ensure correct assignment linkage.
Add a comprehensive RSpec request spec (spec/requests/view_submissions_spec.rb) for GET /submitted_content/:id/view_submissions. The new tests cover: assignment-not-found, assignments with no teams, teams with/without submission records, hyperlink and file submissions (including type/name extraction), filtering by assignment, multiple teams, teams with no members, and response shape contract. Test setup creates institutions, users, teams, and SubmissionRecord fixtures and uses JsonWebToken for auth.
Switched implementation from assignment_controller to submitted_content_controller
📝 WalkthroughWalkthroughIntroduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as SubmittedContentController
participant Helper as SubmittedContentHelper
participant DB as Database<br/>(SubmissionRecord)
participant FS as File System
Client->>Controller: GET /submitted_content/:id/view_submissions
Controller->>Controller: Load assignment (validate exists)
loop For each team
Controller->>DB: Query teams with users
Controller->>Helper: get_hyperlinks(team)
Helper->>DB: Query SubmissionRecord hyperlinks
DB-->>Helper: hyperlink records
Helper-->>Controller: link objects with id/modified
Controller->>Helper: get_files(team)
Helper->>FS: Enumerate files in team directory
Helper->>DB: Query SubmissionRecord files
DB-->>Helper: file records
Helper-->>Controller: file objects with id/url/metadata
end
Controller->>Client: Return JSON submissions response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Fix student deletion assignment
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
app/helpers/submitted_content_helper.rb (2)
265-294: Consider refactoring to reduce complexity.This method has high cyclomatic complexity (15 vs limit of 7) due to handling multiple input formats and parameter access patterns. Consider extracting helper methods:
- Extract param resolution (lines 266-271) into a dedicated method
- Extract the new-style path resolution (lines 273-285) into its own method
- Extract the legacy-style path resolution (lines 287-293) into its own method
✨ Suggested structure
def resolved_files_for_deletion if new_style_deletion_params? resolve_new_style_deletion_paths else resolve_legacy_deletion_paths end end private def new_style_deletion_params? current_folder_param.present? && delete_target_param.present? end def current_folder_param params.dig(:current_folder, :name) || params.dig('current_folder', 'name') || params[:current_folder] || params['current_folder'] end def delete_target_param params.dig(:faction, :delete) || params.dig('faction', 'delete') end def resolve_new_style_deletion_paths team = `@participant.team` team.set_team_directory_num current_folder = clean_folder(current_folder_param) base_path = team.path.to_s directory_path = current_folder == '/' ? base_path : File.join(base_path, current_folder) Array(delete_target_param).filter_map do |entry_name| sanitized_name = clean_filename(entry_name) File.join(directory_path, sanitized_name) if sanitized_name.present? end end def resolve_legacy_deletion_paths Array(params[:chk_files]).filter_map do |idx| directory = params.dig(:directories, idx) || params.dig(:directories, idx.to_s) filename = params.dig(:filenames, idx) || params.dig(:filenames, idx.to_s) next if directory.blank? || filename.blank? File.join(directory, filename) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/submitted_content_helper.rb` around lines 265 - 294, The resolved_files_for_deletion method is too complex; extract parameter resolution and path-resolution logic into helpers: create current_folder_param and delete_target_param methods that encapsulate the params.dig fallbacks, add new_style_deletion_params? to check current_folder_param.present? && delete_target_param.present?, move the new-style branch into resolve_new_style_deletion_paths (use participant_team and its set_team_directory_num, clean_folder, clean_filename, and build directory_path) and move the legacy branch into resolve_legacy_deletion_paths (the Array(params[:chk_files]) loop using params.dig for :directories and :filenames); then make resolved_files_for_deletion simply dispatch to resolve_new_style_deletion_paths or resolve_legacy_deletion_paths based on new_style_deletion_params?.
237-237: Line exceeds maximum length (155 > 120 characters).Extract the error message to a variable for readability.
✨ Suggested fix
else # File doesn't exist, return error - render json: { error: "Cannot delete '#{File.basename(file_path)}': File does not exist. It may have already been deleted." }, status: :not_found + error_msg = "Cannot delete '#{File.basename(file_path)}': File does not exist. " \ + "It may have already been deleted." + render json: { error: error_msg }, status: :not_found return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/submitted_content_helper.rb` at line 237, Extract the long inline string into a local variable before the render call to keep line length under 120; for example, assign a variable (e.g., error_message) to "Cannot delete '#{File.basename(file_path)}': File does not exist. It may have already been deleted." and then call render json: { error: error_message }, status: :not_found — locate the render statement using File.basename(file_path) and replace the inline message with the variable.spec/requests/view_submissions_spec.rb (2)
391-393: Missing final newline.Add a trailing newline at the end of the file for POSIX compliance.
🔧 Add trailing newline
end end end +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/view_submissions_spec.rb` around lines 391 - 393, The file spec/requests/view_submissions_spec.rb is missing a trailing newline at EOF; open that file and ensure there is a single newline character after the final "end" (i.e., add a blank line after the last end) so the file ends with a POSIX-compliant newline.
290-294: Minor: Unnecessary spacing on line 292.RuboCop flagged extra spacing. Remove the trailing spaces after
other_assignment.id.🔧 Fix spacing
- assignment_id: other_assignment.id # different assignment + assignment_id: other_assignment.id # different assignment🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/view_submissions_spec.rb` around lines 290 - 294, Remove the trailing whitespace after the `other_assignment.id` token in the test data declaration (the line inside the example that sets assignment_id: other_assignment.id) so the line ends cleanly with no extra spaces; edit the spec `view_submissions_spec.rb` block where `team_id: team.id, user: student1.name, assignment_id: other_assignment.id` is defined and delete the extra spaces at the end of that line.app/controllers/submitted_content_controller.rb (2)
456-465: Line 458 exceeds maximum length; consider breaking it up.The URL construction line is 156 characters (limit is 120). Additionally, the same ID collision issue exists here with
id: matching_record&.id || index + 1.♻️ Break long line and fix ID fallback
{ - id: matching_record&.id || index + 1, - url: "/submitted_content/download?download=#{URI.encode_uri_component(file_name)}¤t_folder[name]=#{URI.encode_uri_component(current_folder)}", + id: matching_record&.id || -(index + 1), + url: "/submitted_content/download?" \ + "download=#{URI.encode_uri_component(file_name)}&" \ + "current_folder[name]=#{URI.encode_uri_component(current_folder)}", display_name: file_name,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/submitted_content_controller.rb` around lines 456 - 465, The id fallback and the long URL line need fixing: replace the inline URL construction (the long string using URI.encode_uri_component) with building the encoded components into short variables (e.g., encoded_file_name and encoded_current_folder using URI.encode_www_form_component or URI.encode) and then interpolate them into a shorter url variable to keep lines <120 chars, and change the id expression from matching_record&.id || index + 1 to a collision-safe fallback such as matching_record&.id || SecureRandom.uuid (or another guaranteed-unique value) so ids cannot collide; update the hash keys (id and url) in the block accordingly (refer to the id: matching_record&.id || index + 1 and the url: "/submitted_content/download?download=..." lines).
305-338: Method complexity is acceptable given the feature scope.The
view_submissionsaction has clear linear logic assembling the response. Thegithub: ""hardcoded value appears intentional per the PR context. While RuboCop flags ABC/method-length metrics, this is a reasonable trade-off for readability in this view-assembly pattern.One minor observation: consider eager-loading teams to reduce queries.
♻️ Optional: Eager-load teams_users to reduce queries
- submissions = assignment.teams.map do |team| + submissions = assignment.teams.includes(teams_users: :user).map do |team|🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/submitted_content_controller.rb` around lines 305 - 338, The action does N+1 queries when iterating assignment.teams to load teams_users and their users; change the lookup to eager-load those associations by using Assignment.includes(teams: { teams_users: :user }).find_by(id: params[:id]) (keep the nil check and rest of view_submissions intact) so team.teams_users.includes(:user) no longer triggers extra queries.config/routes.rb (1)
43-56: Route configuration looks correct; minor indentation inconsistency.The new
view_submissionsmember route is correctly configured and will generate the expectedGET /submitted_content/:id/view_submissionspath. However, RuboCop flagged inconsistent indentation in this block compared to other route definitions in the file.🔧 Suggested indentation fix
- resources :submitted_content do - collection do - get :download - get :list_files - delete :remove_hyperlink - post :submit_file - post :submit_hyperlink - post :folder_action - end - - member do - get :view_submissions - end - end + resources :submitted_content do + collection do + get :download + get :list_files + delete :remove_hyperlink + post :submit_file + post :submit_hyperlink + post :folder_action + end + + member do + get :view_submissions + end + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/routes.rb` around lines 43 - 56, Indentation in the resources :submitted_content block is inconsistent with the rest of the routes file; reformat the block so inner keywords (collection and member) and their route lines align with the project's routing indentation style (match other resource blocks), ensuring the collection do and member do lines and their contained get/post/delete entries are indented the same way as other resource definitions and that get :view_submissions remains under member; update the spacing for uniformity without changing any route names or HTTP verbs.db/seeds.rb (2)
161-175: Avoid silent exception swallowing withrescue nil.The inline
rescue nilon line 164 silently suppresses all exceptions fromteam.submit_hyperlink(url), including network failures or unexpected validation errors. This can mask legitimate issues during seeding and make debugging harder.♻️ Consider logging exceptions instead of silently ignoring
- team.submit_hyperlink(url) rescue nil + begin + team.submit_hyperlink(url) + rescue StandardError => e + puts "Warning: Could not submit hyperlink for team #{team.id}: #{e.message}" + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/seeds.rb` around lines 161 - 175, Replace the silent inline rescue on the team.submit_hyperlink(url) call so exceptions aren’t swallowed; wrap the call in a begin/rescue block that catches StandardError, logs the error and backtrace (e.g. Rails.logger.error with the exception message and backtrace) including context (team.id, url, assignment_id), and then continue execution so the subsequent SubmissionRecord.create still runs or is skipped intentionally; update the code around team.submit_hyperlink(url) to use that explicit rescue handling instead of `rescue nil`.
177-186: Minor: Extra spacing on line 180.RuboCop flagged unnecessary spacing. The code is functional and creates valid file SubmissionRecord entries with all required fields.
🔧 Fix spacing
- content: file[:url], # store real URL instead of local path + content: file[:url], # store real URL instead of local path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/seeds.rb` around lines 177 - 186, RuboCop flagged extra spacing on the line inside the seed where SubmissionRecord.create is called; in the block that builds the file submission (the variable file and the call SubmissionRecord.create), remove the excessive spaces before the inline comment on the content assignment so it uses a single space (or move the comment to its own line) — specifically adjust the line "content: file[:url], # store real URL instead of local path" to eliminate the unnecessary spacing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/submitted_content_controller.rb`:
- Around line 421-432: The current mapping in team.hyperlinks.each_with_index
mixes database IDs (matching_record&.id) with index-based fallbacks (index + 1),
risking collisions; change the fallback to a non-colliding identifier
namespace—e.g., return id: (matching_record&.id || "temp-#{index + 1}") or id:
(matching_record&.id || - (index + 1))—so update the block that builds the hash
(the each_with_index loop using matching_record and hyperlink_records) to use a
string-prefixed or negative numeric fallback instead of a plain positive index.
- Around line 435-454: The current_team_files method is matching
SubmissionRecord entries by basename only which can produce false positives when
files share names in different folders; update the matching logic in
current_team_files to compare the record's path relative to team.path (e.g.,
normalize record.content.to_s to a full or relative path) against the computed
relative_path/current_folder plus file_name (or build the expected absolute path
using base_path + relative_path) so matching_record only returns when the
record's content path equals the file entry path (or its relative equivalent)
rather than just matching File.basename(record.content.to_s).
In `@spec/requests/view_submissions_spec.rb`:
- Around line 147-167: The test creates a SubmissionRecord but never populates
the team's YAML-backed hyperlinks, so current_team_links (which iterates
team.hyperlinks) yields nothing; update the spec to call
team.submit_hyperlink('https://github.com/student1/project') (or otherwise set
team.submitted_hyperlinks) before invoking get_view_submissions so that
team.hyperlinks contains the URL and the links array assertion will pass.
- Around line 201-236: The tests fail because current_team_files filters to
actual disk files (Dir.glob) and the controller builds download URLs, so the
DB-only SubmissionRecord with content 'https://example.com/...' won’t appear or
match; fix by either (A) in the spec create the expected physical file under the
team's submission folder before calling get_view_submissions and assert the
controller-generated download URL and file name (adjust expectations away from
the raw content URL), or (B) change the controller logic (where
current_team_files is used and where the files array and URL are constructed in
the view_submissions action) to also include SubmissionRecord entries that lack
a filesystem counterpart and preserve their original content URL/type when
building the files array—pick one approach and update the spec or the controller
accordingly.
---
Nitpick comments:
In `@app/controllers/submitted_content_controller.rb`:
- Around line 456-465: The id fallback and the long URL line need fixing:
replace the inline URL construction (the long string using
URI.encode_uri_component) with building the encoded components into short
variables (e.g., encoded_file_name and encoded_current_folder using
URI.encode_www_form_component or URI.encode) and then interpolate them into a
shorter url variable to keep lines <120 chars, and change the id expression from
matching_record&.id || index + 1 to a collision-safe fallback such as
matching_record&.id || SecureRandom.uuid (or another guaranteed-unique value) so
ids cannot collide; update the hash keys (id and url) in the block accordingly
(refer to the id: matching_record&.id || index + 1 and the url:
"/submitted_content/download?download=..." lines).
- Around line 305-338: The action does N+1 queries when iterating
assignment.teams to load teams_users and their users; change the lookup to
eager-load those associations by using Assignment.includes(teams: { teams_users:
:user }).find_by(id: params[:id]) (keep the nil check and rest of
view_submissions intact) so team.teams_users.includes(:user) no longer triggers
extra queries.
In `@app/helpers/submitted_content_helper.rb`:
- Around line 265-294: The resolved_files_for_deletion method is too complex;
extract parameter resolution and path-resolution logic into helpers: create
current_folder_param and delete_target_param methods that encapsulate the
params.dig fallbacks, add new_style_deletion_params? to check
current_folder_param.present? && delete_target_param.present?, move the
new-style branch into resolve_new_style_deletion_paths (use participant_team and
its set_team_directory_num, clean_folder, clean_filename, and build
directory_path) and move the legacy branch into resolve_legacy_deletion_paths
(the Array(params[:chk_files]) loop using params.dig for :directories and
:filenames); then make resolved_files_for_deletion simply dispatch to
resolve_new_style_deletion_paths or resolve_legacy_deletion_paths based on
new_style_deletion_params?.
- Line 237: Extract the long inline string into a local variable before the
render call to keep line length under 120; for example, assign a variable (e.g.,
error_message) to "Cannot delete '#{File.basename(file_path)}': File does not
exist. It may have already been deleted." and then call render json: { error:
error_message }, status: :not_found — locate the render statement using
File.basename(file_path) and replace the inline message with the variable.
In `@config/routes.rb`:
- Around line 43-56: Indentation in the resources :submitted_content block is
inconsistent with the rest of the routes file; reformat the block so inner
keywords (collection and member) and their route lines align with the project's
routing indentation style (match other resource blocks), ensuring the collection
do and member do lines and their contained get/post/delete entries are indented
the same way as other resource definitions and that get :view_submissions
remains under member; update the spacing for uniformity without changing any
route names or HTTP verbs.
In `@db/seeds.rb`:
- Around line 161-175: Replace the silent inline rescue on the
team.submit_hyperlink(url) call so exceptions aren’t swallowed; wrap the call in
a begin/rescue block that catches StandardError, logs the error and backtrace
(e.g. Rails.logger.error with the exception message and backtrace) including
context (team.id, url, assignment_id), and then continue execution so the
subsequent SubmissionRecord.create still runs or is skipped intentionally;
update the code around team.submit_hyperlink(url) to use that explicit rescue
handling instead of `rescue nil`.
- Around line 177-186: RuboCop flagged extra spacing on the line inside the seed
where SubmissionRecord.create is called; in the block that builds the file
submission (the variable file and the call SubmissionRecord.create), remove the
excessive spaces before the inline comment on the content assignment so it uses
a single space (or move the comment to its own line) — specifically adjust the
line "content: file[:url], # store real URL instead of local path" to
eliminate the unnecessary spacing.
In `@spec/requests/view_submissions_spec.rb`:
- Around line 391-393: The file spec/requests/view_submissions_spec.rb is
missing a trailing newline at EOF; open that file and ensure there is a single
newline character after the final "end" (i.e., add a blank line after the last
end) so the file ends with a POSIX-compliant newline.
- Around line 290-294: Remove the trailing whitespace after the
`other_assignment.id` token in the test data declaration (the line inside the
example that sets assignment_id: other_assignment.id) so the line ends cleanly
with no extra spaces; edit the spec `view_submissions_spec.rb` block where
`team_id: team.id, user: student1.name, assignment_id: other_assignment.id` is
defined and delete the extra spaces at the end of that line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d0c603e-f9c1-4e2e-94ce-24f73957f7ec
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (7)
app/controllers/assignments_controller.rbapp/controllers/submitted_content_controller.rbapp/helpers/submitted_content_helper.rbconfig/routes.rbdb/schema.rbdb/seeds.rbspec/requests/view_submissions_spec.rb
Comment on lines
+421
to
+432
| team.hyperlinks.each_with_index.map do |hyperlink, index| | ||
| matching_record = hyperlink_records.find { |record| record.content == hyperlink } | ||
|
|
||
| { | ||
| id: matching_record&.id || index + 1, | ||
| url: hyperlink, | ||
| display_name: hyperlink, | ||
| name: hyperlink, | ||
| type: 'Hyperlink', | ||
| modified: matching_record&.created_at || team.updated_at | ||
| } | ||
| end |
There was a problem hiding this comment.
Potential ID collisions when mixing record IDs with index-based fallbacks.
The pattern id: matching_record&.id || index + 1 mixes database IDs (which could be any integer) with sequential indices starting at 1. If a record exists with ID 2 and another hyperlink falls back to index + 1 = 2, you'd have duplicate IDs in the response.
Consider using a different fallback strategy or a separate identifier namespace.
🛡️ Suggested fix using negative indices for fallbacks
- {
- id: matching_record&.id || index + 1,
+ {
+ id: matching_record&.id || -(index + 1),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| team.hyperlinks.each_with_index.map do |hyperlink, index| | |
| matching_record = hyperlink_records.find { |record| record.content == hyperlink } | |
| { | |
| id: matching_record&.id || index + 1, | |
| url: hyperlink, | |
| display_name: hyperlink, | |
| name: hyperlink, | |
| type: 'Hyperlink', | |
| modified: matching_record&.created_at || team.updated_at | |
| } | |
| end | |
| team.hyperlinks.each_with_index.map do |hyperlink, index| | |
| matching_record = hyperlink_records.find { |record| record.content == hyperlink } | |
| { | |
| id: matching_record&.id || -(index + 1), | |
| url: hyperlink, | |
| display_name: hyperlink, | |
| name: hyperlink, | |
| type: 'Hyperlink', | |
| modified: matching_record&.created_at || team.updated_at | |
| } | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/submitted_content_controller.rb` around lines 421 - 432, The
current mapping in team.hyperlinks.each_with_index mixes database IDs
(matching_record&.id) with index-based fallbacks (index + 1), risking
collisions; change the fallback to a non-colliding identifier namespace—e.g.,
return id: (matching_record&.id || "temp-#{index + 1}") or id:
(matching_record&.id || - (index + 1))—so update the block that builds the hash
(the each_with_index loop using matching_record and hyperlink_records) to use a
string-prefixed or negative numeric fallback instead of a plain positive index.
Comment on lines
+435
to
+454
| def current_team_files(team, assignment_id) | ||
| team.set_team_directory_num | ||
| base_path = team.path.to_s | ||
| return [] unless File.directory?(base_path) | ||
|
|
||
| file_records = SubmissionRecord | ||
| .where(team_id: team.id, assignment_id: assignment_id, record_type: 'file') | ||
| .order(created_at: :desc) | ||
|
|
||
| file_entries = Dir.glob(File.join(base_path, '**', '*')).select { |entry| File.file?(entry) }.sort | ||
|
|
||
| file_entries.each_with_index.map do |entry, index| | ||
| relative_path = entry.delete_prefix("#{base_path}/") | ||
| current_folder = File.dirname(relative_path) | ||
| current_folder = current_folder == '.' ? '/' : "/#{current_folder}" | ||
| file_name = File.basename(relative_path) | ||
| matching_record = | ||
| file_records.find do |record| | ||
| record.content.to_s == entry || File.basename(record.content.to_s) == file_name | ||
| end |
There was a problem hiding this comment.
File matching by basename alone may cause false matches.
The matching logic on lines 452-454 checks if File.basename(record.content.to_s) == file_name. If two files in different folders share the same name (e.g., /folder1/report.pdf and /folder2/report.pdf), the first record found will match both files incorrectly.
Consider including the folder path in the match criteria.
🛡️ Suggested fix to include path in matching
matching_record =
file_records.find do |record|
- record.content.to_s == entry || File.basename(record.content.to_s) == file_name
+ record.content.to_s == entry ||
+ record.content.to_s.end_with?("/#{relative_path}")
end🧰 Tools
🪛 RuboCop (1.86.1)
[convention] 435-466: Assignment Branch Condition size for current_team_files is too high. [<12, 32, 11> 35.9/20]
(Metrics/AbcSize)
[convention] 435-466: Cyclomatic complexity for current_team_files is too high. [9/7]
(Metrics/CyclomaticComplexity)
[convention] 435-466: Method has too many lines. [26/20]
(Metrics/MethodLength)
[convention] 435-466: Perceived complexity for current_team_files is too high. [9/8]
(Metrics/PerceivedComplexity)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/submitted_content_controller.rb` around lines 435 - 454, The
current_team_files method is matching SubmissionRecord entries by basename only
which can produce false positives when files share names in different folders;
update the matching logic in current_team_files to compare the record's path
relative to team.path (e.g., normalize record.content.to_s to a full or relative
path) against the computed relative_path/current_folder plus file_name (or build
the expected absolute path using base_path + relative_path) so matching_record
only returns when the record's content path equals the file entry path (or its
relative equivalent) rather than just matching
File.basename(record.content.to_s).
Comment on lines
+147
to
+167
| describe 'when team has hyperlink submission records' do | ||
| let!(:hyperlink_record) do | ||
| SubmissionRecord.create!( | ||
| record_type: 'hyperlink', | ||
| content: 'https://github.com/student1/project', | ||
| operation: 'Submit Hyperlink', | ||
| team_id: team.id, | ||
| user: student1.name, | ||
| assignment_id: assignment.id | ||
| ) | ||
| end | ||
|
|
||
| it 'returns the hyperlink in the links array' do | ||
| get_view_submissions(assignment.id) | ||
|
|
||
| expect(response).to have_http_status(:ok) | ||
| links = response.parsed_body['submissions'].first['links'] | ||
| expect(links.length).to eq(1) | ||
| expect(links.first['url']).to eq('https://github.com/student1/project') | ||
| expect(links.first['type']).to eq('Hyperlink') | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how current_team_links works - it iterates over team.hyperlinks
rg -n -B2 -A10 "def current_team_links" app/controllers/submitted_content_controller.rb
# Check if team.hyperlinks returns SubmissionRecords or stored YAML
rg -n -A5 "def hyperlinks" app/models/assignment_team.rbRepository: expertiza/reimplementation-back-end
Length of output: 783
Test will fail because team.hyperlinks is not populated.
The current_team_links method iterates over team.hyperlinks.each_with_index, which is a YAML-loaded list from the submitted_hyperlinks field. The test creates a SubmissionRecord but never calls team.submit_hyperlink(url) to add the hyperlink to the team's YAML-stored list. As a result, team.hyperlinks will be empty [], the iteration will yield no results, and the test will fail at line 164 expecting links.length to equal 1.
Add team.submit_hyperlink('https://github.com/student1/project') to the setup or modify the test to populate team.hyperlinks before calling get_view_submissions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/requests/view_submissions_spec.rb` around lines 147 - 167, The test
creates a SubmissionRecord but never populates the team's YAML-backed
hyperlinks, so current_team_links (which iterates team.hyperlinks) yields
nothing; update the spec to call
team.submit_hyperlink('https://github.com/student1/project') (or otherwise set
team.submitted_hyperlinks) before invoking get_view_submissions so that
team.hyperlinks contains the URL and the links array assertion will pass.
Comment on lines
+201
to
+236
| describe 'when team has file submission records' do | ||
| let!(:file_record) do | ||
| SubmissionRecord.create!( | ||
| record_type: 'file', | ||
| content: 'https://example.com/uploads/report.pdf', | ||
| operation: 'Submit File', | ||
| team_id: team.id, | ||
| user: student1.name, | ||
| assignment_id: assignment.id | ||
| ) | ||
| end | ||
|
|
||
| it 'returns the file in the files array' do | ||
| get_view_submissions(assignment.id) | ||
|
|
||
| expect(response).to have_http_status(:ok) | ||
| files = response.parsed_body['submissions'].first['files'] | ||
| expect(files.length).to eq(1) | ||
| expect(files.first['name']).to eq('report.pdf') | ||
| expect(files.first['url']).to eq('https://example.com/uploads/report.pdf') | ||
| end | ||
|
|
||
| it 'correctly extracts file extension as type' do | ||
| get_view_submissions(assignment.id) | ||
|
|
||
| files = response.parsed_body['submissions'].first['files'] | ||
| expect(files.first['type']).to eq('PDF') | ||
| end | ||
|
|
||
| it 'does not include files in the links array' do | ||
| get_view_submissions(assignment.id) | ||
|
|
||
| links = response.parsed_body['submissions'].first['links'] | ||
| expect(links).to eq([]) | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any test setup that creates actual files or mocks the filesystem
rg -n "FileUtils|File\.write|File\.open.*'w'|tempfile|tmp" spec/requests/view_submissions_spec.rb
# Check the controller implementation to confirm it only uses Dir.glob
rg -n -A5 "Dir\.glob" app/controllers/submitted_content_controller.rbRepository: expertiza/reimplementation-back-end
Length of output: 446
🏁 Script executed:
# Get the full current_team_files method implementation
rg -B5 "def current_team_files" app/controllers/submitted_content_controller.rb | head -40
# Then get more lines after the Dir.glob to see how SubmissionRecord is used
sed -n '440,500p' app/controllers/submitted_content_controller.rbRepository: expertiza/reimplementation-back-end
Length of output: 1366
🏁 Script executed:
# Find base_path definition in the controller
rg -n "base_path" app/controllers/submitted_content_controller.rb | head -10
# Check for any test setup, fixtures, or mocking in the test file
sed -n '1,200p' spec/requests/view_submissions_spec.rb | rg -n "before|let!|fixture|mock|stub|Dir|File"Repository: expertiza/reimplementation-back-end
Length of output: 787
🏁 Script executed:
# Check for mocking of Dir.glob or File operations in the test file
rg -n "allow.*Dir|allow.*File|double|stub|mock" spec/requests/view_submissions_spec.rb
# Check the Team model for path definition
rg -n "def path" app/models/team.rb
# Look at the full test setup section
sed -n '1,100p' spec/requests/view_submissions_spec.rbRepository: expertiza/reimplementation-back-end
Length of output: 3076
🏁 Script executed:
# Find Team model and path definition
rg -n "def path|has_one.*path|path =" app/models/team.rb | head -10
# Check if Team has callbacks that create directories
rg -n "after_create|before_create|FileUtils" app/models/team.rb
# Also check AssignmentTeam model
rg -n "def path|after_create|before_create|FileUtils" app/models/assignment_team.rb
# Look at the actual file submission test in detail
sed -n '201,240p' spec/requests/view_submissions_spec.rbRepository: expertiza/reimplementation-back-end
Length of output: 1430
🏁 Script executed:
# Get the full path method definition from Team model
sed -n '70,85p' app/models/team.rb
# Also check if team directories are created anywhere (e.g., in controller, model callbacks)
rg -n "FileUtils.mkdir|Dir.mkdir|directory" app/models/team.rb
rg -n "FileUtils.mkdir|Dir.mkdir|directory" app/models/assignment_team.rbRepository: expertiza/reimplementation-back-end
Length of output: 1055
🏁 Script executed:
# Find assignment_path definition
rg -n "def assignment_path" app/models/team.rb
# Check if test setup creates team directories
rg -n "FileUtils.mkdir|Dir.mkdir|setup_files" spec/requests/view_submissions_spec.rb
# Also verify what assignment_path returns
rg -B2 -A2 "assignment_path" app/models/team.rbRepository: expertiza/reimplementation-back-end
Length of output: 61
🏁 Script executed:
# Search all models for assignment_path definition
rg -n "def assignment_path" app/models/
# Check Team model inheritance
head -20 app/models/team.rb
# Also check if path is correctly constructed - look for full Team implementation
rg -B5 -A5 "def path" app/models/team.rbRepository: expertiza/reimplementation-back-end
Length of output: 879
🏁 Script executed:
# Search entire codebase for assignment_path
rg -n "def assignment_path" app/
# Check AssignmentTeam model specifically
cat app/models/assignment_team.rb
# Also check if path method works with belongs_to :assignment
rg -B5 -A10 "def path" app/models/assignment_team.rbRepository: expertiza/reimplementation-back-end
Length of output: 9374
🏁 Script executed:
# Check the view_submissions controller method to see if it calls current_team_files
rg -B5 -A20 "def view_submissions" app/controllers/submitted_content_controller.rb
# Also check if there's any recent git history that might explain this
git log --oneline -20 app/controllers/submitted_content_controller.rb 2>/dev/null || echo "No git history available"Repository: expertiza/reimplementation-back-end
Length of output: 985
🏁 Script executed:
# Check Assignment model path definition
rg -n "def path" app/models/assignment.rb
# Confirm the exact URL generation in current_team_files
sed -n '460,470p' app/controllers/submitted_content_controller.rbRepository: expertiza/reimplementation-back-end
Length of output: 270
🏁 Script executed:
# Get full Assignment path method
sed -n '36,50p' app/models/assignment.rb
# Also verify the exact line where controller generates the URL
sed -n '455,465p' app/controllers/submitted_content_controller.rbRepository: expertiza/reimplementation-back-end
Length of output: 1222
Tests will fail: Missing filesystem files and URL format mismatch.
The tests create SubmissionRecord entries with URL content ('https://example.com/uploads/report.pdf') but expect specific behavior that the controller cannot provide:
-
Missing filesystem files: The
current_team_filesmethod only returns files that physically exist on disk viaDir.glob. Since no actual files are created in the test setup, the files array will be empty, failing the assertionexpect(files.length).to eq(1). -
URL format mismatch: Even if files existed, the controller generates URLs in the format
/submitted_content/download?download=...¤t_folder[name]=...(line 455), not the original content URL fromSubmissionRecord. The test expectsfiles.first['url']to equal'https://example.com/uploads/report.pdf', which will never match.
Either the tests need to create actual files in the team's filesystem directory, or the controller needs to be refactored to include file records that exist only in the database with their original URLs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/requests/view_submissions_spec.rb` around lines 201 - 236, The tests
fail because current_team_files filters to actual disk files (Dir.glob) and the
controller builds download URLs, so the DB-only SubmissionRecord with content
'https://example.com/...' won’t appear or match; fix by either (A) in the spec
create the expected physical file under the team's submission folder before
calling get_view_submissions and assert the controller-generated download URL
and file name (adjust expectations away from the raw content URL), or (B) change
the controller logic (where current_team_files is used and where the files array
and URL are constructed in the view_submissions action) to also include
SubmissionRecord entries that lack a filesystem counterpart and preserve their
original content URL/type when building the files array—pick one approach and
update the spec or the controller accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
E2603: Implement ViewSubmissions frontend
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests