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

Adds invitations_counter cache for triggering the number of invitations ... #425

Merged
merged 1 commit into from Jan 30, 2014

Conversation

kurenn
Copy link

@kurenn kurenn commented Jan 29, 2014

This seems useful for optimization purposes, I'm using it on a project of mine! works pretty good.

Great gem by the way!

scambra added a commit that referenced this pull request Jan 30, 2014
Adds invitations_counter cache for triggering the number of invitations ...
@scambra scambra merged commit f0b3a83 into scambra:master Jan 30, 2014
@bradleypriest
Copy link
Contributor

@scambra how do you feel about adding a flag for this?
I don't use or need invitation limits but now I have to add an extra unnecessary field to the User model

@scambra
Copy link
Owner

scambra commented Feb 10, 2014

@kurenn, what rails version are you using? are you using polymorphic invite_by or did you set invited_by_class_name?

scambra added a commit that referenced this pull request Feb 10, 2014
…ss_name is set and invitations_count column is present, related to #425
@kurenn
Copy link
Author

kurenn commented Feb 10, 2014

I used the polymorphic one.

On 10 February 2014 04:48, Sergio Cambra notifications@github.com wrote:

@kurenn https://github.com/kurenn, what rails version are you using?
are you using polymorphic invite_by or did you set invited_by_class_name?

Reply to this email directly or view it on GitHubhttps://github.com//pull/425#issuecomment-34618448
.

Abraham Kuri Vargas
Cel - 8117902523
abraham.kuri@gmail.com
http://kurenn.net

@@ -7,6 +7,8 @@ def up
t.datetime :invitation_accepted_at
t.integer :invitation_limit
t.references :invited_by, :polymorphic => true
t.integer :invitations_count, default: 0
t.index :invitations_count
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need an index on invitations_count? Do you query users somewhere based only on invitation counts? And even if you do, the index doesn't seem selective enough.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I queried users based on that and displaying it on a view. I made it as a preventive matter if I recall correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt if the majority of people use it for that purpose (some kind of leaderboard), and it would lie as an unused index in their table. I personally think that this index does not belong to the gem migration but should be added separately (in their own migration) if someone needs that kind of functionality.

Copy link
Author

Choose a reason for hiding this comment

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

You may be right, and probably is a better idea to have the index on a separate migration file, in case someone needs it

Copy link
Contributor

Choose a reason for hiding this comment

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

Created a PR: #830

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

4 participants