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

ActiveAdmin: Allow for both singular and plural model names #776

Merged
merged 1 commit into from
May 9, 2020

Conversation

vfonic
Copy link
Contributor

@vfonic vfonic commented Mar 12, 2020

This is an addition to this PR: #692
I've seen it's been stale for a while and it's rather simple fix.

I noticed my ActiveAdmin resources are not being annotated.
I know ActiveAdmin uses plural model names, but, as far as I remember, they used to use singular model names in the past.

We can see some "evidence" of that here: https://activeadmin.info/2-resource-customization.html#customizing-parent-menu-items
Probably in other places as well.

In order to keep the gem working as expected for singular ActiveAdmin resources, I suggest we annotate both singular and plural model names.

I just tested this out in a project of mine where I renamed one file to be in singular form and left the other files in plural form.
It worked like a charm. :)

I also added a test, as best to my knowledge, as there are no similar tests that I could find.

Please let me know what you think!

/cc @danielricecodes @drwl

Also please, don't make the default rake task do a git checkout! I've never seen this and I've lost the above changes once due to this! I just didn't expect that running tests would be changing the source files in any way.

Copy link
Collaborator

@drwl drwl left a comment

Choose a reason for hiding this comment

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

Had a question

let(:pattern_type) { 'admin' }
let(:options) { {} }

it 'returns an empty array' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this expectation description accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right. I've updated the description. Thanks!

@vfonic vfonic force-pushed the activeadmin-plural-model-names branch from ccffeb7 to b5029b6 Compare April 6, 2020 10:16
@vfonic
Copy link
Contributor Author

vfonic commented Apr 6, 2020

I've updated the spec description, rebased on develop and force-pushed the amended commit.

@vfonic vfonic force-pushed the activeadmin-plural-model-names branch from b5029b6 to 7caf867 Compare April 7, 2020 08:12
@vfonic
Copy link
Contributor Author

vfonic commented Apr 7, 2020

I've resolved merge conflicts with the latest changes to develop again.

@drwl can you please have a look? Thanks!

Copy link
Collaborator

@drwl drwl left a comment

Choose a reason for hiding this comment

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

Looks like it's good to go except rubocop is complaining about the single vs double quotes. After that, this looks like it should be good to go 🙂.

@vfonic vfonic force-pushed the activeadmin-plural-model-names branch from 7caf867 to bbde2a7 Compare May 8, 2020 06:43
@vfonic vfonic force-pushed the activeadmin-plural-model-names branch from bbde2a7 to c1ad378 Compare May 8, 2020 06:45
@vfonic
Copy link
Contributor Author

vfonic commented May 8, 2020

@drwl I fixed the rubocop issues, ran rubocop locally to confirm, rebased over develop, force-pushed to my feature branch and now everything should be up to date. :)

Copy link
Collaborator

@drwl drwl left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! 👍

@drwl drwl merged commit b3aa361 into ctran:develop May 9, 2020
@drwl
Copy link
Collaborator

drwl commented May 9, 2020

Also please, don't make the default rake task do a git checkout! I've never seen this and I've lost the above changes once due to this! I just didn't expect that running tests would be changing the source files in any way.

@vfonic to clarify, did you mean that running the tests would clear your branch? If so, I apologize for that. It was a hacky approach to getting integration tests working. I've since made it opt-in via env variable. Once this project stabilizes a bit more I'll work on making it non-destructive.

@vfonic
Copy link
Contributor Author

vfonic commented May 11, 2020

@drwl Yes, that's what happened. Thanks for clarifying this! I'm glad to hear this wasn't a permanent solution. :)

ocarta-l pushed a commit to ocarta-l/annotate_models that referenced this pull request Jun 18, 2021
This is an addition to this PR: ctran#692
I've seen it's been stale for a while and it's rather simple fix.

I noticed my ActiveAdmin resources are not being annotated.
I know ActiveAdmin uses plural model names, but, as far as I remember, they used to use singular model names in the past.

We can see some "evidence" of that here: https://activeadmin.info/2-resource-customization.html#customizing-parent-menu-items
Probably in other places as well.

In order to keep the gem working as expected for singular ActiveAdmin resources, I suggest we annotate both singular and plural model names.

I just tested this out in a project of mine where I renamed one file to be in singular form and left the other files in plural form.
It worked like a charm. :)
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