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

Nothing found #5250

Merged
merged 2 commits into from
Oct 1, 2019
Merged

Nothing found #5250

merged 2 commits into from
Oct 1, 2019

Conversation

vince0656
Copy link
Contributor

@vince0656 vince0656 commented Sep 24, 2019

Description

As a contributor using the explorer if nothing is found the "nothing found" message should have a link to https://gitcoin.co/requests

Refers/Fixes

#4695

Testing

I have done a lot of dev testing including the testing done for the video demonstration. As this was simply a case of storing a new field and using it to change the 'to' address of an email, there was not much scope for a new unit test that would achieve a lot.

Video demo

https://drive.google.com/open?id=1-puxbFUQpHwboNNjNI5syHKIo3WWFcOA

@codecov
Copy link

codecov bot commented Sep 25, 2019

Codecov Report

Merging #5250 into master will decrease coverage by <.01%.
The diff coverage is 27.27%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5250      +/-   ##
=========================================
- Coverage    29.9%   29.9%   -0.01%     
=========================================
  Files         231     231              
  Lines       19115   19133      +18     
  Branches     2731    2733       +2     
=========================================
+ Hits         5716    5721       +5     
- Misses      13160   13173      +13     
  Partials      239     239
Impacted Files Coverage Δ
app/marketing/mails.py 14.26% <0%> (-0.04%) ⬇️
app/bounty_requests/models.py 96.29% <100%> (+0.29%) ⬆️
app/bounty_requests/forms.py 53.84% <100%> (+1.84%) ⬆️
app/bounty_requests/views.py 30.43% <14.28%> (-2.9%) ⬇️
app/retail/emails.py 23.29% <16.66%> (-0.08%) ⬇️

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 b32f9f7...6f671f4. Read the comment docs.

@vince0656 vince0656 force-pushed the nothing-found branch 2 times, most recently from 8eaeaf3 to ac000a4 Compare September 25, 2019 20:49
@vince0656 vince0656 marked this pull request as ready for review September 25, 2019 20:57
@vince0656
Copy link
Contributor Author

My understanding is that this meets the following definition of done (from #4695):
-Edit empty state to add a the link as the image
-Create a new field in bounty request "org email"
-fetch the organisation issue email (ex: https://api.github.com/orgs/gitcoinco)
-Send an email to the organisation with the request data (simple email template will be provided for this task) Data to include into the email are:
user profile,
link to the issue,
amount,
comment,
link to create an issue with parameters (this already works like this with issue url and user id: https://gitcoin.co/bounty/new?invite=USERID&source=ISSUEURL )

As well as meeting the following security requirements:
-Be sure to fill the org email in the server side to prevent abuse of spammers
-Be sure to fill userid checking if the user is logged in
-Be sure the url field coming from the form is actually a valid one (github url)

@vince0656
Copy link
Contributor Author

Let me know if you guys spot something that isn't right!

@octavioamu
Copy link
Contributor

Yay! I will take a look @vince0656

@octavioamu
Copy link
Contributor

Testing on my local I get error: "Invalid JSON." seems there is some problem in forms.py
I had created an org here is an issue you can use to test if you need.
whatcomesnext/web#1

@octavioamu
Copy link
Contributor

I believe the problem could be on the js this is the form data

{"github_url":"https://github.com/whatcomesnext/web/issues/1","eth_address":"0xeda95ed3e3436c689376889f9ed0a8f4ba23e866","amount":"30","comment":"I want to work on that","github_org_name":""}: 

@octavioamu
Copy link
Contributor

Update, maybe was cache now is sending that data but seems the problem is with the email.
Sometimes the org can choose not to display or have an email, we need to cover that.
If there is not email from the gh Api just follow the old behaviour (send the email to vivek)
Also if there if everything ok, send to the org email and vivek email.

app/marketing/mails.py Outdated Show resolved Hide resolved
app/bounty_requests/views.py Show resolved Hide resolved
app/marketing/mails.py Outdated Show resolved Hide resolved
app/marketing/mails.py Outdated Show resolved Hide resolved
@octavioamu
Copy link
Contributor

Just that fixes and this will be done, good work!

@thelostone-mc
Copy link
Member

Oh nice :D damn neat

@octavioamu
Copy link
Contributor

@vince0656 how is going?? When do you think we can get this in?

@vince0656
Copy link
Contributor Author

@octavioamu sorry I will look into this tomorrow. Hopefully won't be much longer - thanks for your patience

@vince0656
Copy link
Contributor Author

image

@vince0656
Copy link
Contributor Author

@octavioamu all good to go as per the above screenshot

thelostone-mc
thelostone-mc previously approved these changes Sep 30, 2019
Copy link
Contributor

@octavioamu octavioamu left a comment

Choose a reason for hiding this comment

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

I left some changes and the missing part is apply the header/footer email template, just the generic one. Also I will add a tip to this task since is taking more time than originally planned.
Sounds good?

app/marketing/mails.py Show resolved Hide resolved
@octavioamu
Copy link
Contributor

@vince0656 great work, Just commented on my comments, I was getting "wrong" email just because is overwrite by default to avoid sending emails everywhere.
The only missing piece is apply the email header / footer you can check any of the other emails (maybe invite one)

@octavioamu
Copy link
Contributor

octavioamu commented Sep 30, 2019

Cool just a small thing
can we put the link to gitcoin profiles? <a href="domain/profile/username>@username
and add a margin bottom at the end ?
Just that and we are ready to go 😅

@vince0656
Copy link
Contributor Author

image

@octavioamu how does this look? :)

@vince0656
Copy link
Contributor Author

@octavioamu code pushed

@octavioamu
Copy link
Contributor

Yay, excellent @vince0656 I will ask the other team members to review.

octavioamu
octavioamu previously approved these changes Sep 30, 2019
Copy link
Contributor

@octavioamu octavioamu left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM - just need the migrations squashed

@@ -0,0 +1,18 @@
# Generated by Django 2.1.7 on 2019-09-24 20:57
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you combine these migrations?

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 :)

@vince0656
Copy link
Contributor Author

I know that I've followed the pattern used within the other html files for Gitcoin emails but are you guys aware that emails should really be built exclusively in HTML tables? It ensures the best compatibility across multiple email clients. However, it's quite time consuming to do it that way

danlipert
danlipert previously approved these changes Oct 1, 2019
@danlipert
Copy link
Contributor

Thanks @vince0656 - as far as tables go its probably something we should get to at some point but we haven't received much in the form of complaints

@vince0656
Copy link
Contributor Author

@danlipert fair enough. I'm quite knowledgable so if you guys ever needed assistance, you're more than welcome to get in contact

thelostone-mc
thelostone-mc previously approved these changes Oct 1, 2019
@thelostone-mc
Copy link
Member

@vince0656 mind resolving the conflicts ?

@vince0656 vince0656 dismissed stale reviews from thelostone-mc and danlipert via 6f671f4 October 1, 2019 16:38
@vince0656
Copy link
Contributor Author

@thelostone-mc done😊

@octavioamu octavioamu merged commit 1eab2c4 into gitcoinco:master Oct 1, 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.

4 participants