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

Feature/retro participation #115

Merged
merged 6 commits into from
Apr 26, 2017
Merged

Feature/retro participation #115

merged 6 commits into from
Apr 26, 2017

Conversation

pulsedemon
Copy link
Contributor

No description provided.

@vanderhoop
Copy link
Contributor

vanderhoop commented Apr 25, 2017

This looks great, but there's a weird bug in that, if a user had a session before the user persistence story got merged, this persistence breaks in the retros controller, because the user on session isn't backed up by a persisted record, and therefore no user is available to pluck the id off of. we could handle that via having a consistent dev user that we add in seeds? @pulsedemon @Zanadar thoughts?

example:

image

@pulsedemon
Copy link
Contributor Author

@vanderhoop Regarding the issue you mentioned about an existing session without a record in the users table: What if we add a check somewhere to kill their session when a db record doesn't exist? This is an edge case, so we could remove the code later on.

@vanderhoop
Copy link
Contributor

@pulsedemon that's a good thought; we really just need a bridge until all the sessions that could have been introduced expire. let me sleep on it; we might be overthinking the problem.

@vanderhoop
Copy link
Contributor

yo @pulsedemon! yo @Zanadar to handle this edge case, the simplest thing we can do is to NOT persist a participation when there is no user returned by Repo.get_by(User, email: user["email"]) Then, in a couple weeks, we can remove this code, as it's only a bandaid.

Get that in, and we can merge this puppy.

@pulsedemon
Copy link
Contributor Author

@vanderhoop if we do that, then once the email stuff is merged in, those users will be left out of the emails.

@vanderhoop
Copy link
Contributor

vanderhoop commented Apr 26, 2017 via email

@vanderhoop
Copy link
Contributor

@pulsedemon the reason that's okay for now is that those users aren't getting emails right now anyway.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 94.444% when pulling 454776c on feature/retro-participation into 6f09aa3 on master.

@vanderhoop
Copy link
Contributor

@pulsedemon @Zanadar Shit. Didn't realize this lacked tests for the AuthController code. we need a test that a Participation is persisted when there's a persisted user.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 94.382% when pulling e792186 on feature/retro-participation into 6f09aa3 on master.

@vanderhoop
Copy link
Contributor

mergin'!

@vanderhoop vanderhoop merged commit 36cfba7 into master Apr 26, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 94.382% when pulling 21ccbeb on feature/retro-participation into 6f09aa3 on master.

@vanderhoop vanderhoop deleted the feature/retro-participation branch February 23, 2018 01:39
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.

4 participants