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 ownership to channels and associated enhancements #223

Merged
merged 11 commits into from Mar 19, 2013

Conversation

mjtko
Copy link
Contributor

@mjtko mjtko commented Mar 16, 2013

This is a bit of a big one, so I want to get it open for discussion.

This PR fixes both #131 and #154 by introducing Cancan to manage authorization. Channels are now owned by a user. A user may only delete a channel they own, unless they are an admin. Nobody may delete channel id=1.

As always, feedback/questions welcomed! 😺

@coveralls
Copy link

Coverage increased (+1.92%) when pulling c21ace8 on mjtko:kandan-131 into ff6530a on kandanapp:master.

View Details

@gabceb
Copy link
Member

gabceb commented Mar 16, 2013

I would appreciate some comments especially on that last commit 👮

@gabceb
Copy link
Member

gabceb commented Mar 16, 2013

Overall this looks good. I have to pull it down and test it. 👍 for using CanCan

@fusion94
Copy link
Member

This is pretty much how I envisioned it working. It's a good first start but over time will need improvements. Ie channels will eventually need to have the concept of private vs public etc.

Thoughts?

@fusion94
Copy link
Member

Also to note is that I won't be able to test until tomorrow

@ghost ghost assigned donthorp Mar 16, 2013
@mjtko
Copy link
Contributor Author

mjtko commented Mar 16, 2013

Latest commits:

  1. I've tried to make the FFBT a bit less flaky by increasing the timeout when looking for .chat-input.
  2. @gabceb: I've refactored kandan.js.coffee a little and added some comments so hopefully it's slightly more obvious what's going on now.

@fusion94: Yup, this is definitely just a starting point and will need improvements - was initially focussing on fixing #131 before considering branching out into more fine-grained control of channels (which will need UI improvements for modifying, creating etc.).

@coveralls
Copy link

Coverage increased (+1.92%) when pulling 4338246 on mjtko:kandan-131 into ff6530a on kandanapp:master.

View Details

@fusion94
Copy link
Member

Ok so I installed this locally and ran into issues.
When I run bundle exec rake db:create db:migrate kandan:bootstrap I get the following errors:

rake aborted!
undefined method `attributes' for nil:NilClass

Here's a gist of the full output: https://gist.github.com/fusion94/5195247

Once I start up the server I see this almost immediately:

Started GET "/channels/undefined/attachments" for 127.0.0.1 at 2013-03-19 03:56:50 -0700
Processing by AttachmentsController#index as JSON
  Parameters: {"channel_id"=>"undefined"}
  User Load (0.1ms)  SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1
  Channel Load (0.1ms)  SELECT "channels".* FROM "channels" WHERE "channels"."id" = ? LIMIT 1  [["id", "undefined"]]
Completed 404 Not Found in 2ms

ActiveRecord::RecordNotFound - Couldn't find Channel with id=undefined:

Here's a gist of the full output: https://gist.github.com/fusion94/5195255

Now once I logon I get this:

Screenshot_3_19_13_3_53_AM

So has something changed? Is your PR missing something?

@mjtko
Copy link
Contributor Author

mjtko commented Mar 19, 2013

This is probably because I didn't update the kandan:bootstrap rake task. My working theory is that no channel is ever being created, probably due to it not being allocated a user. I'll prod it and see.

@fusion94
Copy link
Member

@mjtko right on, just let us know what you find out.

…alidate correctly; ensure the primary channel is assigned to the primary user.
@mjtko
Copy link
Contributor Author

mjtko commented Mar 19, 2013

Ok, that should fix it I reckon.

@coveralls
Copy link

Coverage increased (+1.85%) when pulling 83de39a on mjtko:kandan-131 into ff6530a on kandanapp:master.

View Details

fusion94 added a commit that referenced this pull request Mar 19, 2013
Add ownership to channels and associated enhancements
@fusion94 fusion94 merged commit 646bc8f into kandanapp:master Mar 19, 2013
@mjtko
Copy link
Contributor Author

mjtko commented Mar 19, 2013

Hmm, no idea wtf travis is on about this time. Is our/their ruby 2.0 test environment borked?

@fusion94
Copy link
Member

I think their's might be as it works for me in ruby 2.0

ar7em pushed a commit to ar7em/kandan that referenced this pull request Jun 9, 2013
Add ownership to channels and associated enhancements
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.

None yet

5 participants