-
Notifications
You must be signed in to change notification settings - Fork 198
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
Adding mongoid support #123
Conversation
@@orm = :mongoid | ||
else | ||
@@orm = :active_record | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this if
construct. @@orm
is by default active_record
, and can be overridden in the initializer.
When this looks good to both of us I'll merge. I'll do it "silently" (without broadcasting in the README) cause I don't work with (and thus can't reliably maintain) other ORMs for now. |
Also moved common methods between AR and mongoid to a base class
OK @tute check it out, updated to your needs. When you decide to merge this over, I think it would be best to broadcast it in the readme. There are Mongoid developers like myself who can keep the code updated, and if it was broadcast in the readme more mongoid developers would use it leading to further development (and bug feedback etc). Let me know if you decide to go through with this and i'll update the readme for directions on how to use the gem with mongoid. |
Looking way better! @potomak and @huacnlee will probably be interested in this PR/discussion. Aside, would be nice to add back mongoid testing, in the form of the following block to
|
@@ -45,13 +45,17 @@ class Engine < Rails::Engine | |||
config.app_generators.orm Merit.orm | |||
|
|||
initializer 'merit.controller' do |app| | |||
require 'merit/models/base/base/sash' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why base/base/
? Should be merit/base/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, ill fix this
Check it out now @tute I have addressed everything you mentioned with the exception of fixing up the dummy app. Let me know your thoughts, thanks |
require "merit/models/#{Merit.orm}/merit/activity_log" | ||
require "merit/models/#{Merit.orm}/merit/badges_sash" | ||
require "merit/models/#{Merit.orm}/merit/sash" | ||
require "merit/models/#{Merit.orm}/merit/score" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Looking great, we are very close! Dummy app should be fixed, I can do it after merge I guess. Can you please rebase on top of current master, so it's mergeable again? Left a few comments, as always, thank you very much for your work! |
|
||
belongs_to :action, class_name: 'Merit::Action' | ||
belongs_to :related_change, polymorphic: true | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny comments (ok to ignore): stray newline.
Take a look @tute, let me know what you think and we can look at rebasing if you think that it's ready |
require "merit/models/#{Merit.orm}/merit/activity_log" | ||
require "merit/models/#{Merit.orm}/merit/badges_sash" | ||
require "merit/models/#{Merit.orm}/merit/sash" | ||
require "merit/models/#{Merit.orm}/merit/score" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
include Mongoid::Timestamps | ||
include Base::Sash | ||
|
||
belongs_to :user, class_name: 'User' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Sash
belongs to a "meritable model", which happens to be User many times, but it's not always the case. We should be able to attach sashes to any model in the app, this won't pass the "dummy app" tests, where comments also hold reputation.
Looking really good! Should we pair to wrap it up? Specially the test app. We should decide how to attach sashes to arbitrary models in Mongoid first. |
Added observer to Merit.
Adds test assertions.
Sorry @tute. Yes lets wrap this up. We could always make a one sided relationship, so |
It seems AR doesn't have the |
Also moved common methods between AR and mongoid to a base class
Ok, have a look @tute, I've rebased on master, I think it should be ready to pull |
Thanks @mrjlynch. Though you may have rebased with master on your repository, you need to fetch from |
There's actually duplicate commits, and a merge. I'll do it myself, and fix the tests. Thanks a lot! |
Merged from the command line, thanks for your help! :) |
Great, Thanks Tute! |
Can we also update the readme? |
Can't make it work @mrjlynch, and got late. If you want to fix tests in current master ( For now, code seems closer to work with mongoid, but won't update the README until it works well. Thanks for the work! |
any progress ? |
@tagrudev I am no longer using this gem with mongoid, everything should work though, the specs just don't pass. Feel free to update them. |
@mrjlynch @tute I can't seem to get Merit to install on a Mongoid setup. rails g merit:install gives I went into /lib/merit.rb, commented out The install went through this time as far as copying over the files, but it gave an However, I went into the rails console and created a Badge which went through fine, and then added that Badge to my user which also seems to have succeeded:
I can do Any suggestions? UPDATE: So it seems that the badge is removed, but that change isn't reflected immediately in I apologize for my ignorance, but is this expected behavior? |
You can also do Though to be 100% sure that it works in mongoid, a Pull Request that reenables CI and gets it green would be needed. See: https://github.com/tute/merit/blob/master/.travis.yml#L14-L16. |
I still can't get merit to work with only mongoid. Here are the issues I run into:
|
I have reintroduced mongoid support. It is working
with the exception of the.top_scored
methodI have tested this completely with mongoid 3.1.5 and rails 3.2.15
I could not get the mongoid-dummy app to run for some reason, I might look into this later.
Todo: update generators to work with mongoid projects.