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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unique nicknames #2360

Merged
merged 38 commits into from Dec 20, 2017
Merged

Unique nicknames #2360

merged 38 commits into from Dec 20, 2017

Conversation

@deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Dec 12, 2017

馃帺 What? Why?

This PR adds a unique nickname column to the users table. This column will be displayed together with the public display information in a "twitter-like" way.

馃搶 Related Issues

馃搵 Subtasks

  • Add nickname column to DB and fill it with current "name" data.
  • Add nickname to new account form and validate it.
  • Display the new nickname together with the "display name" everywhere where the display name is currently displayed.

馃摲 Screenshots (optional)

Proposal List

proposallist

Proposal Page

proposalpage

Commens

comments

Conversations

conversations

馃懟 GIF

plant_love

@codecov
Copy link

@codecov codecov bot commented Dec 13, 2017

Codecov Report

Merging #2360 into master will increase coverage by 0.82%.
The diff coverage is 99.52%.

@@            Coverage Diff             @@
##           master    #2360      +/-   ##
==========================================
+ Coverage   97.81%   98.64%   +0.82%     
==========================================
  Files        1266     1279      +13     
  Lines       29334    29576     +242     
==========================================
+ Hits        28694    29175     +481     
+ Misses        640      401     -239

@deivid-rodriguez deivid-rodriguez force-pushed the feature/unique_nicknames branch 8 times, most recently from d70427a to 03d2d3f Dec 13, 2017
name: Run main folder yarn lint
command: yarn lint
name: Run main folder yarn lint & tests
command: yarn test:ci
Copy link
Contributor

@mrcasals mrcasals Dec 14, 2017

Damn it, sorry :(

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 14, 2017

No problem! :)

@deivid-rodriguez deivid-rodriguez force-pushed the feature/unique_nicknames branch from 03d2d3f to c45b13b Dec 14, 2017
@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Dec 14, 2017

Is this still WIP or should I review it?

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 14, 2017

Just a bit of tweaks left. I'll ping you in a bit!

  • Codeclimate.
  • Rebase.
  • Fix developer omniauth strategy.

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 14, 2017

@josepjaume You can start reviewing it though, there won't be substantial changes.

