Skip to content
This repository has been archived by the owner on Oct 2, 2019. It is now read-only.

[Delivers #89433964] File upload #299

Merged
merged 8 commits into from
Apr 21, 2015
Merged
Show file tree
Hide file tree
Changes from 5 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
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,6 @@ coverage
# Ignore Cloud Foundry files
cf-ssh.yml
*manifest.yml

# Ignore file uploads
/public/system
2 changes: 2 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ gem 'acts_as_list'
gem 'ar_outer_joins'
gem 'autoprefixer-rails'
gem 'awesome_print'
gem 'aws-sdk-v1'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why v1? aws-sdk is the newer one, which it seems like Paperclip supports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite yet: thoughtbot/paperclip#1764

Copy link
Contributor

Choose a reason for hiding this comment

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

Far enough. Mind adding an inline comment linking to that issue?

gem 'bootstrap-sass', '~> 3.3.0'
gem 'dotenv-rails', require: 'dotenv/rails-now'
gem 'draper'
Expand All @@ -16,6 +17,7 @@ gem 'jquery-rails'
gem 'jquery-turbolinks'
gem 'newrelic_rpm'
gem 'omniauth-myusa', git: 'https://github.com/18F/omniauth-myusa.git'
gem "paperclip", "~> 4.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

There are differing opinions on this, but I would leave the version restriction off unless there's a specific reason to lock the range.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the very least, for consistency.

gem 'pg'
gem 'pundit'
gem 'rack-cors', require: 'rack/cors'
Expand Down
14 changes: 14 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ GEM
autoprefixer-rails (4.0.2.2)
execjs
awesome_print (1.6.1)
aws-sdk-v1 (1.60.2)
json (~> 1.4)
nokogiri (>= 1.4.4)
bootstrap-sass (3.3.1.0)
sass (~> 3.2)
builder (3.2.2)
Expand All @@ -71,7 +74,11 @@ GEM
xpath (~> 2.0)
celluloid (0.16.0)
timers (~> 4.0.0)
climate_control (0.0.3)
activesupport (>= 3.0)
cliver (0.3.2)
cocaine (0.5.7)
climate_control (>= 0.0.3, < 1.0)
codeclimate-test-reporter (0.4.7)
simplecov (>= 0.7.1, < 1.0.0)
coderay (1.1.0)
Expand Down Expand Up @@ -186,6 +193,11 @@ GEM
multi_json (~> 1.3)
oauth2 (~> 1.0)
omniauth (~> 1.2)
paperclip (4.2.1)
activemodel (>= 3.0.0)
activesupport (>= 3.0.0)
cocaine (~> 0.5.3)
mime-types
pg (0.18.1)
poltergeist (1.6.0)
capybara (~> 2.1)
Expand Down Expand Up @@ -325,6 +337,7 @@ DEPENDENCIES
ar_outer_joins
autoprefixer-rails
awesome_print
aws-sdk-v1
bootstrap-sass (~> 3.3.0)
capybara
codeclimate-test-reporter
Expand All @@ -343,6 +356,7 @@ DEPENDENCIES
mail_view
newrelic_rpm
omniauth-myusa!
paperclip (~> 4.2)
pg
poltergeist
pry-byebug
Expand Down
2 changes: 1 addition & 1 deletion app/assets/stylesheets/common/_constants.scss
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ $bgdetails: #efefef;
$borderdetails: #bdbdbd;
$lightgray: #838383;
$medgray: #545454;

$break-small: 480px;
4 changes: 4 additions & 0 deletions app/assets/stylesheets/common/communicarts.scss
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,10 @@ body.webapp {
text-align: center;
}

.righted {
text-align: right;
}

.text-area-info-container {
border: 1px solid #bdbdbd;
margin-left: 0px;
Expand Down
63 changes: 34 additions & 29 deletions app/assets/stylesheets/webapp/webviews.scss
Original file line number Diff line number Diff line change
Expand Up @@ -213,38 +213,55 @@ tr.cart_item_information p {
color: $lightgray;
}

.cart-comments-container {
.proposal-submodel-container {
width: 100%;
max-width: $web-content-width;
background-color: $bgdetails;
border-top: 1px solid $borderdetails;
padding: 65px 0;
padding: 0 0 65px 0;
padding-left: $webpadding;
padding-right: $webpadding;
margin: 0 auto;
@media (max-width: $break-small) {
padding: 10px;
}

#cart-comments {
margin: 0 auto;
input[type=submit] {
color: $white;
background-color: $c2blue;
border: 0;
padding: 15px;

#add_a_comment {
color: $white;
background-color: $c2blue;
border: 0;
padding: 15px;
@media(max-width: $break-small) {
padding: 10px;
}
}

#cart-comments {
margin: 0 auto;

textarea {
width: 100%;
}
}

.comment-item {
border-bottom: 1px solid #ccc;
padding: 20px 40px;
background-color: #fff;
h3 {
padding-top: 65px;
@media (max-width: $break-small) {
padding-top: 10px;
}
}

.date {
color: $medgray;
}

.comment-date {
color: $medgray;
}
.line-item {
border-bottom: 1px solid #ccc;
padding: 20px 40px;
background-color: #fff;
@media(max-width: $break-small) {
padding: 10px 15px;
}
}
}
Expand Down Expand Up @@ -367,7 +384,7 @@ body.webapp {
color: #666;
}

@media (max-width: 480px) {
@media (max-width: $break-small) {
tr.cart_item_information p {
font-size: 67%;
}
Expand All @@ -384,25 +401,13 @@ body.webapp {
margin-bottom: 10px;
}

.cart-comments-container #cart-comments .comment-item {
padding: 10px 15px;
}

.text-area-info-web {
padding: 4px 4px;
p {
font-size: .9em;
line-height: 1.1em;
}
}
.cart-comments-container #cart-comments #add_a_comment {
padding: 10px;
}

.cart-comments-container {
padding: 10px;
}



h1 {
Expand Down
30 changes: 30 additions & 0 deletions app/controllers/attachments_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
class AttachmentsController < ApplicationController
before_filter :authenticate_user!
before_filter ->{authorize self.proposal, :can_show!}
rescue_from Pundit::NotAuthorizedError, with: :auth_errors

def create
attachment = self.proposal.attachments.build(attachments_params)
attachment.user = current_user
if attachment.save
flash[:success] = "You successfully added a attachment"
else
flash[:error] = attachment.errors.full_messages
end

redirect_to proposal.cart
end

protected
def proposal
@cached_proposal ||= Cart.find(params[:cart_id]).proposal
end

def attachments_params
params.require(:attachment).permit(:file)
end

def auth_errors(exception)
redirect_to carts_path, :alert => "You are not allowed to see that cart"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "you aren't allowed to add an attachment to that proposal"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do

end
end
2 changes: 1 addition & 1 deletion app/controllers/carts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class CartsController < ApplicationController
def show
@cart = self.cart.decorate
@proposal = self.cart.proposal
@show_comments = true
@include_comments_files = true
end

def index
Expand Down
11 changes: 11 additions & 0 deletions app/models/attachment.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class Attachment < ActiveRecord::Base
has_attached_file :file
do_not_validate_attachment_file_type :file

validates_presence_of :file
validates_presence_of :proposal
validates_presence_of :user

belongs_to :proposal
belongs_to :user
end
1 change: 1 addition & 0 deletions app/models/proposal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class Proposal < ActiveRecord::Base
has_one :cart
has_many :approvals
has_many :approvers, through: :approvals, source: :user
has_many :attachments
has_many :comments
has_many :observations
has_many :observers, through: :observations, source: :user
Expand Down
57 changes: 49 additions & 8 deletions app/views/carts/_cart.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,10 @@



<div class="cart-comments-container">
<div id="cart-comments">
<h3>Comments on this purchase request</h3>
<%- if @show_comments %>
<%- if @include_comments_files %>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that we need this anymore, but that can be a separate discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's loop in @phirefly ; I think he added this initially.

<div class="cart-comments-container proposal-submodel-container">
<div id="cart-comments">
<h3>Comments on this purchase request</h3>
<%= form_for [cart, Comment.new] do |f| %>
<%= f.text_area :comment_text, rows: 5 %>

Expand All @@ -96,14 +96,14 @@

<% if cart.comments.any? %>
<% cart.comments.each do |c| %>
<div class='comment-item'>
<div class='line-item'>
<div class='row'>
<% unless c.user.nil? %>
<p class='comment-sender col-sm-6 col-xs-12'>
<strong><%= c.user_full_name %></strong>
</p>
<% end %>
<p class='comment-date col-sm-6 col-xs-12'>
<p class='date col-sm-6 col-xs-12'>
<%= date_with_tooltip(c.created_at) %>
</p>
</div>
Expand All @@ -120,9 +120,50 @@
No comments have been added yet
</p>
<% end %>
<%- end %>
</div>

<div id="files">
<h3>Attachments to this proposal</h3>
<%= form_for [cart, Attachment.new] do |f| %>
<div class="line-item">
<div class="row">
<%= f.file_field :file %>
</div>
</div>
<div class='row text-area-info-container'>
<div class='col-xs-7 col-sm-6 text-area-info-web'>
<p>
Attaching a file will associate it with this proposal. Files cannot be removed
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that the last part is necessary, even if it's true for the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will delete

</p>
</div>
<p class='col-xs-5 col-sm-6 text-area-button'>
<%= submit_tag "Attach a File", id: :add_a_file %>
</p>
</div>
<%- end %>
<% cart.proposal.attachments.each do |attachment| %>
<div class="line-item">
<div class="row">
<p class="col-sm-6 col-xs-12">
<a href="<%= attachment.file.expiring_url(10*60) %>"><%= attachment.file_file_name %></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the magic number here? Maybe move to a helper or decorator or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. I'll just replace override file_url.

Magic number is for an arbitrary timeout: 10 minutes.

</p>
<p class="col-sm-3 col-xs-6">
<strong><%= attachment.user.full_name %></strong>
</p>
<p class="col-sm-3 col-xs-6 date righted">
<%= date_with_tooltip(attachment.created_at) %>
</p>
</div>
</div>
<% end %>
<% if cart.proposal.attachments.empty? %>
<p class="empty-list-label">
No attachments have been added yet
</p>
<% end %>
</div>
</div>
</div>
<%- end %>

<% if policy(cart.proposal).can_approve_or_reject? %>
<%= render partial: 'approval_actions', locals: { current_user: @current_user, cart: cart} %>
Expand Down
13 changes: 13 additions & 0 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,18 @@ class Application < Rails::Application
config.autoload_paths << Rails.root.join('lib')

config.assets.precompile << 'common/communicarts.css'

# Paperclip's attachment settings are determined by S3 env vars
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention that it will be saved to the filesystem otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do

if ENV['S3_BUCKET_NAME'] && ENV['S3_ACCESS_KEY_ID'] && ENV['S3_SECRET_ACCESS_KEY']
Paperclip::Attachment.default_options.merge!(
bucket: ENV['S3_BUCKET_NAME'],
s3_credentials: {
access_key_id: ENV['S3_ACCESS_KEY_ID'],
secret_access_key: ENV['S3_SECRET_ACCESS_KEY']
},
s3_permissions: :private,
storage: :s3,
)
end
end
end
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
end

resources :comments, only: [:index, :create]
resources :attachments, only: [:create]
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well put this under /proposals/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just waiting for that shoe to drop :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not wait.

end

namespace :ncr do
Expand Down
10 changes: 10 additions & 0 deletions db/migrate/20150415165247_create_attachments.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class CreateAttachments < ActiveRecord::Migration
def change
create_table :attachments do |t|
t.attachment :file
t.references :proposal
t.references :user
t.timestamps
end
end
end
Loading