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

4414 - Fix typo in bulk export api #4415

Open
wants to merge 2 commits into
base: development
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion app/controllers/api/api_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ def help
render plain: api_routes.map {|r| "#{r.verb}\t#{r.path.spec}" }.join("\n")
end
end
end
end
141 changes: 63 additions & 78 deletions app/controllers/api/v1/bulk_export_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,60 +17,55 @@ def index
else
exports = @api_user.bulk_exports
end
render json: exports.to_json(except: [:updated_at, :user_id, :collection_id], include: {:collection => {only: [:title, :slug]}})
render json: exports.to_json(except: [:updated_at, :user_id, :collection_id], include: { collection: { only: [:title, :slug] } })
else
render status: 401, json: 'You must use an API token to access bulk exports'
end
end


def start
if @api_user
collection_slug = params[:collection_slug]
if collection_slug
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is redundant checking. The route requires /:collection_id so this will always be present. It should suffice that we check if collection.nil? instead.

Copy link
Owner

Choose a reason for hiding this comment

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

That sounds true, but what are we doing differently with a collection_slug parameter versus the usual collection_id (which should populate @collection?

collection = Collection.where(slug: collection_slug).first
if collection.nil?
render status: 404, json: "No collection exists with the slug #{collection_slug}"
return
end
collection = Collection.where(slug: collection_slug).first
Copy link
Owner

Choose a reason for hiding this comment

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

I'm still surprised that @collection isn't automatically loaded here from the ApplicationController before filter. Can we check to see if it is and save a line of code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We only have :set_api_user in the filter. We also expect :collection_slug instead of :collection_id so it won't be set using our before filters.

I think what we have now is ideal. We want to throw the message render status: 404, json: "No collection exists with the slug #{collection_slug}" since this is an API endpoint rather than a webpage. This allows user to know the error in their api calls.

Copy link
Owner

Choose a reason for hiding this comment

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

You've convinced me.

if collection.nil?
render status: 404, json: "No collection exists with the slug #{collection_slug}"
return
end

if collection.show_to?(@api_user)
# try to parse the bulk_export
bulk_export = BulkExport.new
bulk_export.collection = collection
bulk_export.user = @api_user
bulk_export.status = BulkExport::Status::NEW
bulk_export.plaintext_verbatim_page = !(params[:plaintext_verbatim_page].blank?)
bulk_export.plaintext_verbatim_work = !(params[:plaintext_verbatim_work].blank?)
bulk_export.plaintext_emended_page = !(params[:plaintext_emended_page].blank?)
bulk_export.plaintext_emended_work = !(params[:plaintext_emended_work].blank?)
bulk_export.plaintext_searchable_page = !(params[:plaintext_searchable_page].blank?)
bulk_export.plaintext_searchable_work = !(params[:plaintext_searchable_work].blank?)
bulk_export.tei_work = !(params[:tei_work].blank?)
bulk_export.html_page = !(params[:html_page].blank?)
bulk_export.html_work = !(params[:html_work].blank?)
bulk_export.subject_csv_collection = !(params[:subject_csv_collection].blank?)
bulk_export.subject_details_csv_collection = !(params[:subject_details_csv_collection].blank?)
bulk_export.table_csv_collection = !(params[:table_csv_collection].blank?)
bulk_export.table_csv_work = !(params[:table_csv_work].blank?)
bulk_export.notes_csv = !(params[:notes_csv].blank?)
bulk_export.save
bulk_export.submit_export_process
if collection.show_to?(@api_user)
# try to parse the bulk_export
bulk_export = BulkExport.new
bulk_export.collection = collection
bulk_export.user = @api_user
bulk_export.status = BulkExport::Status::NEW
bulk_export.plaintext_verbatim_page = !params[:plaintext_verbatim_page].blank?
Copy link
Owner

Choose a reason for hiding this comment

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

Can you remind me what the precedence is of ! versus .blank? ? I'm trying to figure out why we added parentheses here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

blank? function produces boolean. We just invert the boolean value here.

Ideally I'd like to change this to .present? instead so we don't need to invert. But I kept this similar to old implementation as I may have thought this is a preferential thing. Do you want me to change it to present?

Copy link
Owner

Choose a reason for hiding this comment

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

Sure! I wasn't aware that present? existed universally.

bulk_export.plaintext_verbatim_work = !params[:plaintext_verbatim_work].blank?
bulk_export.plaintext_emended_page = !params[:plaintext_emended_page].blank?
bulk_export.plaintext_emended_work = !params[:plaintext_emended_work].blank?
bulk_export.plaintext_searchable_page = !params[:plaintext_searchable_page].blank?
bulk_export.plaintext_searchable_work = !params[:plaintext_searchable_work].blank?
bulk_export.tei_work = !params[:tei_work].blank?
bulk_export.html_page = !params[:html_page].blank?
bulk_export.html_work = !params[:html_work].blank?
bulk_export.subject_csv_collection = !params[:subject_csv_collection].blank?
bulk_export.subject_details_csv_collection = !params[:subject_details_csv_collection].blank?
bulk_export.table_csv_collection = !params[:table_csv_collection].blank?
bulk_export.table_csv_work = !params[:table_csv_work].blank?
bulk_export.notes_csv = !params[:notes_csv].blank?
bulk_export.save
bulk_export.submit_export_process

response = {
:id => bulk_export.id,
:status => bulk_export.status,
:status_uri => api_v1_bulk_export_status_url(bulk_export.id),
:download_uri => api_v1_bulk_export_download_url(bulk_export.id)
}
response = {
id: bulk_export.id,
status: bulk_export.status,
status_uri: api_v1_bulk_export_status_url(bulk_export.id),
download_uri: api_v1_bulk_export_download_url(bulk_export.id)
}

render status: 202, json: response.to_json
render status: 202, json: response.to_json

else
render status: 403, json: 'User #{@api_user} is not authorized to view #{collection.title}'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is one of the status typo. We are not interpolating the string here due to single quotes

Copy link
Owner

Choose a reason for hiding this comment

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

Let's fix it!

end
else
render status: 401, json: 'You must use a collection slug in the URL to create a bulk export'
render status: 403, json: "User #{@api_user} is not authorized to view #{collection.title}"
end
else
render status: 401, json: 'You must use an API token to access bulk exports'
Expand All @@ -80,59 +75,49 @@ def start
def status
if @api_user
bulk_export_id = params[:bulk_export_id]
if bulk_export_id
bulk_export = @api_user.bulk_exports.where(:id => bulk_export_id).first
if bulk_export
response = {
:id => bulk_export.id,
:status => bulk_export.status,
:status_uri => api_v1_bulk_export_status_url(bulk_export.id),
:download_uri => api_v1_bulk_export_download_url(bulk_export.id)
}
render status: 200, json: response.to_json
else
render status: 403, json: 'User #{@api_user} has no bulk export with ID #{bulk_export_id}'
end
bulk_export = @api_user.bulk_exports.find_by(id: bulk_export_id)

if bulk_export
response = {
id: bulk_export.id,
status: bulk_export.status,
status_uri: api_v1_bulk_export_status_url(bulk_export.id),
download_uri: api_v1_bulk_export_download_url(bulk_export.id)
}
render status: 200, json: response.to_json
else
render status: 401, json: 'You must use a bulk export ID in the URL'
render status: 403, json: "User #{@api_user} has no bulk export with ID #{bulk_export_id}"
end
else
render status: 401, json: 'You must use an API token to access bulk exports'
end
end


def download
if @api_user
bulk_export_id = params[:bulk_export_id]
if bulk_export_id
bulk_export = @api_user.bulk_exports.where(:id => bulk_export_id).first
if bulk_export
if bulk_export.status == BulkExport::Status::FINISHED
send_file(bulk_export.zip_file_name,
filename: "fromthepage_export.zip",
:content_type => "application/zip")
else
if bulk_export.status == BulkExport::Status::CLEANED
render status: 410, json: "Bulk export #{bulk_export_id} has been deleted. Please start a new export."
elsif bulk_export.status == BulkExport::Status::ERROR
render status: 410, json: "Bulk export #{bulk_export_id} failed with an error. Please report this to support. Re-running an export with different requested formats might succeed."
else
render status: 409, json: "Bulk export #{bulk_export_id} is not ready to download. It is probably still running, but might have failed with an un-caught error."
end
end
bulk_export = @api_user.bulk_exports.find_by(id: bulk_export_id)
if bulk_export
case bulk_export.status
when BulkExport::Status::FINISHED
send_file(
bulk_export.zip_file_name,
filename: 'fromthepage_export.zip',
content_type: 'application/zip'
)
when BulkExport::Status::CLEANED
render status: 410, json: "Bulk export #{bulk_export_id} has been deleted. Please start a new export."
when BulkExport::Status::ERROR
render status: 410, json: "Bulk export #{bulk_export_id} failed with an error. Please report this to support. Re-running an export with different requested formats might succeed."
else
render status: 403, json: 'User #{@api_user} has no bulk export with ID #{bulk_export_id}'
render status: 409, json: "Bulk export #{bulk_export_id} is not ready to download. It is probably still running, but might have failed with an un-caught error."
end
else
render status: 401, json: 'You must use a bulk export ID in the URL'
render status: 403, json: "User #{@api_user} has no bulk export with ID #{bulk_export_id}"
end
else
render status: 401, json: 'You must use an API token to access bulk exports'
end
end

private

end
end
end
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@
namespace :v1 do
get 'bulk_export', to: 'bulk_export#index'
get 'bulk_export/:collection_slug', to: 'bulk_export#index'
post 'bulk_export/:collection_slug', to: 'bulk_export#start'
post 'bulk_export/:collection_slug', to: 'bulk_export#start', as: 'bulk_export_start'
get 'bulk_export/:bulk_export_id/status', to: 'bulk_export#status', as: 'bulk_export_status'
get 'bulk_export/:bulk_export_id/download', to: 'bulk_export#download', as: 'bulk_export_download'
end
Expand Down
30 changes: 30 additions & 0 deletions spec/factories/bulk_export.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
FactoryBot.define do
factory :bulk_export do
collection_id { association(:collection).id }
user_id { association(:user).id }

trait :new do
status { BulkExport::Status::NEW }
end

trait :queued do
status { BulkExport::Status::QUEUED }
end

trait :processing do
status { BulkExport::Status::PROCESSING }
end

trait :finished do
status { BulkExport::Status::FINISHED }
end

trait :cleaned do
status { BulkExport::Status::CLEANED }
end

trait :error do
status { BulkExport::Status::ERROR }
end
end
end
19 changes: 19 additions & 0 deletions spec/factories/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,24 @@
sequence(:display_name) { |n| "admin_#{n}_login" }
sequence(:login) { |n| "admin_#{n}_login" }
end

factory :unique_user do
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Took the chance to add a temporary solution to one of my minor annoyances with Factories when combined with transactional_fixtures = false specs.

The issue is, doing just sequence(:display_name) { |n| "admin_#{n}_login" } without resetting the database each time we run specs IS FAILING at times, due to n here not accounting for duplicates (since each specs run does not delete the created data necessarily)

Using SecureRandom should work here. Initially I planned to add this to the already defined factory bots above, but it caused some legacy specs (especially Devise specs which needed to query created user by user_#{index}_login). So I decided we should add a sub-factory here :unique_user so it will always create a unique user regardless if we reset test db or not.

sequence(:login) { "user_#{SecureRandom.hex(2)}_login" }
sequence(:email) { "user_#{SecureRandom.hex(2)}@sample.com" }
password { 'password' }
password_confirmation { 'password' }

trait :with_api_key do
api_key { User.generate_api_key }
end

trait :owner do
admin { true }
end

trait :admin do
admin { true }
end
end
end
end
2 changes: 1 addition & 1 deletion spec/interactors/article/combine_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require 'spec_helper'

describe Article::Combine do
let(:user) { User.find_by(login: OWNER) }
let!(:user) { create(:unique_user, :owner) }
let!(:collection) { create(:collection, owner_user_id: user.id) }
let!(:work) { create(:work, collection: collection, owner_user_id: user.id) }
let!(:from_related_page) { create(:page, work: work, source_text: '[[Duplicate]]', source_translation: '[[Duplicate]]') }
Expand Down
4 changes: 2 additions & 2 deletions spec/interactors/article/destroy_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require 'spec_helper'

describe Article::Destroy do
let(:user) { User.find_by(login: OWNER) }
let!(:user) { create(:unique_user, :owner) }
let!(:collection) { create(:collection, owner_user_id: user.id) }
let!(:article) { create(:article, collection: collection) }

Expand All @@ -25,7 +25,7 @@
end

context 'when user is not owner' do
let(:other_user) { User.find_by(login: NEW_OWNER) }
let!(:other_user) { create(:unique_user) }

let(:result) do
described_class.call(
Expand Down
2 changes: 1 addition & 1 deletion spec/interactors/article/update_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require 'spec_helper'

describe Article::Update do
let(:user) { User.find_by(login: OWNER) }
let!(:user) { create(:unique_user, :owner) }
let!(:collection) { create(:collection, owner_user_id: user.id) }
let!(:work) { create(:work, collection: collection, owner_user_id: user.id) }
let!(:related_page) { create(:page, work: work, source_text: '[[Original]]', source_translation: '[[Original]]') }
Expand Down
2 changes: 1 addition & 1 deletion spec/interactors/work/refresh_metadata_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require 'spec_helper'

describe Work::RefreshMetadata do
let(:owner) { User.find_by(login: OWNER) }
let!(:owner) { create(:unique_user, :owner) }
let(:collection) { create(:collection, owner_user_id: owner.id) }
let(:original_metadata) { [{ label: 'en', value: ['Original Metadata'] }].to_json }
let(:at_id) { 'http://example.com/manifest' }
Expand Down
16 changes: 16 additions & 0 deletions spec/requests/api/api_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
require 'spec_helper'

describe Api::ApiController do
describe '#help' do
let(:action_path) { api_path }

let(:subject) { get action_path }

it 'renders status and plain_text' do
subject

expect(response).to have_http_status(:ok)
expect(response.content_type).to eq('text/plain; charset=utf-8')
end
end
end
Loading
Loading