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

[READY] adding mileage #279

Merged
merged 3 commits into from Mar 24, 2015
Merged

[READY] adding mileage #279

merged 3 commits into from Mar 24, 2015

Conversation

mischaboldy
Copy link
Contributor

pull request for review

@jurre
Copy link
Contributor

jurre commented Feb 24, 2015

Hey, friendly reminder that we all get an email for each issue that hound finds ^^

@mischaboldy
Copy link
Contributor Author

apologies for the mails, still cant get rubocop setup correctly to check localy

@jurre
Copy link
Contributor

jurre commented Mar 4, 2015

No worries :)

@tarzan
Copy link
Member

tarzan commented Mar 4, 2015

@mischaboldy you can download the Rubocop config file that Hound uses here: https://houndci.com/configuration#ruby

@tarzan tarzan changed the title adding mileage [WIP] adding mileage Mar 5, 2015
@mischaboldy
Copy link
Contributor Author

so everything seems to be working. I still want to rework this to 1 entries table instead of the 2 i've got now and any refactoring tips are appreciated.

return Hour.find(params[:entry_id]).audits
elsif params[:entry_type] == "mileages"
return Mileage.find(params[:entry_id]).audits
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use constantize for this?
http://apidock.com/rails/Inflector/constantize

@tarzan
Copy link
Member

tarzan commented Mar 9, 2015

If you want to only have one entries table you can do STI (single table inheritance), there is a lot of documentation and guides out there.

However, one of the main reasons for wanting to do this is is because you would generally want to Q for all entries. I thought this would be rare as in our use case you'd generally either get all hours or all 'milages', right?

@mischaboldy mischaboldy force-pushed the mischa/entries branch 2 times, most recently from 6130190 to 85f81bb Compare March 9, 2015 12:53
@mischaboldy
Copy link
Contributor Author

Did some refactoring, Still not quite sure how to remove the == "string" everywhere. Suggestions welcome.


def edit_entry(new_project, new_mileages, new_date)
click_link I18n.t("navbar.entries")
click_link "edit"
Copy link
Contributor

Choose a reason for hiding this comment

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

i18n

def entry_type?
if request.fullpath == mileage_entry_path
return true
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the name describes what it does
also this could be a oneliner :)

@mischaboldy mischaboldy force-pushed the mischa/entries branch 2 times, most recently from ec73ecb to 1db2445 Compare March 10, 2015 13:49
@@ -4,22 +4,32 @@ class EntriesController < ApplicationController
DATE_FORMAT = "%d/%m/%Y".freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could put this code in two seperate controllers? One for HoursEntries and one for MilesEntries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would mean 2 controllers with very similar code. I chose not to do this to adhere to the DRY rule. It does make the coding more complicated to read and it contains a few horrible if statements. If you think two separate controllers is a better solution then i'll do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just see a lot of if/else statements which would suggest two seperate controllers would be better, you could also do it with inheritance, put methods like resource in the parent controller and create method in the child controller.

@Marthyn
Copy link
Contributor

Marthyn commented Mar 12, 2015

I think you broke something in the dutch translation file:
screen shot 2015-03-12 at 15 14 42

@Marthyn
Copy link
Contributor

Marthyn commented Mar 12, 2015

Other than that looks good

def to_object(entry_type)
entry_type.singularize.camelize.constantize
end

Copy link
Contributor

Choose a reason for hiding this comment

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

is this still necessary after you seperated the controllers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is still used by two other controllers, billables_controller and reports_controller

@mischaboldy
Copy link
Contributor Author

Anyone else with comments or suggestions?

@mischaboldy mischaboldy force-pushed the mischa/entries branch 7 times, most recently from 476c975 to cbe896c Compare March 16, 2015 16:01
@mischaboldy mischaboldy changed the title [WIP] adding mileage [READY] adding mileage Mar 19, 2015
@mischaboldy
Copy link
Contributor Author

Did you still want to discuss whether or not to actually implement the mileages this way? @dkhgh @tarzan

$(this).addClass('is-active');
} else {
event.preventDefault();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could change the if and else codeblock, and drop the !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix

@tarzan tarzan force-pushed the mischa/entries branch 3 times, most recently from 83f5ad4 to df5f660 Compare March 23, 2015 17:16
@@ -35,4 +26,20 @@ def options

CSV::DEFAULT_OPTIONS
end

def get_fields(entry, entry_type)

Choose a reason for hiding this comment

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

Assignment Branch Condition size for get_fields is too high. [15.17/15]

@tarzan
Copy link
Member

tarzan commented Mar 24, 2015

merging this to test on staging

tarzan added a commit that referenced this pull request Mar 24, 2015
@tarzan tarzan merged commit b81ec48 into development Mar 24, 2015
@tarzan tarzan deleted the mischa/entries branch March 24, 2015 12:52
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

6 participants