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

FIX show name of members in summary tables #273

Merged

Conversation

NightJar
Copy link
Contributor

Anonymous comments (posted by the public at large) must have a name
and an email address associated with them. On the other hand, a logged
in user will have the Member record details used for this information,
via the Author relationship.

However the summary fields do not allow for this, and only reference
Name and Email on the Comment model directly, so any comment posted by a
logged in member has no data for name and email displayed in the various
GridFields in the CMS for administering comments (either per page, or in
the global ModelAdmin).

To recitfy this we can change the summary fields to use getter methods
that will return the Comment model info, or fall back to the Author
associated Member record if Name and Email are unset on the Comment.

Resolves #260

image

image

Anonymous comments (posted by the public at large) must have a name
and an email address associated with them. On the other hand, a logged
in user will have the `Member` record details used for this information,
via the Author relationship.

However the summary fields do not allow for this, and only reference
Name and Email on the Comment model directly, so any comment posted by a
logged in member has no data for name and email displayed in the various
GridFields in the CMS for administering comments (either per page, or in
the global ModelAdmin).

To recitfy this we can change the summary fields to use getter methods
that will return the Comment model info, or fall back to the Author
associated Member record if Name and Email are unset on the Comment.
Copy link
Contributor

@ScopeyNZ ScopeyNZ left a comment

Choose a reason for hiding this comment

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

The failing build is related to a recent change in framework: silverstripe/silverstripe-framework#8698

Frankly asserting that labels propagate correctly is a pretty low value test. Anyway I digress, the failure is not related to this PR.

@ScopeyNZ
Copy link
Contributor

I've raised a PR that should fix those failing tests: #274

@NightJar
Copy link
Contributor Author

NightJar commented Jan 11, 2019

apparently not :(
(the build on #274 failed)

@ScopeyNZ ScopeyNZ merged commit 20c6a50 into silverstripe:3.1 Jan 11, 2019
@ScopeyNZ ScopeyNZ deleted the pulls/3.1/name-that-member branch January 11, 2019 01:56
@robbieaverill
Copy link
Contributor

This PR could've used a simple unit test TBH, but not a huge problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants