Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
AO3-4516 Make remaining absolute urls, relative paths #2395
Conversation
zz9pzza
added some commits
Mar 11, 2016
zz9pzza
added
the
Awaiting review
label
Mar 12, 2016
sarken
reviewed
Mar 14, 2016
| @@ -126,7 +126,7 @@ def non_anonymous_byline(creation) | ||
| end | ||
| def pseud_link(pseud) |
sarken
Mar 14, 2016
Owner
This is used in byline, which is used in a lot of places, some of which need the full URL -- have you checked RSS feeds, downloads, emails, and share code to make sure that they still have the full URL? I just did a spot check and while the share code looks okay, an HTML download of a work has a broken link for the byline, giving me only <a href="/users/testy2/pseuds/testy2" rel="author">testy2</a> when it needs to be the full URL.
sarken
reviewed
Mar 14, 2016
| @@ -76,7 +76,7 @@ def link_to_tag_works(tag, options = {}) | ||
| end | ||
| def link_to_tag_with_text(tag, link_text, options = {}) | ||
| - link_to_with_tag_class(@collection ? collection_tag_url(@collection, tag) : tag_url(tag), link_text, options) | ||
| + link_to_with_tag_class(@collection ? collection_tag_path(@collection, tag) : tag_path(tag), link_text, options) |
sarken
Mar 14, 2016
Owner
I'm pretty sure these are okay, but while you're checking on the byline change, it would be a good idea to also do a quick check of tag links in emails, RSS feeds, downloads, and the share code just to be sure.
sarken
added
Reviewed: Needs Coder Action
and removed
Awaiting review
labels
Mar 14, 2016
|
Thanks sarken I will look at that and fix it, I can't see an obvious way of testing the content of the downloads. We might be able to do it for html. If we had multiple urls we supported, would the correct way be to have a set url at the front ( eg a download generated via a "insecure or secure" url should have the canonical url for the site be from the archive config "APP_URL" |
zz9pzza
added some commits
Mar 15, 2016
hlieberman
commented
Mar 18, 2016
|
For what it's worth, the branch that's currently on test.a.o seems to have solved a mixed-content issue that happens with the user icons off of the s3 bucket when you expand comments on a work. |
|
I have done the downloads. I have checked the rss and that those need to be absolute.The share code is absolute. |
zz9pzza
added
Coder has actioned review
and removed
Reviewed: Needs Coder Action
labels
Mar 19, 2016
sarken
reviewed
Mar 20, 2016
| @@ -76,7 +76,11 @@ def link_to_tag_works(tag, options = {}) | ||
| end | ||
| def link_to_tag_with_text(tag, link_text, options = {}) | ||
| - link_to_with_tag_class(@collection ? collection_tag_path(@collection, tag) : tag_path(tag), link_text, options) | ||
| + if options[:full_path] |
sarken
Mar 20, 2016
Owner
Would it be possible to do something like this to make it DRYer?
def link_to_tag_with_text(tag, link_text, options = {})
collection_url_for_tag = options[:full_path] ? collection_tag_url(@collection, tag) : collection_tag_path(@collection, tag)
link_to_with_tag_class(@collection ? collection_url_for_tag : tag_path(tag), link_text, options)
end
My other question is do we definitely always want tag_path, or should it be tag_url when options[:full_path]?
sarken
Mar 25, 2016
Owner
Actually, ignore my question about DRYness -- it's so much easier to read the way it is, I think we can probably leave it. >.>
However, the question about path vs. url remains.
zz9pzza
Mar 25, 2016
Contributor
If we want the full url then we should se the option to be full_path otherwise not :)
sarken
reviewed
Mar 20, 2016
| @@ -134,7 +134,7 @@ def download_url_for_work(work, format) | ||
| # Generates a list of a work's tags and details for use in feeds | ||
| def feed_summary(work) | ||
| tags = work.tags.group_by(&:type) | ||
| - text = "<p>by #{byline(work, visibility: 'public')}</p>" | ||
| + text = "<p>by #{byline(work, { visibility: 'public', full_path: true})}</p>" |
sarken
reviewed
Mar 20, 2016
| @@ -152,7 +152,7 @@ def feed_summary(work) | ||
| else | ||
| type.pluralize | ||
| end | ||
| - text << "<li>#{label}: #{tags[type].map{ |t| link_to_tag_works(t) }.join(', ')}</li>" | ||
| + text << "<li>#{label}: #{tags[type].map{ |t| link_to_tag_works(t,{full_path: true}) }.join(', ')}</li>" |
sarken
reviewed
Mar 20, 2016
| @@ -90,15 +90,16 @@ def byline(creation, options={}) | ||
| if creation.respond_to?(:anonymous?) && creation.anonymous? | ||
| anon_byline = ts("Anonymous") | ||
| if (logged_in_as_admin? || is_author_of?(creation)) && options[:visibility] != 'public' | ||
| - anon_byline += " [".html_safe + non_anonymous_byline(creation) + "]".html_safe | ||
| + anon_byline += " [".html_safe + non_anonymous_byline(creation,options[:only_path]) + "]".html_safe |
sarken
reviewed
Mar 20, 2016
| - expand_all = content_tag(:a, ts("Expand All"), :href=>"#", :class => "expand_all", "target_class" => target, :role => "button") | ||
| - contract_all = content_tag(:a, ts("Contract All"), :href=>"#", :class => "contract_all", "target_class" => target, :role => "button") | ||
| - content_tag(:span, expand_all + "\n".html_safe + contract_all, :class => "actions hidden showme", :role => "menu") | ||
| + expand_all = content_tag(:a, ts("Expand All"), href:"#", class: "expand_all", "target_class" => target, role: "button") |
sarken
reviewed
Mar 20, 2016
| - contract_all = content_tag(:a, ts("Contract All"), :href=>"#", :class => "contract_all", "target_class" => target, :role => "button") | ||
| - content_tag(:span, expand_all + "\n".html_safe + contract_all, :class => "actions hidden showme", :role => "menu") | ||
| + expand_all = content_tag(:a, ts("Expand All"), href:"#", class: "expand_all", "target_class" => target, role: "button") | ||
| + contract_all = content_tag(:a, ts("Contract All"), href:"#", class: "contract_all", "target_class" => target, role: "button") |
sarken
added
Reviewed: Needs Coder Action
and removed
Coder has actioned review
labels
Mar 22, 2016
zz9pzza
added some commits
Mar 25, 2016
zz9pzza
added
Coder has actioned review
and removed
Reviewed: Needs Coder Action
labels
Mar 25, 2016
|
The code looks okay to me reading it over but I am definitely leery of whether the existing tests adequately cover this change. Would there be any way to crawl the live production AO3 site to find all the instances of "http://(www.)?archiveofourown" that currently exist so we can check them? |
shalott
added
Reviewed: Needs Coder Action
and removed
Coder has actioned review
labels
Apr 10, 2016
All checks have passed
houndci-bot
reviewed
May 8, 2016
| @@ -152,7 +152,7 @@ def feed_summary(work) | ||
| else | ||
| type.pluralize | ||
| end | ||
| - text << "<li>#{label}: #{tags[type].map{ |t| link_to_tag_works(t) }.join(', ')}</li>" | ||
| + text << "<li>#{label}: #{tags[type].map{ |t| link_to_tag_works(t, {full_path: true }) }.join(', ')}</li>" |
houndci-bot
May 8, 2016
Redundant curly braces around a hash parameter.
Space missing to the left of {.
Space inside { missing.
houndci-bot
reviewed
May 8, 2016
| @@ -134,7 +134,7 @@ def download_url_for_work(work, format) | ||
| # Generates a list of a work's tags and details for use in feeds | ||
| def feed_summary(work) | ||
| tags = work.tags.group_by(&:type) | ||
| - text = "<p>by #{byline(work, visibility: 'public')}</p>" | ||
| + text = "<p>by #{byline(work, { visibility: 'public', full_path: true })}</p>" |
houndci-bot
reviewed
May 8, 2016
| @@ -79,7 +79,7 @@ def allowed_css_instructions | ||
| # see http://stackoverflow.com/questions/2425690/multiple-remote-form-for-on-the-same-page-causes-duplicate-ids | ||
| def field_with_unique_id( form, field_type, object, field_name ) | ||
| field_id = "#{object.class.name.downcase}_#{object.id.to_s}_#{field_name.to_s}" | ||
| - form.send( field_type, field_name, :id => field_id ) | ||
| + form.send( field_type, field_name, id: field_id ) |
houndci-bot
reviewed
May 8, 2016
| }.merge(options) | ||
| field_name = options[:field_name] || field_name(form, attribute) | ||
| field_name += '[]' | ||
| base_id = options[:field_id] || field_id(form, attribute) | ||
| checkboxes_id = "#{base_id}_checkboxes" | ||
| - opts = options[:disabled] ? {:disabled => "true"} : {} | ||
| + opts = options[:disabled] ? {disabled: "true"} : {} |
houndci-bot
reviewed
May 8, 2016
| - form.fields_for(nested_model_name, new_nested_model, :child_index => child_index) {|child_form| | ||
| - render(:partial => partial_to_render, :locals => {:form => child_form, :index => child_index}.merge(locals)) | ||
| + form.fields_for(nested_model_name, new_nested_model, child_index: child_index) {|child_form| | ||
| + render(partial: partial_to_render, locals: {form: child_form, index: child_index}.merge(locals)) |
houndci-bot
reviewed
May 8, 2016
| toggle = "".html_safe | ||
| if options[:include_toggle] && !options[:concise] && size > (ArchiveConfig.OPTIONS_TO_SHOW * 6) | ||
| toggle = checkbox_section_toggle(checkboxes_id, size) | ||
| end | ||
| # We wrap the whole thing in a div | ||
| - return content_tag(:div, checkboxes_ul + toggle + (options[:include_blank] ? hidden_field_tag(field_name, " ") : ''.html_safe), :id => checkboxes_id) | ||
| + return content_tag(:div, checkboxes_ul + toggle + (options[:include_blank] ? hidden_field_tag(field_name, " ") : ''.html_safe), id: checkboxes_id) |
houndci-bot
reviewed
May 8, 2016
| else | ||
| - link_to_unless params[:sort_column].nil?, title, url_for(params.merge :sort_column => nil, :sort_direction => nil) | ||
| + link_to_unless params[:sort_column].nil?, title, url_for(params.merge sort_column: nil, sort_direction: nil) |
houndci-bot
May 8, 2016
Add parentheses to nested method call params.merge sort_column: nil, sort_direction: nil.
houndci-bot
reviewed
May 8, 2016
| end | ||
| # see above | ||
| def link_to_remove_section(linktext, form, class_of_section_to_remove="removeme") | ||
| form.hidden_field(:_destroy) + "\n" + | ||
| - link_to_function(linktext, "remove_section(this, \"#{class_of_section_to_remove}\")", :class => "hidden showme") | ||
| + link_to_function(linktext, "remove_section(this, \"#{class_of_section_to_remove}\")", class: "hidden showme") |
houndci-bot
reviewed
May 8, 2016
| @@ -512,8 +513,8 @@ def checkbox_section(form, attribute, choices, options = {}) | ||
| end | ||
| value = choice.send(options[:value_method]) | ||
| checkbox_id = "#{base_id}_#{name_to_id(value)}" | ||
| - checkbox = check_box_tag(field_name, value, is_checked, opts.merge({:id => checkbox_id})) | ||
| - checkbox_and_label = label_tag checkbox_id, :class => "action" do | ||
| + checkbox = check_box_tag(field_name, value, is_checked, opts.merge({id: checkbox_id})) |
houndci-bot
May 8, 2016
Redundant curly braces around a hash parameter.
Space inside { missing.
Space inside } missing.
houndci-bot
reviewed
May 8, 2016
| @@ -323,16 +324,16 @@ def link_to_add_section(linktext, form, nested_model_name, partial_to_render, lo | ||
| new_nested_model = form.object.class.reflect_on_association(nested_model_name).klass.new | ||
| child_index = "new_#{nested_model_name}" | ||
| rendered_partial_to_add = | ||
| - form.fields_for(nested_model_name, new_nested_model, :child_index => child_index) {|child_form| | ||
| - render(:partial => partial_to_render, :locals => {:form => child_form, :index => child_index}.merge(locals)) | ||
| + form.fields_for(nested_model_name, new_nested_model, child_index: child_index) {|child_form| |
houndci-bot
reviewed
May 8, 2016
| @@ -253,9 +254,9 @@ def sort_link(title, column=nil, options = {}) | ||
| direction = options[:desc_default] ? 'DESC' : 'ASC' | ||
| end | ||
| link_to_unless condition, ((direction == 'ASC' ? '↑ ' : '↓ ') + title).html_safe, | ||
| - request.parameters.merge( {:sort_column => column, :sort_direction => direction} ), {:class => css_class, :title => (direction == 'ASC' ? ts('sort up') : ts('sort down'))} | ||
| + request.parameters.merge( {sort_column: column, sort_direction: direction} ), {class: css_class, title: (direction == 'ASC' ? ts('sort up') : ts('sort down'))} |
houndci-bot
May 8, 2016
Align the parameters of a method call if they span more than one line.
Redundant curly braces around a hash parameter.
Space inside { missing.
Space inside } missing.
Space inside parentheses detected.
houndci-bot
reviewed
May 8, 2016
| - Rails.cache.fetch("#{creation.cache_key}/byline-nonanon") do | ||
| + def non_anonymous_byline(creation, url_path = nil) | ||
| + only_path = url_path.nil? ? true : url_path | ||
| + Rails.cache.fetch("#{creation.cache_key}/byline-nonanon/#{only_path.to_s}") do |
|
"The code looks okay to me reading it over but I am definitely leery of whether the existing tests adequately cover this change. Would there be any way to crawl the live production AO3 site to find all the instances of "http://(www.)?archiveofourown" that currently exist so we can check them?" Not easily and I would point out that we don't need to rely on this code immediately we can start out with a secure.archiveofourown.org which we can examine and look at before the migration to https by default. So in short I can't reliably crawl ao3.org. |
zz9pzza
added
the
Coder has actioned review
label
May 8, 2016
All checks have passed
zz9pzza commentedMar 12, 2016
https://otwarchive.atlassian.net/browse/AO3-4516