-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
Town Square MVP #5744
Town Square MVP #5744
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5744 +/- ##
========================================
Coverage ? 29.2%
========================================
Files ? 264
Lines ? 23543
Branches ? 3631
========================================
Hits ? 6875
Misses ? 16344
Partials ? 324
Continue to review full report at Codecov.
|
@@ -230,18 +230,19 @@ const alertMessage = function(msg) { | |||
return html; | |||
}; | |||
|
|||
const _alert = function(msg, _class) { | |||
const _alert = function(msg, _class, remove_after_ms) { |
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.
this is quite a nice little feature... removing the msg after some time... clears notifications so users dont have to!
app/townsquare/views.py
Outdated
|
||
|
||
@ratelimit(key='ip', rate='10/m', method=ratelimit.UNSAFE, block=True) | ||
@csrf_exempt |
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.
havent figured out how to feed enough csrf tokens to the frontend to make multi comment experience work well. punting for now.
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.
So I spent some time looking into this and I believe you can use the same CSRF token multiple times - it only gets invalidated on user login. https://stackoverflow.com/questions/25507514/does-django-csrf-token-must-be-unique-on-every-request
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.
fixing this now
@@ -1498,6 +1498,16 @@ $(document).ready(function() { | |||
}); | |||
}); | |||
|
|||
const copyToClipboard = str => { |
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.
don't know if exactly what you need but there is https://github.com/gitcoinco/web/blob/master/app/assets/v2/js/clipboard.js
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.
hmmm yeah this function is somehwat different (creates a diff message in a diff way, my copyToClipBoard()
file, which is more generlizaable...
made a quick view looking nice I need to go deeper. |
good call; i wish i had thought of the vue stuff before i broke ground on this! |
app/dashboard/models.py
Outdated
@property | ||
def text(self): | ||
from django.template.loader import render_to_string | ||
from bs4 import BeautifulSoup |
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.
We should move these imports to module scope
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.
done
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.
Fat PR! Looks good though - I think we can deploy Wednesday if you can turn around some of these comments quick
for to_email in to_emails: | ||
cur_language = translation.get_language() | ||
try: | ||
setup_lang(to_email) |
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 move the setup_lang
and cur_language
calls out of the loop for a tiny bit of performance
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.
cur_language and setup_lang are each relative to each to_email
so we cant do that..
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.
my bad on the setup_lang
part - for the get_language
part my understanding is that we are storing the current language (english) - then translating the email via the setup_lang
function which changes the current language, then going back to english with the finally:
section. So its probably good to move out of the loop, right?
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.
updated!
app/marketing/mails.py
Outdated
@@ -747,7 +787,24 @@ def new_bounty_daily(bounties, old_bounties, to_emails=None): | |||
plural = "s" if len(bounties) != 1 else "" | |||
worth = round(sum([bounty.value_in_usdt for bounty in bounties if bounty.value_in_usdt]), 2) | |||
worth = f" worth ${worth}" if worth else "" | |||
subject = _(f"⚡️ {len(bounties)} New Open Funded Issue{plural}{worth} matching your profile") | |||
offers = f"" | |||
if to_emails and len(to_emails): |
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 is the len
check here covering?
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.
removing
app/marketing/mails.py
Outdated
subject = _(f"⚡️ {len(bounties)} New Open Funded Issue{plural}{worth} matching your profile") | ||
offers = f"" | ||
if to_emails and len(to_emails): | ||
from townsquare.utils import is_email_townsquare_enabled, is_there_an_action_available |
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.
is this to avoid a circular import? Performance isn't a huge deal here since its outside of a request/response cycle but always good to check
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.
nah; just laziness.. removing
app/townsquare/views.py
Outdated
|
||
|
||
@ratelimit(key='ip', rate='10/m', method=ratelimit.UNSAFE, block=True) | ||
@csrf_exempt |
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.
So I spent some time looking into this and I believe you can use the same CSRF token multiple times - it only gets invalidated on user login. https://stackoverflow.com/questions/25507514/does-django-csrf-token-must-be-unique-on-every-request
return redirect('/login/github?next=' + request.get_full_path()) | ||
if request.user.profile.offeractions.filter(what='click', offer=offer) and not is_debugging_offers: | ||
raise Exception('already visited this offer') | ||
OfferAction.objects.create(profile=request.user.profile, offer=offer, what='click') |
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.
Lots of code in these methods we could DRY up
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.
id be open to discussions about how to do that... if we were gonna get fancy we could use an inherited class or view or something..
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 just a non-view helper function is probably fine, something like offer = get_offer_and_create_offer_action(offer_id, what='click')
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.
updated
@danlipert i turned around all the code review here => 3288bbb lets aim for weds deploy pls! |
latest code review turnaround => abd3563 |
app/app/urls.py
Outdated
@@ -361,6 +364,16 @@ | |||
re_path(r'^results/?(?P<keyword>.*)/?', retail.views.results, name='results_by_keyword'), | |||
re_path(r'^results/?', retail.views.results, name='results'), | |||
re_path(r'^activity/?', retail.views.activity, name='activity'), | |||
re_path(r'^townsquare/?', townsquare.views.index, name='townsquare'), | |||
re_path(r'^$', townsquare.views.index, name='inex'), |
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.
typo? "inex"
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.
fixed
@owocki There are some stuff I can review but instead since a design for this is coming I can fix css and styles then. |
Description
This is the MVP of Town square. I will be demoing on tomorrows sprint meeting. The idea is to be the central nervous system of Gitcoin. A place to view social updates + find out whats new on the network
My hope is that this functionality MASSIVELY increases DAUs on Gitcoin
Includes
Refers/Fixes
https://app.mural.co/t/consensys3989/m/consensys3989/1576552443637/93b352eccd110e4827ae96f3533fa3de3a5b5571
Testing
Tested a butt ton locally
Release Plan
I plan to release this incrementally to production behind a feature flag. Since its a very social feature, iterative user feedback in production will be important (theres real data in production, so the social experience is more meaningful!).
My Todo list