Skip to content

[WIP] Join team error #391 #229

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

Merged
merged 3 commits into from
Oct 24, 2014
Merged

[WIP] Join team error #391 #229

merged 3 commits into from
Oct 24, 2014

Conversation

YaroSpace
Copy link
Contributor

This PR is waiting for feature specs to be added

Fix for Error when attempting to join a team instead of create a team #391

The fix was a one liner - incorrect format of params, which caused params.permit to raise a 400 error.

I covered the related logic with specs and also did a little refactoring to make it easier to test and to understand the logic.

Changes:

  • extracted #with_similar_names to Team model method
  • added shared notification bar that can be used to display flash error/notice from within js responses.
  • added notification on success/failure of team creation

screen

@seuros
Copy link
Contributor

seuros commented Oct 20, 2014

Thank you @YaroSpace , could you please squash the commits ?

@YaroSpace
Copy link
Contributor Author

sure, to how many - one?

@seuros
Copy link
Contributor

seuros commented Oct 20, 2014

No, always by context. I see 3 context there (refactoring, fix and added specs) and YMMV.

@seuros
Copy link
Contributor

seuros commented Oct 20, 2014

@just3ws :shipit: once 💚

@seuros
Copy link
Contributor

seuros commented Oct 20, 2014

Thanks you @YaroSpace

@YaroSpace
Copy link
Contributor Author

@seuros, np at all. any other comments to cleanup the PR?

@just3ws
Copy link
Contributor

just3ws commented Oct 22, 2014

I'm getting 500 errors because the path is being formatted as "http://localhost:3000/teams?team%5Bselected%5D=true&team%5Bslug%5D=xxx"

@YaroSpace
Copy link
Contributor Author

Arghh....found the bug. It is not the path formatting (it is a valid encoding of nested params %5B='[' and %5D=']'), but something else that I managed to overlook both in code and specs.

I am very sorry about this, will fix asap.

@YaroSpace YaroSpace changed the title Join team error #391 [WIP] Join team error #391 Oct 23, 2014
@just3ws
Copy link
Contributor

just3ws commented Oct 23, 2014

You’re correct, I misread the error. I didn’t get a chance to work through the entire issue but there was another error about missing attributes. :( I think it was name.
-- 
Mike Hall
Sent with Airmail

On October 22, 2014 at 6:36:12 PM, Yaro (notifications@github.com) wrote:

Weird. Are you sure it is because of this?

It is a valid encoding of nested params %5B='[' and %5D=']' and we always had it in app/views/teams/_similar_teams.html.haml:15 prior to my changes.


Reply to this email directly or view it on GitHub.

@YaroSpace YaroSpace changed the title [WIP] Join team error #391 Join team error #391 Oct 23, 2014
split rendering similar teams into a separate template

refactored #creare and added specs

refactored #exact_team_exists? and added specs

added shared notification_bar partial and linked to product_description layout

added showing notifications on team creation
added specs for success/filure notifications
@YaroSpace
Copy link
Contributor Author

Fixed the bug. But please hold until I add the feature specs.

@seuros
Copy link
Contributor

seuros commented Oct 23, 2014

please squash commits

@seuros
Copy link
Contributor

seuros commented Oct 23, 2014

nevermind, i had outdated page, i had to refresh.

@YaroSpace YaroSpace changed the title Join team error #391 [WIP] Join team error #391 Oct 23, 2014
@just3ws
Copy link
Contributor

just3ws commented Oct 23, 2014

Awesome @YaroSpace thanks!

just3ws added a commit that referenced this pull request Oct 24, 2014
@just3ws just3ws merged commit 8595b24 into coderwall:master Oct 24, 2014
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.

3 participants