Request to code review 2 PR's and determine if they're following Rails best practices
New here? Learn about Bountify and follow @bountify to get notified of new bounties! x

Hi, I have 2 PR's and was wondering if someone could code review these 2 PR's and help determine if we're using the best rails patterns to implement these features.

I'm not an advanced Rails developer, so a code review from a rails developer would be great! Here are the 2 PR's
https://github.com/akshatpradhan/compliance_chimp/pull/20
https://github.com/akshatpradhan/compliance_chimp/pull/25/files

awarded to sguha

Crowdsource coding tasks.

1 Solution

Winning solution
Argh, this isn't what I'm looking for actually. What I'm really concerned about is the use of nested resources that can only be nested twice, vs using a hidden_field for user_id. The use of a hidden_field for user_id seems very messy but deep nesting resources isn't an option, so I'm at a loss of trying to figure out a better pattern. The other thing I'm concerned about is how messy the view helpers are looking. @kc00l suggested using decorators with draper but I guess I'm just wondering what people did to make their view helpers look good before the advent of decorators which seems to have come in 2013. Moreover, are decorators still a good pattern to use considering people are putting angular and ember into their Rails apps? The Draper gem hasn't been updated in 4 months.
akshatpradhan over 5 years ago
Ah, I see. I'll take a look at it again later on tonight
sguha over 5 years ago
I updated PR#28 with a fix for the setting the user_id, it's more secure to assign user in the controller so that it can't be spoofed
sguha over 5 years ago
a more modern approach (in the optic of a Rails4 upgrade especially the use of strongparameters) would be merging `userid: current_user.id` to the proof parameters.
kc00l over 5 years ago
@Sguha I asked kc00l this question: Does sguha's @proofs.user = current_user pattern help with cleaning out the some of the current_user stuff that's in the RequirementsHelper? kc00l’s reply was, “No and its because sguha's changes are in proofs_controller#create action and users#show view doesn't know anything about that. You know what I'm thinking about? Things are becoming hard to understand for you because we are showing stuff about requirements and proofs in users#show view. Why don't we move all this to the requirements#index view?” I thought you would find it informative or maybe you would have another opinion for me to figure out!
akshatpradhan over 5 years ago
@akshatpradhan, ya my change for current_user doesn't really address the RequirementsHelper. I agree with kc00l, I think making an index action for requirements will help clean things up, rather than showing everything under users#show
sguha over 5 years ago
@sguha Would you be able to send me an email real quick? I can't seem to find it when you sent it to me last time. My email address is akshat.ip@gmail.com
akshatpradhan over 5 years ago