commentsData.push({
id: random.uuid(),
type: "Decidim::Comments::Comment",
body: lorem.words(),
createdAt: date.past().toISOString(),
createdAt: creationDate.toISOString(),
formattedCreatedAt: creationDate.toLocaleTimeString(),
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 14, 2017

I'm adding this new parameter to unify the format of dates in messages & conversations. Previous dates were very verbose and I couldn't make it look decent with that amount of information. Also, I guess the same logic could be "repeated" in javascript but this was the easiest approach for me.

resolve lambda { |obj, _args, _ctx|
obj.friendly_created_at
}
end
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 14, 2017

This is the new parameter I mentioned in the previous comment.

Copy link
Contributor

@josepjaume josepjaume Dec 15, 2017

it's GraphQL and denormalizing fields doesn't come at any cost, so great for me 馃憤

Copy link
Contributor

@mrcasals mrcasals Dec 15, 2017

I think you can simplify this:

field :formattedCreatedAt, !types.String, "The creation date of the comment in relative format", property: : friendly_created_at

@@ -36,6 +36,8 @@ def create
on(:error) do |user|
if user.errors[:email]
set_flash_message :alert, :failure, kind: @form.provider.capitalize, reason: t("decidim.devise.omniauth_registrations.create.email_already_exists")
elsif user.errors[:nickname]
set_flash_message :alert, :failure, kind: @form.provider.capitalize, reason: t("decidim.devise.omniauth_registrations.create.nickname_already_exists")
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 14, 2017

This error logic is not very good (one single error per form, so two invalid submissions if both fields are incorrect), and I'm not sure it's correct at all. Some tests I made led to user.errors[:email] being empty and not nil when no errors were found so maybe this error is being displayed everytime if the form has any errors at all?

@@ -41,6 +44,10 @@ def email_unique_in_organization
errors.add :email, :taken if User.where(email: email, organization: current_organization).first.present?
end

def nickname_unique_in_organization
errors.add :nickname, :taken if User.where(nickname: nickname, organization: current_organization).first.present?
end
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 14, 2017

We have a lot of "duplicated validations" between the registration form and the User model. I just kept repeating them but maybe we can improve things in this regard in the future?

Copy link
Contributor

@josepjaume josepjaume Dec 19, 2017

I think they have different purposes. models deal with data integrity and forms with data normalization & user input validation. I think it's natural that some validations (like presence) are duplicated, but others (length, format) are form-specific. Does this make sense?

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 19, 2017

When I checked some validations felt off at the model level, but I didn't want to explore deeper. But yeah, you're right.

new:
complete_profile: Complete profile
nickname_help: Your unique identifier in decidim.
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 14, 2017

Once we add mentions and a public profile page, we can mention those things here. For now, I don't know what else to say to explain this field.


def migrate!
User.find_each do |user|
user.update!(nickname: User.nicknamize(user.name))
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 14, 2017

I added this "migrator" class so I could properly unit test it, but in the end most of the logic went down to the model, so this could be removed and the tests moved around.

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 15, 2017

I ended up removing it because I found a bug in nickname disambiguation and it was hard to test it against the migration. Now I'm testing the Nicknamizable mixin directly.

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 14, 2017

I added some self-review comments, and I'll be posting some screenshots in a bit.

@deivid-rodriguez deivid-rodriguez force-pushed the feature/unique_nicknames branch from c45b13b to 4f17f08 Dec 14, 2017
@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 14, 2017

Added a few screenshots!

@deivid-rodriguez deivid-rodriguez force-pushed the feature/unique_nicknames branch from 4f17f08 to 7125dd8 Dec 14, 2017
@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 14, 2017

I detected some bugs, working on them and adding tests.

@deivid-rodriguez deivid-rodriguez force-pushed the feature/unique_nicknames branch from 7125dd8 to 29da631 Dec 14, 2017
Copy link
Contributor

@josepjaume josepjaume left a comment

I've left some comments, but this is in good state. Nice job!

end
end

def clon(candidate, number)
Copy link
Contributor

@josepjaume josepjaume Dec 19, 2017

Shouldn't this be clone?

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 19, 2017

mmm, I guess? Is this a grammar nit or something else?

Copy link
Contributor

@josepjaume josepjaume Dec 19, 2017

It was just grammar extremism, but I'm thinking maybe clone will collide with Ruby's object.clone?


module Decidim
# This concern contains the logic related to nicknames.
module Nicknamizable
Copy link
Contributor

@josepjaume josepjaume Dec 19, 2017

Since we're including validation logic here like nickname_length_range, I would turn this into a full-fledged ActiveSupport::Concern (or its lowest-level approach, if you like it).

Also, a bit of documentation regarding its usage & methods wouldn't harm. This is quite a sensible part of the code that might need a bit of understanding.

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 19, 2017

I can document the public methods 馃憤.

Regarding the other suggestion, I'm "meh" on it, but I'll do it!

module Decidim
module Proposals
#
# A Null Object to abstract out the author of an official proposal.
Copy link
Contributor

@josepjaume josepjaume Dec 19, 2017

Not to be a purist, but this isn't a null object (as that would be an object that returns null regardless of the method call) but rather a dummy presenter (and not sure about this either xD).

Copy link
Contributor

@josepjaume josepjaume Dec 19, 2017

Either way, really good abstraction 馃憤

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 19, 2017

Aaaaaahm, ok, I'll change the comment.

#
class ProposalPresenter < SimpleDelegator
def author
@author ||= if official?
Copy link
Contributor

@josepjaume josepjaume Dec 19, 2017

Super nice.

@@ -21,6 +21,11 @@
)
end

matcher :have_author do |name|
Copy link
Contributor

@josepjaume josepjaume Dec 19, 2017

We should be using this more often.

Copy link
Contributor

@mrcasals mrcasals Dec 19, 2017

+1! 鉂わ笍

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 19, 2017

Let me know what you think now. Specifically, if 8350dab is what you had in mind.

end
end

def clon(candidate, number)
Copy link
Contributor

@mrcasals mrcasals Dec 19, 2017

Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Dec 19, 2017

I ended up changing the name of the method so it doesn't use none. Since clone is a core low level method, I don't want confusions!

@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Dec 19, 2017

Hey! Thanks for the changes! 8350dab it actually half way of what I meant (but probably did a poor job explaining myself).

What I meant is: It looks like nicknamizable could come with ActiveRecord validations & other stuff, so why not put them in there instead of the models? Or is there any reason not to?

Anyway, if you think it'd be taking this too far, we could just merge it 馃憤

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 19, 2017

Aaaaah, I see. I can change that, yeah.

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 19, 2017

@josepjaume Unfortunately moving the validations doesn't work very well, since the mixin was included in both a form and a model, but the model has some conditionals on the validation. I moved the conditionals in 0788e33, but it looks plain wrong to me because now the mixin depends on some totally unrelated methods being defined.

Let me know how you want me to proceed.

@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 19, 2017

I could maybe add allow_blank to the length validation?

* Don't use clon or clone, since it conflicts with core methods and it's
a confusing words anyways.

* Rename local parameter, `candidate` doesn't make much sense here.
And add tests for length validation.
I guess for low values of length parameters, we run out of unique
values.
@deivid-rodriguez deivid-rodriguez force-pushed the feature/unique_nicknames branch from 4433d5b to 315167f Dec 19, 2017
@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 19, 2017

Adding allow_blank worked fine. Ready again!

@deivid-rodriguez deivid-rodriguez mentioned this pull request Dec 19, 2017
2 tasks
@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Dec 20, 2017

Merging this! Super nice job! 馃帀 After this, do you mind rebasing the profile page on top of master?

@josepjaume josepjaume merged commit 2e2931a into master Dec 20, 2017
19 checks passed
@ghost ghost removed the in-progress label Dec 20, 2017
@josepjaume josepjaume deleted the feature/unique_nicknames branch Dec 20, 2017
@deivid-rodriguez
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez commented Dec 20, 2017

Thanks @josepjaume, it's rebased!

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

Successfully merging this pull request may close these issues.

None yet

3 participants