Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add direct download of proforma zip #2834

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion app/controllers/exercises_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
before_action :set_exercise_and_authorize,
only: MEMBER_ACTIONS + %i[clone implement working_times intervention statistics reload feedback
study_group_dashboard export_external_check export_external_confirm
external_user_statistics]
external_user_statistics download_proforma]
before_action :collect_set_and_unset_exercise_tags, only: MEMBER_ACTIONS
before_action :set_external_user_and_authorize, only: [:external_user_statistics]
before_action :set_file_types, only: %i[create edit new update]
Expand Down Expand Up @@ -181,6 +181,13 @@
render json: t('exercises.import_codeharbor.import_errors.internal_error'), status: :internal_server_error
end

def download_proforma
zip_file = ProformaService::ExportTask.call(exercise: @exercise)
send_data(zip_file.string, type: 'application/zip', filename: "exercise_#{@exercise.id}.zip", disposition: 'attachment')
rescue ProformaXML::PostGenerateValidationError => e
redirect_to :root, danger: JSON.parse(e.message).join('<br>')

Check warning on line 188 in app/controllers/exercises_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/exercises_controller.rb#L188

Added line #L188 was not covered by tests
end

def user_from_api_key
authorization_header = request.headers['Authorization']
api_key = authorization_header&.split&.second
Expand Down
2 changes: 1 addition & 1 deletion app/policies/exercise_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ def batch_update?
admin?
end

%i[show? feedback? statistics? external_user_statistics? rfcs_for_exercise?].each do |action|
%i[show? feedback? statistics? external_user_statistics? rfcs_for_exercise? download_proforma?].each do |action|
define_method(action) { admin? || teacher_in_study_group? || (teacher? && @record.public?) || author? }
end

Expand Down
1 change: 1 addition & 0 deletions app/views/exercises/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ h1 = Exercise.model_name.human(count: :other)
li = link_to(t('shared.destroy'), exercise, data: {confirm: t('shared.confirm_destroy')}, method: :delete, class: 'dropdown-item') if policy(exercise).destroy?
li = link_to(t('.clone'), clone_exercise_path(exercise), data: {confirm: t('shared.confirm_destroy')}, method: :post, class: 'dropdown-item') if policy(exercise).clone?
li = link_to(t('exercises.export_codeharbor.label'), '', class: 'dropdown-item export-start', data: {'exercise-id': exercise.id}) if policy(exercise).export_external_confirm?
li = link_to(t('exercises.download_proforma.label'), download_proforma_exercise_path(exercise), class: 'dropdown-item', target: '_blank', rel: 'noopener noreferrer') if policy(exercise).download_proforma?

= render('shared/pagination', collection: @exercises)
p = render('shared/new_button', model: Exercise)
Expand Down
1 change: 1 addition & 0 deletions app/views/exercises/show.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ h1.d-inline-block
li = link_to(t('shared.destroy'), @exercise, data: {confirm: t('shared.confirm_destroy')}, method: :delete, class: 'dropdown-item') if policy(@exercise).destroy?
li = link_to(t('exercises.index.clone'), clone_exercise_path(@exercise), data: {confirm: t('shared.confirm_destroy')}, method: :post, class: 'dropdown-item') if policy(@exercise).clone?
li = link_to(t('exercises.export_codeharbor.label'), '', class: 'dropdown-item export-start', data: {'exercise-id': @exercise.id}) if policy(@exercise).export_external_confirm?
li = link_to(t('exercises.download_proforma.label'), download_proforma_exercise_path(@exercise), class: 'dropdown-item', target: '_blank', rel: 'noopener noreferrer') if policy(@exercise).download_proforma?

= row(label: 'exercise.title', value: @exercise.title)
= row(label: 'exercise.internal_title', value: @exercise.internal_title)
Expand Down
2 changes: 2 additions & 0 deletions config/locales/de/exercise.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ de:
download_file_tree:
file_root: Erstellte Dateien
gone: Die angeforderte Datei konnte nicht abgerufen werden. Erstellte Dateien werden nur kurzzeitig vorgehalten und dann gelöscht. Bitte führen Sie den Code erneut aus und versuchen Sie dann wieder den Download der Datei.
download_proforma:
label: Download Proforma Zip
Copy link
Member

Choose a reason for hiding this comment

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

Based on our wording in CodeHarbor, I would recommend using ZIP with capital letters.

Suggested change
label: Download Proforma Zip
label: Download Proforma ZIP

In CodeHarbor, we also used the term "ProformaXML". However, this might be too long:

https://github.com/openHPI/codeharbor/blob/432e12f54365293137b439832908999dfff9985d/config/locales/de/proforma_errors.yml#L4

Alternatively, we dropped "ProformaXML" completely 🙈:

https://github.com/openHPI/codeharbor/blob/432e12f54365293137b439832908999dfff9985d/config/locales/de/common.yml#L14


Long story short: I would, at least, capitalize ZIP. On everything else, I am undecided.

editor:
collapse_action_sidebar: Aktions-Leiste Einklappen
collapse_output_sidebar: Ausgabe-Leiste Einklappen
Expand Down
2 changes: 2 additions & 0 deletions config/locales/en/exercise.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ en:
download_file_tree:
file_root: Generated Files
gone: The requested file could not be retrieved. Generated files are only held for a short time and are then deleted. Please run the code again and then try downloading the file a second time.
download_proforma:
label: Download Proforma Zip
Copy link
Member

Choose a reason for hiding this comment

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

See above.

editor:
collapse_action_sidebar: Collapse Action Sidebar
collapse_output_sidebar: Collapse Output Sidebar
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
get 'study_group_dashboard/:study_group_id', to: 'exercises#study_group_dashboard'
post :export_external_check
post :export_external_confirm
get :download_proforma
end

resources :programming_groups
Expand Down
30 changes: 30 additions & 0 deletions spec/controllers/exercises_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -518,4 +518,34 @@
end
end
end

describe 'GET #download_proforma' do
subject(:get_request) { get :download_proforma, params: {id: exercise.id} }

let(:zip) { instance_double(StringIO, string: 'dummy') }

before do
allow(ProformaService::ExportTask).to receive(:call).with(exercise:).and_return(zip)
end

it 'calls the ExportTask service' do
get_request
expect(ProformaService::ExportTask).to have_received(:call)
end

it 'sends the correct data' do
get_request
expect(response.body).to eql 'dummy'
end

it 'sets the correct Content-Type header' do
get_request
expect(response.header['Content-Type']).to eql 'application/zip'
end

it 'sets the correct Content-Disposition header' do
get_request
expect(response.header['Content-Disposition']).to include "attachment; filename=\"exercise_#{exercise.id}.zip\""
end
end
end