FirehoseCommunity / DEFCON

6 stars 4 forks source link

Amp/admins promote users issue16: UGLY but functional #64

Closed amarkpark closed 8 years ago

amarkpark commented 8 years ago

Having a hard time making the pull-right thing work using HAML... maybe we need a dedicated issue to figure this out...? At any rate, using the new Admin Menu on the navbar and an index page and an edit page with an update action Admin users can now promote non-admin users to admins. I also added an Admin tag to the User dashboard for users who are admins.

amarkpark commented 8 years ago

DON'T MERGE ME YET! I've almost got the pull-right thing fixed!

kenmazaika commented 8 years ago

Code looks good. One piece of feedback is the code that you have testing the admin functionality, we might want to put in an admin_controller_test.rb. It's a little bit weird, but here's how you can test these abstract controller methods.

https://gist.github.com/kenmazaika/7a3221bd7c9536c787a7

I can go over this next week if the code itself is super confusing. Code seems good to merge by me.

amarkpark commented 8 years ago

I have the navbar issues cleared up, just need to re-integrate the current master branch and resolve merge conflicts.

amarkpark commented 8 years ago

Resolved merge conflict(s). Ran Rake before pushing, tests ran successfully but there is a very odd deprecation warning:

# Running tests:

......................DEPRECATION WARNING: input method now accepts a `wrapper_options` argument. The method definition without the argument is deprecated and will be removed in the next Simple Form version. Change your code from:

    def input

to

    def input(wrapper_options)

See https://github.com/plataformatec/simple_form/pull/997 for more information.
 (called from block in _app_views_posts_index_html_haml___647320704__579095908 at /vagrant/src/defcon/app/views/posts/index.html.haml:20)
........................

Finished tests in 8.753835s, 5.2548 tests/s, 10.5097 assertions/s.
amarkpark commented 8 years ago

@kenmazaika THANKS for that, I was trying to figure out how to test the admin_controller distinctly but was puzzled by it so I tested it indirectly through the child controller.

Code is ready to merge if you want to fix the navbar and add the functionality while I figure out that last test per the gist you linked.

kenmazaika commented 8 years ago

The deprecation warning seems to be a different issue. I'll open a ticket for it.