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

Add shortcut to tip user #5415

Merged
merged 9 commits into from
Dec 13, 2019
Merged

Conversation

zoek1
Copy link
Contributor

@zoek1 zoek1 commented Oct 29, 2019

Description

When a user adds the query parameter with a user handle, if the user exists set the user data at the tip form, if not display a error messages.

Refers/Fixes

#5346

Testing

tip_test

@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #5415 into master will increase coverage by 0.39%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5415      +/-   ##
==========================================
+ Coverage   29.83%   30.23%   +0.39%     
==========================================
  Files         241      248       +7     
  Lines       20433    21200     +767     
  Branches     2926     3072     +146     
==========================================
+ Hits         6096     6409     +313     
- Misses      14086    14515     +429     
- Partials      251      276      +25
Impacted Files Coverage Δ
app/app/urls.py 90.19% <ø> (+0.19%) ⬆️
app/dashboard/tip_views.py 17.41% <0%> (-1.31%) ⬇️
app/kudos/test_views.py 0% <0%> (-100%) ⬇️
app/kudos/test_models.py 0% <0%> (-100%) ⬇️
app/kudos/test_utils.py 0% <0%> (-70.59%) ⬇️
app/retail/templatetags/is_in_list.py 66.66% <0%> (-33.34%) ⬇️
...eting/management/commands/assemble_leaderboards.py 61.72% <0%> (-13.38%) ⬇️
app/kudos/views.py 15.59% <0%> (-6.52%) ⬇️
app/kudos/admin.py 58.82% <0%> (-3.52%) ⬇️
app/quests/models.py 42.85% <0%> (-2.33%) ⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c3f67c...c82443b. Read the comment docs.

let existent_properties = {};

if (base_user.element) {
let attr = base_user.element.attributes;
Copy link
Member

@thelostone-mc thelostone-mc Oct 29, 2019

Choose a reason for hiding this comment

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

would it make sense to keep this as const ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 maybe, we dont want alterate the attributes object, so const makes more sense than 'let`.

let attr_length = attr.length;


for (var i = 0; i < attr_length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

let i

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link
Contributor

@danlipert danlipert left a comment

Choose a reason for hiding this comment

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

Just one question but otherwise looks good

@@ -379,5 +395,7 @@ def send_tip_2(request):
'from_handle': from_username,
'title': 'Send Tip | Gitcoin',
'card_desc': 'Send a tip to any github user at the click of a button.',
'user_json': user,
'username': username
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need to conditionally include these context params to prevent someone crafting a malicious URL and sending it to another user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added!

{% endblock %}
@ratelimit(key='ip', rate='5/m', method=ratelimit.UNSAFE, block=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

this need to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's embarrassing, i apologize for that, I remove immediately.

@zoek1 zoek1 requested review from danlipert and octavioamu November 5, 2019 16:09
{% if not user_json and username %}
<script>
setTimeout(function() {
alert("Sorry, we can find the user @{{ username }}");
Copy link
Contributor

Choose a reason for hiding this comment

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

lets remove the username here - I'm not sure exactly how the django template injection prevention would work here, but I worry someone will create a URL with the username "); do_something_bad();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - I'm just paranoid :) I trust the Django devs are good and all, but you can never be too careful!

@zoek1 zoek1 requested a review from danlipert November 7, 2019 06:06
@@ -172,7 +172,7 @@ <h3>{% trans "with instructions about how to receive their tip." %}</h1>
{% if not user_json and username %}
<script>
setTimeout(function() {
alert("Sorry, we can find the user @{{ username }}");
alert("Sorry, we can find the user");
Copy link
Contributor

Choose a reason for hiding this comment

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

@thelostone-mc we should use _alert here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@zoek1 zoek1 requested a review from danlipert November 19, 2019 06:15
@zoek1
Copy link
Contributor Author

zoek1 commented Nov 28, 2019

@octavioamu could you review this PR? 😄

@owocki
Copy link
Contributor

owocki commented Dec 6, 2019

@zoek1 what would you guys think of adding a 'tip this user' button on the gitcoin profiile at https://bits.owocki.com/P8uYjrNd when this goes live? putting it next to the chat icon could be a really compelling way to drive more tips.

@octavioamu @danlipert i'm excited for this to go live! can we get some review cycles on it?

@zoek1
Copy link
Contributor Author

zoek1 commented Dec 6, 2019

That would be great @owocki, definitely will be more accessible and I hope increase the tips for users. The button should be only text or will has the tip icon? Also will be the same color as the chat icon?

{% if not user_json and username %}
<script>
setTimeout(function() {
_alert("Sorry, we can find the user");
Copy link
Contributor

Choose a reason for hiding this comment

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

can -> can't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

is_user_authenticated = request.user.is_authenticated
from_username = request.user.username if is_user_authenticated else ''
primary_from_email = request.user.email if is_user_authenticated else ''

user = {}
if username:
profiles = Profile.objects.filter(handle__icontains=username)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should do an exact match here to prevent similar names matching: i.e. I try to tip user Dan but theres a user named DanBot. Since there's no order_by clause we can't be sure which user will get returned. Better to use iexact here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, i already changed it! 😃

@owocki
Copy link
Contributor

owocki commented Dec 6, 2019

@zoek1 same color as the chat icon pls + feel free to use this fontawesome icon for it https://fontawesome.com/icons/ethereum?style=brands

@zoek1
Copy link
Contributor Author

zoek1 commented Dec 9, 2019

The tip button at the user profile page look like this:
Opera Instantánea_2019-12-09_005042_localhost

@owocki

@owocki
Copy link
Contributor

owocki commented Dec 9, 2019

@zoek1 looks really good! im pumped.. @danlipert can we target release this week for this

@owocki
Copy link
Contributor

owocki commented Dec 11, 2019

@danlipert can we target release this week for this

looks like it didnt get into the release? any objections if i test/push it myself?

@owocki owocki merged commit ce6f397 into gitcoinco:master Dec 13, 2019
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.

5 participants