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

Corrected pagination for charges #241

Closed
wants to merge 33 commits into from
Closed

Conversation

brucethesloth
Copy link

According to #214 , there were problems with pagination on charges. @AkshayGoradia and I worked on this so that we cased on the users' role and show them the appropriate charges that they are supposed to see. @pkoenig10 Could you let us know if this is the desired behavior. Thanks!

Bruce Lin added 30 commits February 26, 2017 14:46
… merge rumby's modification on factories into my test branch
This will update the factory file in my branch so that I do not overrite existing factories by accident
…class for orgniazation_status but have been getting error message: factory not registered. Will come back to this later
Add in all the factories so that we don't get messed up from this point on

More convinient to review other people's code
…hange before checking out akshay's unit tests
…th the built in timezone indicated in application.rb
Merging goat branch to get the most recent update by my teammates, in case we have to edit common files again.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 15.357% when pulling 24cd432 on bl/bugfix into bbb4739 on 17/goat/main.

@pkoenig10
Copy link
Member

@blinblinblin can you clean up your PRs so only the relevant commits are in the PR? Its really hard to review when there are 30+ commits many of which have nothing to do with the PR and have already been merged. It also can easily lead to regressions when old code is accidentally merged.

The easiest way to do this is to create a separate branch for each PR which you branch off the latest commit on 17/goat/main before you begin work on that PR.

@pkoenig10
Copy link
Member

@blinblinblin It looks like as of CanCan 1.4 that this filtering is done automatically for index methods. I would much prefer letting CanCan do all the filtering when returning objects. As you can see, doing it manually like this is really prone to breaking and requires whoever is writing index methods to know all the correct permissions for objects. If we put everything in abilities.rb we have a nice declarative way of specifying permissions all in once place.

Could you try upgrading CanCan to the latest version and see if it automatically filters charges? If that works, we can update other models to switch to CanCan in a future PR.

@brucethesloth
Copy link
Author

@pkoenig10 I will close this PR and restart the bug fix in another branch. We will try to keep our submits clean from now on so that the commits look clean. Will look into cancan for this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants