-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Custom realm emoji #543
Custom realm emoji #543
Conversation
Automated message from Dropbox CLA bot @blablacio, it looks like you've already signed the Dropbox CLA. Thanks! |
name = models.TextField() | ||
img_url = models.TextField() | ||
name = models.TextField(validators=[MinLengthValidator(1), RegexValidator(regex=r'^[0-9a-zA-Z_\-\.]*$')]) | ||
img_url = models.URLField() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens to existing instances of a model when you change the type of a field from TextField to URLField? Seems like this may involve a messy migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, on the database the field is changed from text
to character varying(200)
as URLField enforces a max_length=200
by default. Other than that there should be no problems as values are still retained, even if they don't represent valid URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, so you're saying that either Django or Postgres retains the value in the existing column even when its type is changed from text
to varchar(200)
? Seems worth testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the case here in particular and in most (if not all) cases when changing from text
to character varying
and vice versa. I think it would only be a problem if we already have URLs longer than 200 characters in existing databases. If you think this is worth testing or even extending the migration in order to handle such cases, let me know.
Maybe I should rather set a bigger max_length
to the URLField in order to avoid such problems? A perfect solution would be to also check the existing URLs in the database against the URLValidator that Django provides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there aren't a ton of these in existence, and a 200 character URL is pretty long, so I think it's OK -- just wanted to make sure we're not setting ourselves up for an unpleasant migration.
Thanks for working on this @blablacio! I posted a bunch of comments (mostly on small style issues); you'll also want to fix the lint/test errors. |
6ef4a2b
to
310c0f5
Compare
Fixed issues and squashed commits. There was a problem with the lint script, I'll push a fix in the |
8116987
to
e4e563d
Compare
@@ -214,8 +215,8 @@ def remote_user_to_email(remote_user): | |||
|
|||
class RealmEmoji(models.Model): | |||
realm = models.ForeignKey(Realm) | |||
name = models.TextField() | |||
img_url = models.TextField() | |||
name = models.TextField(validators=[MinLengthValidator(1), RegexValidator(regex=r'^[0-9a-zA-Z_\-\.]*$')]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment describing the logic behind why this is the right Regex to user here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a comment, even though I'm not sure this is the best regex. That's what we initially discussed, but I think it doesn't make much sense to allow punctuation at the end of the name and I think that would cause some issues with the requests in the frontend too (I think we don't encode the name of the emoji when deleting).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, probably makes sense to harden the regex then. Ideally we wouldn't accept names that are going to cause problems with the frontend (though arguably that should be fixed by encoding the name properly?).
I took a deeper look at the implementation and posted a few comments; I also played around with the feature, and have a few thoughts on how to improve up the design (screenshot here for others following along).
What do you think of those ideas @blablacio ? |
e4e563d
to
2249ba3
Compare
Added Node tests in |
You comments about the visual improvements are spot on, will make the needed changes. |
As a quick thought, I think we should have the 3 columns of the table next
to each other rather than having that huge space between "name" and "image"
|
@timabbott So, basically leaving the empty space after the third column? |
Yeah. I think it might make sense to also think about making the overall widget have more of a margin on the left side (though if we do that we should do something that looks consistent with the other pages, so probably that would mean adding margin for this category of widget; looking at the other similar pages, I think it'd probably make them all look better)... |
(And sorry for the slow reply! Last week was very busy) |
.admin_emoji_table th.image {width: 80px;} | ||
|
||
.emoji_image {width: 50px; display: block;} | ||
.emoji_image img {max-width: 100%;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think stylistically our CSS always has a newline after the {
, so probably should do that here too for consistency.
@blablacio it'd be great if you could focus on getting this one finished, since I think it's almost done! |
14ad252
to
524f188
Compare
Sorry for the late response, last couple of weeks were really busy. All issues have been addressed and code rebased, you can take a look at the emoji table and let me know if I should wrap the tables on other admin tabs similarly. |
Cool, thanks @blablacio! The new changes are definitely an improvement, and I think the new admin page form looks pretty good. I noticed this issue while playing with this:
|
$("<p>").addClass("text-error").text($.parseJSON(xhr.responseText).msg) | ||
); | ||
} else { | ||
btn.text("Failed!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense for this to use Zulip's standard ui.report_error
mechanism?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably make sense, but I think this is the "canonical" way to do it (at least looking at admin.js). It sounds reasonable to me as we denote the table row within which the error occurs. I could change it if you think it makes sense, but I guess it would be a good idea to change the other few occurrences of this pattern in admin.js too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so it is. We should probably switch these but maybe makes sense to do in a separate commit / PR -- doesn't seem super important. Can you open an issue and label it with "refactoring"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do.
Oh one more note on the Emoji creation form: we should probably give a wider text box for the URL field since many URLs will probably not fit there. |
Okay, I'll investigate what is the case with the capital letters in the name and address the other issues you've pointed. |
Okay, so here's the deal with the capital letters: in composebox_typeahead.js we have If I remove the |
My guess is that code's purpose is to make emoji names case-insensitive, which is probably a pretty reasonable way for them to work. Maybe try making it |
Yep, that would make sense. Will change it as suggested and run tests. |
Yeah that seems reasonable |
524f188
to
233bc73
Compare
All done, I unified the regexes in models and urls too. |
233bc73
to
c2233e1
Compare
Great! Looks like there are some issues with narrow screens; I think it's reasonable for the text box to shrink in that case but probably not great for it to stick out over the border area. Oh, one more thing -- ideally, the Zulip tests would work without Internet access, so maybe the example image URL used in the tests should be something included in the Zulip distribution (e.g. a "jenkins" emoji that points to /static/images/integrations/logos/jenkins.png). |
Okay, I'll set the input width in percents, so it shrinks with the screen size. I'll also look into making tests run without an internet connection. |
API call to delete_emoji removed as realm emoji are already populated in page_params. Realm emoji name validation regex changed to disallow names ending with punctuation. do_add_realm_emoji renamed to check_add_realm_emoji. Fix a problem with capital letters in emoji names. Make tests run with locally hosted images. Tweaking emoji table style.
c2233e1
to
d3e35ea
Compare
Done. |
Probably should rename |
Otherwise lgtm, I'll just fix that and merge. Thanks @blablacio! |
I split out f5fe2d4 into its own commit, since it's in some ways an unrelated bug fix, and then merged this. Thanks @blablacio! |
Adding some functionality to allow for managing custom realm emoji from admin.