issue_4211 Record ip addresses on new works #1980
Conversation
| @@ -246,6 +246,7 @@ def new | |||
| @series = current_user.series.uniq | |||
| @unposted = current_user.unposted_work | |||
|
|
|||
| @work.ip_address= request.remote_ip | |||
There was a problem hiding this comment.
You know what I'm going to say... spaces on either sign of operators, please. @work.ip_address = request.remote_ip, in both places.
| <dd class="stats"> | ||
| <% end %><!-- end of cache --> | ||
|
|
||
| <%= work_meta_list(@work, @chapter) %> | ||
| <% if logged_in_as_admin? %> |
There was a problem hiding this comment.
It looks like there are two <dt> elements here -- the one on line 68 that contains the actual IP address data should be changed to a <dd> (the closing tag on line 74 is correct). Also, <dt> and <dd> elements can only ever have the <dl> element as the direct parent, but right now, their parent is a <dd> element. They'll need to be moved outside the stats <dd> so their parent element becomes the <dl> that surrounds the entire meta section.
For class names, we try to use single words separated by spaces rather than smashing words together. <dt class="ip"> and <dd class="ip"> would be sufficient here.
Lastly, there are some indenting issues -- so once you put all the changes in, it should be like this (starting on line 63):
<%= work_meta_list(@work, @chapter) %>
</dd>
<% if logged_in_as_admin? %>
<dt class="ip">
<%= ts('IP Address:') %>
</dt>
<dd class="ip">
<% if @work.ip_address.nil? %>
<%= ts('No address recorded') %>
<% else %>
<%= @work.ip_address %>
<% end %>
</dd>
<% end %>
| <dt class="ip"> | ||
| <%= ts('IP Address:') %> | ||
| </dt> | ||
| <dd class="ip"> |
There was a problem hiding this comment.
I took a look because you mentioned CSS problems, and it looks okay to me:
Which browser are you having a problem with, or what do you want it to look like?
Also, the indenting here is still a bit wrong. The if/else/end is inside the dd, so everything belonging to it needs to move over two more spaces:
<dd class="ip">
<% if @work.ip_address.nil? %>
<%= ts('No address recorded') %>
<% else %>
<%= @work.ip_address %>
<% end %>
</dd>
There was a problem hiding this comment.
I will do as you ask, not convinced it feels right though :) as the it then are not seen in the page that is outputted.
| remove_index "works", "ip_address" | ||
| remove_column :works, :ip_address | ||
| end | ||
|
|
There was a problem hiding this comment.
Extra empty line detected at body end.
| def self.down | ||
| remove_index "works", "ip_address" | ||
| remove_column :works, :ip_address | ||
| end |
There was a problem hiding this comment.
end at 10, 3 is not aligned with def at 7, 2
zz9pzza
changed the title
issue_4211 Record ip addresses on new works
This is lacking css, and I want to see if it passes our tests however I think it looks reasonable.
https://code.google.com/p/otwarchive/issues/detail?id=4211