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
Feature/documents #8
Conversation
@@ -16,4 +16,12 @@ def summary | |||
def core_objective | |||
model.core_objective.html_safe | |||
end | |||
|
|||
def document_list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document_links
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea.
1048bf9
to
42739fb
Compare
app/decorators/document_decorator.rb
Outdated
class DocumentDecorator < Draper::Decorator | ||
delegate_all | ||
|
||
def name_link |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about renaming those methods ..
name_link
=> document_page_link
(or just document_link
)
open_link
=> document_url_link
.. to indicate where those links actually go.
@@ -0,0 +1,16 @@ | |||
class UrlValidator < ActiveModel::EachValidator | |||
def validate_each(record, attribute, value) | |||
return unless value.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we combine return unless value.present?
and ..unless url_valid?(value)
in single condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will make two separate guard clauses. I think 2 will be more readable.
app/validators/url_validator.rb
Outdated
end | ||
|
||
def url_valid?(url) | ||
url = begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think url = URI.parse(url) rescue false
would be enough here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubocop changed that automagically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let's not mess with RuboCop 🤖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already did. lol. I don't like that rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha ok )
this.wrapperClass = this.data.get('wrapperClass') || 'nested-fields'; | ||
} | ||
|
||
addRecord(event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code here looks like it shouldn't be directly related with "documents", rather, it is more like generic add/remove resource logic, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I will change that
The first iteration of documents upload added to Litigations
Document could be
TODO