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

Fix issue #6847 #6905

Merged
merged 5 commits into from
Jul 5, 2016
Merged

Fix issue #6847 #6905

merged 5 commits into from
Jul 5, 2016

Conversation

aoh0x7DE
Copy link
Contributor

@aoh0x7DE aoh0x7DE commented Jul 5, 2016

No description provided.

@jhass
Copy link
Member

jhass commented Jul 5, 2016

Thank you! Could you add a test or two?

@aoh0x7DE
Copy link
Contributor Author

aoh0x7DE commented Jul 5, 2016

Sure. I'm new to rspec, so you might want to double-check me on those.

problem_user.save
end
end
end
Copy link
Member

Choose a reason for hiding this comment

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

I don't think setting it to nil can ever make a user fail validation, so User.where(unconfirmed_email: email).update_all(unconfirmed_email: nil) should be much, much faster.

@aoh0x7DE
Copy link
Contributor Author

aoh0x7DE commented Jul 5, 2016

Okay, I'll make those changes. Should I change the other tests which use [email protected] while I'm at it?

Forgive my noobishness, but is there an example of an error using a translation somewhere in the codebase I could look at? Not quite sure how to go about it as-is.

Also, just changed the controller code a little. Pronto is now complaining about the line after the one I changed having single quotes instead of double; I'm tentatively ignoring this (?) because it matches all the others.

@jhass
Copy link
Member

jhass commented Jul 5, 2016

Yeah, changing the other tests seems fine.

http://guides.rubyonrails.org/i18n.html#translations-for-active-record-models goes a bit into validation error translations, I actually never tried myself, but if the interpolation variant works for custom errors, it would be highly preferable. Would be awesome if you could find out. I think we also have some stuff to clean up if so ;) If you need to add any translation strings, only edit the English locale.

You always should follow pronto's messages. We introduced a fixed styleguide much much later and are porting the codebase to it as we touch things. Porting everything to a consistent style at once would just be too many changes at once.

@aoh0x7DE
Copy link
Contributor Author

aoh0x7DE commented Jul 5, 2016

Got everything sorted. As it turns out, there was a message (errors.messages.taken) that matched what I was after, so I didn't end up learning about custom messages.

However, the message might not be relevant at the moment. Currently, the users controller flashes generic messages on a failed save (e.g., "Email change failed") rather than making any reference to the errors in question. Is this behavior that should be modified?

@@ -111,7 +112,7 @@ def public
respond_to do |format|
format.atom do
@posts = Post.where(author_id: @user.person_id, public: true)
.order('created_at DESC')
.order("created_at DESC")

Choose a reason for hiding this comment

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

Align .order with .where on line 114.

@jhass
Copy link
Member

jhass commented Jul 5, 2016

Is this behavior that should be modified?

Perhaps, but not in this PR ;)

@jhass jhass added this to the 0.6.0.0 milestone Jul 5, 2016
@jhass jhass merged commit d75f795 into diaspora:develop Jul 5, 2016
@jhass
Copy link
Member

jhass commented Jul 5, 2016

Thanks!

jhass added a commit that referenced this pull request Jul 5, 2016
@aoh0x7DE aoh0x7DE deleted the emailfix branch July 6, 2016 00:46
@aoh0x7DE aoh0x7DE restored the emailfix branch July 6, 2016 00:55
@aoh0x7DE aoh0x7DE deleted the emailfix branch July 6, 2016 00:56
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