Skip to content

Issue #29200 scaffold an object with a reference displays an object memory address to user #29204

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 16, 2018
Merged

Issue #29200 scaffold an object with a reference displays an object memory address to user #29204

merged 1 commit into from
Oct 16, 2018

Conversation

RasPat1
Copy link
Contributor

@RasPat1 RasPat1 commented May 24, 2017

Summary

Fix for Issue #29200

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@RasPat1 RasPat1 force-pushed the issue-29200-scaffold-reference-display-memory-address branch 2 times, most recently from 72c86e8 to 4d70cb1 Compare May 24, 2017 17:32
@RasPat1 RasPat1 force-pushed the issue-29200-scaffold-reference-display-memory-address branch 3 times, most recently from 0ab54ec to 1e2d694 Compare May 31, 2017 19:42
@RasPat1 RasPat1 changed the title Issue #29200 scaffold reference display memory address Issue #29200 scaffold an object with a reference displays an object memory address to user May 31, 2017
@RasPat1 RasPat1 force-pushed the issue-29200-scaffold-reference-display-memory-address branch 3 times, most recently from c631146 to 352566f Compare May 31, 2017 21:55
@RasPat1
Copy link
Contributor Author

RasPat1 commented Jun 1, 2017 •
edited
Loading

@pixeltrix Mergeable?

@RasPat1
Copy link
Contributor Author

RasPat1 commented Aug 29, 2017

@maclover7 @pixeltrix any thoughts or todo on this?

gmcgibbon
gmcgibbon approved these changes Jun 3, 2018
View reviewed changes
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

Still relevant in Rails 6.0.0.alpha 👍 Also, when you rebase, template files have been renamed to railties/lib/rails/generators/erb/scaffold/templates/{index, show}.html.erb.tt.

railties/lib/rails/generators/erb/scaffold/templates/index.html.erb Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think its safe to do &.id instead of .try(:id) here. Same with the show template.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking a little further on this, we could also try to guess a display name property similar to ActiveAdmin but that might result in some overhead.

@RasPat1 RasPat1 force-pushed the issue-29200-scaffold-reference-display-memory-address branch 2 times, most recently from 420715c to 9386164 Compare June 3, 2018 21:42
@RasPat1
Copy link
Contributor Author

RasPat1 commented Jun 3, 2018

Syntax updated. @gmcgibbon good to go?

gmcgibbon
gmcgibbon approved these changes Jun 3, 2018
View reviewed changes
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

eugeneius
eugeneius reviewed Jun 3, 2018
View reviewed changes
railties/lib/rails/generators/erb/scaffold/templates/index.html.erb.tt Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This will generate something like the following:

<td><%= comment.post&.id %></td>

We don't need to load the associated record to get its ID, we can use the value from the column directly:

<td><%= comment.post_id %></td>

The column_name method appears to exist for this purpose:

rails/railties/lib/rails/generators/generated_attribute.rb

Lines 119 to 121 in f7fd680

def column_name
@column_name ||= reference? ? "#{name}_id" : name
end

Copy link
Member

Choose a reason for hiding this comment

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

Good point! I guess you can use attribute.column_name instead of attribute.name then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect. Much cleaner. Updated, rebased, and pushed.

@RasPat1 RasPat1 force-pushed the issue-29200-scaffold-reference-display-memory-address branch from 9386164 to b54a25b Compare June 4, 2018 16:53
@dillonwelch
Copy link
Contributor

@RasPat1 can you clean up the merge conflict on the changelog so we can see about flagging down a maintainer to get this finally merged?

@gmcgibbon
Copy link
Member

Also, if you could squash your commits while you're at it that would be 💯

@RasPat1
Copy link
Contributor Author

RasPat1 commented Oct 13, 2018

Yup, np.

@RasPat1 RasPat1 force-pushed the issue-29200-scaffold-reference-display-memory-address branch from b54a25b to 9fa0e89 Compare October 13, 2018 20:49
y-yagi
y-yagi reviewed Oct 14, 2018
View reviewed changes
railties/CHANGELOG.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Please ✂️ unnecessary i at the beginning of the line.

@RasPat1
Show object ids in scaffold pages when displaying referenced objects
ecb2850 
Resolve Issue#29200

When scaffolding a model that references another model the
generated show and index html pages display the object directly
on the page. Basically, it just shows a memory address. That is
not very helpful. In this commit we show the object's id rather
than the memory address.

This updates the scaffold templates and the json builder files.
@RasPat1 RasPat1 force-pushed the issue-29200-scaffold-reference-display-memory-address branch from 9fa0e89 to ecb2850 Compare October 14, 2018 20:04
@kamipo kamipo merged commit ecb2850 into rails:master Oct 16, 2018
kamipo added a commit that referenced this pull request Oct 16, 2018
@kamipo
Merge pull request #29204 from RasPat1/issue-29200-scaffold-reference…
b0b0cd1 
…-display-memory-address

Issue #29200 scaffold an object with a reference displays an object memory address to user
suketa added a commit to suketa/rails_sandbox that referenced this pull request Jul 6, 2019
@suketa
try050
909094d 
Issue #29200 scaffold an object with a reference displays an object memory address to user
rails/rails#29204
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eugeneius eugeneius eugeneius left review comments

@gmcgibbon gmcgibbon gmcgibbon approved these changes

+1 more reviewer

@y-yagi y-yagi y-yagi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@pixeltrix pixeltrix

Labels

railties

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@RasPat1 @rails-bot @dillonwelch @gmcgibbon @eugeneius @y-yagi @pixeltrix @kamipo @maclover7