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

Add configuration option for auditor method name attribute #44

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

olivier-thatch
Copy link
Contributor

@olivier-thatch olivier-thatch commented Jun 8, 2023

Adds a new configuration option auditor_name_attr that accepts a Symbol containing the name of the attribute on the auditor class that returns the auditor's name to display in the UI.

Use case: our User record does not have a name field. We could define a name method on our User model class, but having to define a custom method on a class in our app for the sake of a third-party gem is a bit annoying. Instead, with this change, we can do:

config.audits1984.auditor_name_attr = :email

I've tested this change in our app and verified that it works as expected.

cc @jorgemanrubia

@jorgemanrubia
Copy link
Member

Hi @olivier-thatch,

Thanks for the PR. While it brings flexibility, I find the proc to extract the name a bit off. What about an option to configure the auditor_name_attribute that defaults to current name. Instead of reading #name the could would do public_send(Audits1984.auditor_name_attribute)

@olivier-thatch olivier-thatch changed the title Add configuration option for auditor method name proc Add configuration option for auditor method name attribute Jun 15, 2023
@olivier-thatch
Copy link
Contributor Author

@jorgemanrubia Updated the PR as requested. (Your suggestion was actually my first approach for this PR :) I thought some folks might appreciate the extra flexibility, but I agree it's probably not needed.)

@jorgemanrubia
Copy link
Member

Thanks @olivier-thatch. Could you rename to auditor_name_attribute please? Very subjective, but I'm not a big fan of abbreviations when naming things 😅. I'll merge with that in place 🙏.

@olivier-thatch
Copy link
Contributor Author

Done :)

@jorgemanrubia jorgemanrubia merged commit b2a5c3d into basecamp:master Jun 15, 2023
@jorgemanrubia
Copy link
Member

Thank you!

@olivier-thatch olivier-thatch deleted the auditor-name-method branch June 15, 2023 20:28
@olivier-thatch
Copy link
Contributor Author

You're welcome!

No rush, but if you could release new versions of console1984 and audits1984 with the changes I submitted, it would be greatly appreciated too :)

@jorgemanrubia
Copy link
Member

Hey I already did after merging the PRs! Latest versions should include the last changes.

@olivier-thatch
Copy link
Contributor Author

🚀 Oops! Sorry I didn't check before asking 🤦 Thanks!

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

Successfully merging this pull request may close these issues.

2 participants