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

Untitled #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Untitled #6

wants to merge 3 commits into from

Conversation

andreacampi
Copy link

fruitfly:redmine-master andrea$ ruby test/functional/versions_controller_test.rb
Loaded suite test/functional/versions_controller_test
Started
............
Finished in 2.229972 seconds.

12 tests, 40 assertions, 0 failures, 0 errors

@andreacampi
Copy link
Author

Coverage in this area was incomplete (commenting out some of the code I refactored would still let tests pass), so I added some more.

Incidentally: allowed sharing is only validated in the controller.
In general, I'd rather have that in the model; however, I can see corner cases where that may be bad / hard to implement.
For example, say an admin sets sharing to 'system', and then a manager changes that version--if we validated on the model only, that would fail.
What's your take on this?

Something else I mentioned: in this file there is a mix of:

assert_redirected_to '/projects/ecookbook/settings/versions'

vs

assert_redirected_to :controller => 'projects', :action => 'settings', :tab => 'versions', :id => 'ecookbook'

Want me to address that in a follow-up commit? My preferred form would be the first one, as it's a stronger assertion.

@edavis10
Copy link
Owner

Just wanted to let you know that I haven't forgotten about this, it's in my email waiting to be reviewed. Feel free to submit another pull request for changing the assert_redirected_to to the hash style. I prefer hashs in functional tests and strings in integration tests.

Thanks

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