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 button to sync Github issue #4899

Merged
merged 6 commits into from
Aug 7, 2019

Conversation

PierrickGT
Copy link
Contributor

Description

As a bounty owner, I wish to be able to update my bounty description by syncing from Github.
This PR adds a Sync Issue button that allows a bounty owner to easily update the bounty description.

JmtsK51Wao

Refers/Fixes

Issue: #4870

@codecov
Copy link

codecov bot commented Jul 30, 2019

Codecov Report

Merging #4899 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4899      +/-   ##
==========================================
- Coverage   30.82%   30.82%   -0.01%     
==========================================
  Files         216      216              
  Lines       17387    17388       +1     
  Branches     2374     2375       +1     
==========================================
  Hits         5359     5359              
- Misses      11811    11812       +1     
  Partials      217      217
Impacted Files Coverage Δ
app/dashboard/views.py 14.2% <0%> (-0.01%) ⬇️

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 456e64a...d8e37f7. Read the comment docs.

danlipert
danlipert previously approved these changes Jul 30, 2019
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.

Sweet! Looks great - lots of people will be stoked for this!

@PierrickGT
Copy link
Contributor Author

Sweet! Looks great - lots of people will be stoked for this!

Thank you! I will submit my work tonight once I get home.

octavioamu
octavioamu previously approved these changes Jul 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.

Great

@PierrickGT
Copy link
Contributor Author

Sweet! Looks great - lots of people will be stoked for this!

Thank you! I will submit my work tonight once I get home.

I'm gonna need @thelostone-mc to approve me as a worker cause I can't submit my work for now.

}
).fail(
function(result) {
var alertMsg = result && result.responseJSON ? result.responseJSON.error : null;
Copy link
Member

Choose a reason for hiding this comment

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

could we update this to let ?

function(result) {
var alertMsg = result && result.responseJSON ? result.responseJSON.error : null;

if (alertMsg === null) {
Copy link
Member

Choose a reason for hiding this comment

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

thoughts on making it as

let alertMsg = result && result.responseJSON ? result.responseJSON.error : gettext('Failed to sync issue. Please reload the page and try again.');

}
);
}).fail(function(result) {
var alertMsg = result && result.responseJSON ? result.responseJSON.error : null;
Copy link
Member

Choose a reason for hiding this comment

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

^ let

}).fail(function(result) {
var alertMsg = result && result.responseJSON ? result.responseJSON.error : null;

if (alertMsg === null) {
Copy link
Member

Choose a reason for hiding this comment

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

let alertMsg = result && result.responseJSON ? result.responseJSON.error : gettext('Failed to sync issue. Please reload the page and try again.');

thelostone-mc
thelostone-mc approved these changes Jul 31, 2019
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.

Left a few minor comments !
Otherwise damn neat 🙌

Ah I'm not sure what happened. I did approve it after we synced up on how you'd tackle this.
Nevertheless -> approved now

@PierrickGT PierrickGT dismissed stale reviews from octavioamu and danlipert via af7d995 July 31, 2019 09:07
@PierrickGT
Copy link
Contributor Author

@thelostone-mc I've updated the alertMsg syntax: af7d995
Let me know what you think.

@octavioamu octavioamu merged commit e786817 into gitcoinco:master Aug 7, 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