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

Fixed Bug #5593. #5614

Merged
merged 1 commit into from Feb 2, 2015

Conversation

@sam09
Copy link
Contributor

commented Feb 1, 2015

Added length validation to location in profile model with maximum length 255. The server does not return an error.

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 1, 2015

Thanks! Can you add a test exposing the bug?

@sam09

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2015

I added a test.
Should I open a separate pull request?

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 1, 2015

No, pushing to the branch you have an pull request open for, updates it ;)

@sam09

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2015

Ok. Thanks for the help. I am new to the open source community. Hence the confusion.

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 1, 2015

No worries, you're welcome!

@sam09

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2015

I can't understand what happened. Why did Travis tests fail

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 2, 2015

Don't worry, we still have a few randomly failing tests unfortunately.

@sam09

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2015

Does that mean that the tests I wrote is fine?

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 2, 2015

Yes :)

it "cannot be 256 characters" do
profile = FactoryGirl.build(:profile, :location => "a"*256)
expect(profile).not_to be_valid
end

This comment has been minimized.

Copy link
@jhass

jhass Feb 2, 2015

Member

Please indent the end onto the level of the it ;)

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 2, 2015

Okay, I think it's fair to make this a single commit. Also you committed to develop, which you shouldn't do in the future in case you want to do multiple contributions concurrently ;) Let's fix that up.

git remote add upstream git://github.com/diaspora/diaspora.git
git fetch upstream
git checkout develop
git rebase -i upstream/develop
# replace pick with fix in all except the first line, save & quit
git push -f origin develop
# You can edit the last commit of the current branch with git commit --amend, after which you have to force-push with git push -f
# _After_ this PR is merged, do:
git branch -m develop location_length
git fetch upstream
git checkout -b upstream/develop
git push -f origin develop
# Start your next contributions with something like
git checkout -b my_contribution upstream/develop
@sam09

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2015

should i run all of these commands?

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 2, 2015

You should read and maybe try to understand what they do ;) You should especially read the lines that start with #, which are comments, some of these will tell you when to run those commands.

@sam09

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2015

I think i got it. Thanks for the help

@jhass jhass merged commit a12c0d8 into diaspora:develop Feb 2, 2015

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

jhass added a commit that referenced this pull request Feb 2, 2015

@jhass jhass added this to the next-major milestone Feb 2, 2015

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 2, 2015

Thank you!

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Feb 2, 2015

Thanks!

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Feb 2, 2015

Btw it's nice to see that it doesn't crash anymore, but a message informing the user that the content submitted has been truncated would have be better ;)

@sam09

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2015

I would work on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.