Expanded Team Social Account Links #1320
4 participants
gregmarra
commented on an outdated diff
| + return social_dict | ||
| + | ||
| + @classmethod | ||
| + def _partial_social_dict_from_github(cls, url): | ||
| + social_dict = {} | ||
| + social_dict['social_type_enum'] = SocialConnectionType.GITHUB | ||
| + foreign_key = cls._parse_github_foreign_key(url) | ||
| + if foreign_key is None: | ||
| + logging.warning("Failed to determine foreign_key from url: {}".format(url)) | ||
| + return None | ||
| + social_dict['foreign_key'] = foreign_key | ||
| + | ||
| + return social_dict | ||
| + | ||
| + @classmethod | ||
| + def _parse_facebook_foreign_key(cls, url): |
gregmarra
These methods feel repetitive. Is there a way we can abstract them more by passing in a regex? |
gregmarra
commented on an outdated diff
| + conns_by_type[slug_name] = [conn] | ||
| + return conns_by_type | ||
| + | ||
| + | ||
| +class SocialConnectionParser(object): | ||
| + FACEBOOK_URL_PATTERNS = ['facebook.com/'] | ||
| + TWITTER_URL_PATTERNS = ['twitter.com/'] | ||
| + YOUTUBE_URL_PATTERNS = ['youtube.com/user/'] | ||
| + GITHUB_URL_PATTERNS = ['github.com/'] | ||
| + | ||
| + @classmethod | ||
| + def partial_social_dict_from_url(cls, url): | ||
| + """ | ||
| + Takes a url, and turns it into a partial Media object dict | ||
| + """ | ||
| + if any(s in url for s in cls.FACEBOOK_URL_PATTERNS): |
|
gregmarra
Is there a way to put all the patterns in a list (maybe initially have them all in a dict?) and iterate through it? This feels repetitive. It seems like you'd need to touch code in a lot of places to add a new pattern. |
gregmarra
commented on an outdated diff
| @@ -0,0 +1,3 @@ | ||
| +<div> |
|
gregmarra
Why does each social need it's own partial? They all have the same structure. Why not just an abstract |
| + <div class="row"> | ||
| + <div class="col-xs-12 col-lg-6 col-lg-offset-3"> | ||
| + <div class="panel panel-default"> | ||
| + <div class="panel-heading"> | ||
| + <h1 class="panel-title">Add a team's social account to The Blue Alliance!</h1> | ||
| + </div> | ||
| + <div class="panel-body"> | ||
| + <p>Thanks for helping make The Blue Alliance better! Let us know about a social account so we can add them to the site!</p> | ||
| + <ul> | ||
| + <li>Your suggestion will be reviewed by a moderator</li> | ||
| + <li>Your account info (like {{user_bundle.user.email}} and {{user_bundle.user.nickname}}) will also be submitted. <a href="{{user_bundle.logout_url}}">log in as a different user</a></li> | ||
| + </ul> | ||
| + | ||
| + <hr> | ||
| + | ||
| + <p><strong>Currently supported formats are:</strong></p> |
|
gregmarra
If you wanted to get fancy, you could generate this from your |
| @@ -0,0 +1,69 @@ | ||
| +import json |
|
gregmarra
Why have this as a join model instead of a dict property on 
phil-lopreiato
The idea is that we can eventually link to other models that aren't
phil-lopreiato
Do you think it would be better to do as dicts in each model? I thought we were trying to move away from those JSON properties because it made more complex queries and mutations more difficult. I'm open to changing it if we think it's better done by a different approach, though 
fangeugene
I'm not sure, I was just playing devil's advocate. One way to decide would be to think about how we would query them. I'm a fan of the JSON property because I don't think we would ever query for anything other than "get me all social connections for team XXX." e.g. we would never do "get me all Facebook connections for event XXX (which would us to make an intermediate query for Teams anyways)." Also, unlike Robots, the number of social connections doesn't increase over time for one Team.
phil-lopreiato
That's a good point. I'd agree the strongest argument in favor of doing it as a JSON property is that the relationship will always be 1:1 (connection of a specific type : parent model, eg team) and that we'll only query for connections associated with a given team, like you said. Maybe I'll give it a shot tomorrow and see how it looks that way
fangeugene
To add to your 1:1 argument, Robots are 1:1 too (which we have separate Models for), but they grow linearly with the number of years. Connections don't.
gregmarra
I could see a single team sometimes having more than one github or more than one instagram, although I agree this wouldn't be super common. I think the biggest argument for this join model is that we can use these for Teams, Events, Districts etc. That will let us easily re-use the same model and templates and such with less duplicated code. If we wanted to store more complicated data about a SocialConnection (like scraping posts? last updated time? fan counts?) we'd be able to store them on these models as well. 
chacha
Let's just avoid creating Twitter accounts for individual matches. That might be a bit excessive. |
gregmarra
commented on an outdated diff
| + def __init__(self, *args, **kw): | ||
| + super(SocialConnection, self).__init__(*args, **kw) | ||
| + | ||
| + @property | ||
| + def slug_name(self): | ||
| + return self.SLUG_NAMES[self.social_type_enum] | ||
| + | ||
| + @classmethod | ||
| + def create_reference(self, reference_type, reference_key): | ||
| + return ndb.Key(self.REFERENCE_MAP[reference_type], reference_key) | ||
| + | ||
| + @property | ||
| + def key_name(self): | ||
| + return self.render_key_name(self.social_type_enum, self.foreign_key) | ||
| + | ||
| + # URL Renderers |
|
gregmarra
Since each of these are just a pattern with a foreign_key passed in, why not have them all defined as consts and have single function? |
|
I love this! We can also use social plugins to add a Twitter stream, Facebook stream, etc. This is going to need some adjustments after I land #1306. I apologize for leaving this in limbo so long – I should try to get it merged this weekend. A few other comments inline, mostly to try to abstract things to make extending this later easier. |
|
Good stuff. |
|
I just landed #1306, so this should be reworked to follow those patterns for suggestions. |
|
Okay, master is all merged in and suggestions don't require admin to approve now. I think this pull is ready to go, I'll add in exposing this data in a separate diff. I left the storage as a join model. I think that it's the best choice,mainly because it makes future modularity much simpler, especially when adding social connections to other models and provides an easy way to store metadata about those connections. Does anybody feel differently? |
phil-lopreiato
changed the title from [RFC] Expanded Team Social Account Links to Expanded Team Social Account Links 
|
|
|
Thinking about this more, why not re-use the https://github.com/the-blue-alliance/the-blue-alliance/blob/master/models/media.py It has the same shape: a reference and a foreign key. |
|
They're similar classes in structure, but would it be confusing to make it so a Can you think of a better name for |
|
How would we want to represent each of these?
I guess these are all "references to foreign keys on other sites with some logic based on type". |
|
So we can have one
I could see that working, and probably be cleaner across the codebase when compared to the multiple solutions that currently exist. Thoughts @fangeugene? |
|
I think my main objection right now is that |
|
Fair point :) I can refactor this to include:
|
|
@phil-lopreiato do you know the implications of renaming a Model on an appengine app? I bet it breaks everything :/ |
|
It does break everything. |
|
Shoot. Should I just use |
|
@phil-lopreiato I think using the What would be the right pattern for taking a Media and making, like, a |
|
Not sure about that. Would subclasses play nice with ndb? Or should we go with some helper functions to go from Media to some dict that can be different based on type |
|
Let's go with Media for this. I think the format is basically correct, and we can just use it as a generic foreign key mapper to our own models. I'm not worried about the slightly confusing name. |
|
Sounds good, I'll get this diff refactored and ready to ship this week. |
|
Closing in favor of #1389 |
Something we've been talking about doing for a long time and I've slowly been hacking away at.
Support for adding links to social accounts with a similar suggestion/storage model as media. Currently allows FB, Twitter, YouTube, and GitHub profile links.
This PR doesn't display connections on team pages yet, but that's next on my list.
