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

Improve mapping history with friendly messages #157

Merged
merged 13 commits into from Jan 23, 2014
Merged

Improve mapping history with friendly messages #157

merged 13 commits into from Jan 23, 2014

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jan 23, 2014

  • Remove the tables and replace with a friendly changeset title, date and user, followed by a well containing the complete set of changes
  • Refer to http_status as we do elsewhere, ie as Archives or Redirects
Paul Hayes added 9 commits Dec 24, 2013
* Rename .erb to .html.erb
* We haven’t had versions JS for a while now
* Generate a human readable title based on the type of changes
* Use ordered lists, definition lists and bootstrap wells rather than
nested tables
* Use friendlier dates

Caveats are that the logic for displaying the changes is a little ack n
slash and could do with some love. Most of this comes as result of our
models no longer matching displayed labels.
* Create methods for friendly title, summary and field names
* Test new friendly version helper methods
* Tweak cucumber features to include some of the friendlier messages
end
end

def friendly_changeset_field(field)

This comment has been minimized.

@rgarner

rgarner Jan 23, 2014
Contributor

This is just about the field (not necessarily the changeset, although it is being used in that context). Can we say something like friendly_field_name instead?

else
field.titleize
end
end

This comment has been minimized.

@rgarner

rgarner Jan 23, 2014
Contributor

You could (if you wished 😉) make this a bit shorter with a hash:

    { 
      'http_status' => 'Type', 
      'archive_url' => 'Alternative Archive URL'
    }[field] || field.titleize
end
end

def friendly_changeset_values(field, change)

This comment has been minimized.

@rgarner

rgarner Jan 23, 2014
Contributor

This doesn't seem to be about a changeset and doesn't have multiple values (it's an old/new tuple?). It's about describing a field's old and new values. Could the name reflect that? Suggestions ... er ... this is hard. old_to_new? field_value_transition? describe_value_change?

<% @mapping.versions.reverse.each do |version| %>
<tr>
<td>
<%= anchor '[*]', version.id %>

This comment has been minimized.

@rgarner

rgarner Jan 23, 2014
Contributor

I am glad to see the back of this line in particular 🔫

if changeset['id']
"Mapping created"
elsif changeset['http_status']
if changeset['http_status'][0] == '410'

This comment has been minimized.

@rgarner

rgarner Jan 23, 2014
Contributor

This works for now because we've only two statuses, but I think I'd prefer to see ['http_status'][1] == '301' if only to avoid having to change things if and when future statuses arrive (I'm thinking of 'Content Pending')

Paul Hayes added 4 commits Jan 23, 2014
* Instead of using the old value, use the new value
* Provide an alternative if the new value isn’t a recognised one
* Use a more descriptive method name
* Switch to single quote strings
* Use ‘case field’ rather than if statements
rgarner added a commit that referenced this pull request Jan 23, 2014
Improve mapping history with friendly messages
@rgarner rgarner merged commit cb76166 into master Jan 23, 2014
@rgarner rgarner deleted the a-better-history branch Jan 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.