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.

Sign up for GitHub

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

Jump to bottom

Displays all tokens in user profile #4597

Merged
merged 2 commits into from
Jun 22, 2019
Merged

Displays all tokens in user profile #4597

merged 2 commits into from
Jun 22, 2019

Conversation

rafalkowalski
Copy link
Contributor

@rafalkowalski rafalkowalski commented Jun 8, 2019 •
edited
Loading

Description

Adds all tokens to users profile

Refers/Fixes

#4590

Testing

Tests cover the code changes

Reviewers
@danlipert @thelostone-mc

@rafalkowalski rafalkowalski force-pushed the I4590/add-missing-crypto-in-profile-page branch from 04dae4f to b091b2c Compare June 8, 2019 21:28
@codecov
Copy link

codecov bot commented Jun 8, 2019

Codecov Report

Merging #4597 into master will decrease coverage by 0.06%.
The diff coverage is 5.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4597      +/-   ##
==========================================
- Coverage   30.12%   30.06%   -0.07%     
==========================================
  Files         210      210              
  Lines       16911    16930      +19     
  Branches     2284     2290       +6     
==========================================
- Hits         5095     5090       -5     
- Misses      11619    11643      +24     
  Partials      197      197
Impacted Files Coverage Δ
app/dashboard/models.py 56.02% <5.26%> (-0.61%) ⬇️
app/dashboard/embed.py 28.16% <0%> (-3.45%) ⬇️

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 b17efb5...b091b2c. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 8, 2019 •
edited
Loading

Codecov Report

Merging #4597 into master will decrease coverage by 0.02%.
The diff coverage is 5.26%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4597      +/-   ##
=========================================
- Coverage   30.33%   30.3%   -0.03%     
=========================================
  Files         214     214              
  Lines       17122   17141      +19     
  Branches     2313    2319       +6     
=========================================
+ Hits         5194    5195       +1     
- Misses      11721   11739      +18     
  Partials      207     207
Impacted Files Coverage Δ
app/dashboard/models.py 56.25% <5.26%> (-0.6%) ⬇️

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 653d229...cf8c5ef. Read the comment docs.

@rafalkowalski rafalkowalski force-pushed the I4590/add-missing-crypto-in-profile-page branch from b091b2c to 22120d9 Compare June 8, 2019 21:49
@PixelantDesign PixelantDesign requested a review from danlipert June 9, 2019 13:40
thelostone-mc
thelostone-mc previously approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

codewise lgtm

@PixelantDesign
Copy link
Contributor

lets merge? @thelostone-mc

@octavioamu octavioamu requested a review from a team June 11, 2019 18:56
@thelostone-mc
Copy link
Member

@PixelantDesign -> we need approval from more than 1 reviewer before merging in PR

danlipert
danlipert suggested changes Jun 12, 2019
View reviewed changes
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.

Looks nice! Just a few fixes and questions

app/dashboard/models.py Outdated Show resolved Hide resolved
app/dashboard/models.py Show resolved Hide resolved
app/dashboard/templates/profiles/profile.html Outdated Show resolved Hide resolved
app/dashboard/tests/test_dashboard_models.py Outdated Show resolved Hide resolved
app/dashboard/tests/test_dashboard_models.py Show resolved Hide resolved
@rafalkowalski rafalkowalski force-pushed the I4590/add-missing-crypto-in-profile-page branch from 22120d9 to a874c52 Compare June 12, 2019 19:46
@thelostone-mc thelostone-mc requested a review from danlipert June 14, 2019 15:50
thelostone-mc
thelostone-mc previously approved these changes Jun 14, 2019
View reviewed changes
danlipert
danlipert previously approved these changes Jun 19, 2019
View reviewed changes
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.

Thanks, looks great! @thelostone-mc @octavioamu please review 👍

@danlipert
Copy link
Contributor

@rafalkowalski can you squash and rebase on master? Thanks!

@thelostone-mc thelostone-mc requested a review from octavioamu June 19, 2019 12:09
@rafalkowalski rafalkowalski force-pushed the I4590/add-missing-crypto-in-profile-page branch from f2f84d7 to 5022413 Compare June 19, 2019 12:13
@rafalkowalski rafalkowalski force-pushed the I4590/add-missing-crypto-in-profile-page branch from 5022413 to 4dae3a9 Compare June 19, 2019 12:17
@rafalkowalski
Copy link
Contributor Author

@danlipert I've rebased code with master

danlipert
danlipert previously approved these changes Jun 19, 2019
View reviewed changes
thelostone-mc
thelostone-mc previously approved these changes Jun 19, 2019
View reviewed changes
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

yay

@danlipert
Copy link
Contributor

@octavioamu please review, thanks!

octavioamu
octavioamu suggested changes Jun 21, 2019
View reviewed changes
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.

Left a comment about not closing html tag. After that LGTM

app/dashboard/templates/profiles/profile.html Outdated Show resolved Hide resolved
app/dashboard/templates/profiles/profile.html Show resolved Hide resolved
@rafalkowalski rafalkowalski dismissed stale reviews from thelostone-mc and danlipert via 4f4917c June 21, 2019 15:57
@rafalkowalski rafalkowalski force-pushed the I4590/add-missing-crypto-in-profile-page branch from 4dae3a9 to 4f4917c Compare June 21, 2019 15:57
octavioamu
octavioamu approved these changes Jun 21, 2019
View reviewed changes
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.

Great, LGTM

@thelostone-mc thelostone-mc merged commit 61393af into gitcoinco:master Jun 22, 2019
@gitcoinbot gitcoinbot mentioned this pull request Jun 24, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thelostone-mc thelostone-mc thelostone-mc approved these changes

@octavioamu octavioamu octavioamu approved these changes

@danlipert danlipert danlipert left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rafalkowalski @PixelantDesign @thelostone-mc @danlipert @owocki @octavioamu