Fix edit info on grant page #4793

PierrickGT · 2019-07-13T14:58:50Z

Description

As a grant owner, I wish to be able to edit infos of my grant.
This PR fix a bug who made it impossible to edit infos of a grant if you where not doing it from the first tab, it was due to the Quill editor not being conditioned to be loaded only on the first tab.

Screencast

Here is a recording of the fix

Refers/Fixes

Bounty: https://gitcoin.co/issue/gitcoinco/web/4786/3227
Issue: #4786

codecov · 2019-07-13T15:25:07Z

Codecov Report

Merging #4793 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #4793      +/-   ##
=========================================
- Coverage   30.63%   30.6%   -0.04%     
=========================================
  Files         216     216              
  Lines       17488   17489       +1     
  Branches     2385    2386       +1     
=========================================
- Hits         5358    5352       -6     
- Misses      11915   11922       +7     
  Partials      215     215

Impacted Files	Coverage Δ
app/grants/views.py	`13.85% <0%> (-0.04%)`	⬇️
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 7e2290a...60f8772. Read the comment docs.

codecov · 2019-07-13T15:25:07Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4793      +/-   ##
==========================================
- Coverage   30.41%   30.41%   -0.01%     
==========================================
  Files         216      216              
  Lines       17247    17248       +1     
  Branches     2340     2341       +1     
==========================================
  Hits         5246     5246              
- Misses      11793    11794       +1     
  Partials      208      208

Impacted Files	Coverage Δ
app/grants/views.py	`13.85% <0%> (-0.04%)`	⬇️

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 7e23f9f...f9b5a58. Read the comment docs.

thelostone-mc

Could you add a recording of the fix + update the GitHub description so other folks know exactly what is done before they check out the code ?

Code looks alright ! Will pull it in and check it out locally ^_^

PierrickGT · 2019-07-17T12:38:30Z

Could you add a recording of the fix + update the GitHub description so other folks know exactly what is done before they check out the code ?

Code looks alright ! Will pull it in and check it out locally ^_^

I've added a better description of the issue and a gif recording of the fix.

app/assets/v2/js/grants/detail.js

PierrickGT · 2019-07-20T11:29:11Z

Thanks for accepting my PR @thelostone-mc !
Do you know how I could fix the coverage errors?

thelostone-mc · 2019-07-22T10:41:28Z

@PierrickGT Ah i guess that's alright for now! we are yet to wire in a testing library for frontend

PierrickGT · 2019-07-22T11:30:44Z

@thelostone-mc ok thanks! Waiting for my work to be approved on Gitcoin before sending this PR and then merging it.

thelostone-mc · 2019-07-22T18:57:11Z

@PierrickGT you should be able to apply for work on it ^_^
Could you click on start work and I'll go ahead and approve you

thelostone-mc · 2019-07-24T14:12:18Z

@PierrickGT looks like we have CI failures :| could you resolve that ?

PierrickGT · 2019-07-24T15:28:28Z

@PierrickGT looks like we have CI failures :| could you resolve that ?

I've fixed it, there was a formatting error introduced by merging master.

danlipert

Hey @PierrickGT - looks like there were a lot of unrelated style changes in your PR here, can you revert those so its easier to review? Generally we keep the changes limited to the scope of the issue. Thanks!

PierrickGT · 2019-07-30T13:27:36Z

Hey @PierrickGT - looks like there were a lot of unrelated style changes in your PR here, can you revert those so its easier to review? Generally we keep the changes limited to the scope of the issue. Thanks!

I've fixed it, except for some indentations that were not right: 162ee56
You should definitely use Prettier or even eslint in order to have a more readable JS codebase.

thelostone-mc

@PierrickGT we've got precommit hooks which run eslint + stylelint.
It's documented in our Contributing.md file

Did you check that out ?

PierrickGT · 2019-07-31T09:08:46Z

@PierrickGT we've got precommit hooks which run eslint + stylelint.
It's documented in our Contributing.md file

Did you check that out ?

Oh I didn't know about it. I've set it up so we'll see how it works out for the future commits.

thelostone-mc · 2019-07-31T13:56:13Z

app/assets/v2/js/grants/detail.js

+            error: function() {
+              _alert({ message: gettext('Changing the contract owner address failed to save. Please try again.') }, 'error');
+            }
+          });


@PierrickGT could you undo the linting you've done here ?
It makes it confusing for reviewers to see what exactly your changes are

I've fixed it: 3a57279

This reverts commit 90bb0ff.

danlipert · 2019-07-31T15:37:23Z

Hey @PierrickGT - I just tested this before our production deploy and its broken - the quill library is not loading correctly when I view a grant somebody else created - see screenshot:

please fix and resubmit

This reverts commit 90bb0ff.

PierrickGT · 2019-07-31T18:06:23Z

Hey @PierrickGT - I just tested this before our production deploy and its broken - the quill library is not loading correctly when I view a grant somebody else created - see screenshot:

please fix and resubmit

My bad, I'm gonna take a look at it and fix it.

PierrickGT · 2019-07-31T18:57:21Z

I've reopened a PR where I've fixed the condition to initialize Quill in the file app/grants/templates/grants/detail/tabs.html, I've forgot about the case where you don't have ?tab=description in the url.

#4928

octavioamu · 2019-07-31T20:04:36Z

@PierrickGT is that PR with this PR changes also? because we had reverted this merge since was failing in our deploy tests.

PierrickGT · 2019-07-31T20:05:20Z

@PierrickGT is that PR with this PR changes also? because we had reverted this merge since was failing in our deploy tests.

Yes, it's the same PR but with the fix.
Here is the commit that fix it: 0f524dd

Fix edit info on grant page

2278d90

PierrickGT requested review from danlipert, octavioamu and thelostone-mc as code owners July 13, 2019 14:58

PierrickGT mentioned this pull request Jul 13, 2019

"Edit info" button on Grant page isn't working #4786

Closed

Fix grant data eslint error

f9b5a58

Fix grant/detail.js lint

dfa2d6d

thelostone-mc requested changes Jul 17, 2019

View reviewed changes

thelostone-mc reviewed Jul 17, 2019

View reviewed changes

app/assets/v2/js/grants/detail.js Outdated Show resolved Hide resolved

Fix grant detail.js code formatting

a5a1900

thelostone-mc previously approved these changes Jul 20, 2019

View reviewed changes

PierrickGT added 2 commits July 22, 2019 23:49

Merge branch 'master' into fixes/grant-edit_info

aa4d0d9

Merge branch 'master' into fixes/grant-edit_info

903ee67

PierrickGT dismissed thelostone-mc’s stale review via 903ee67 July 24, 2019 11:34

Merge branch 'master' into fixes/grant-edit_info

d04d022

PierrickGT added 3 commits July 24, 2019 16:38

Merge branch 'master' into fixes/grant-edit_info

178cc12

Fix build error on grant detail.js

bfa2cb7

Merge branch 'master' into fixes/grant-edit_info

861f135

Merge branch 'master' into fixes/grant-edit_info

74326d2

thelostone-mc previously approved these changes Jul 24, 2019

View reviewed changes

danlipert suggested changes Jul 30, 2019

View reviewed changes

PierrickGT dismissed thelostone-mc’s stale review via 8fa9e6b July 30, 2019 13:21

Fix lint formatting

162ee56

PierrickGT force-pushed the fixes/grant-edit_info branch from 8fa9e6b to 162ee56 Compare July 30, 2019 13:25

Merge branch 'master' into fixes/grant-edit_info

cb76e6d

thelostone-mc previously approved these changes Jul 31, 2019

View reviewed changes

thelostone-mc requested a review from danlipert July 31, 2019 05:11

thelostone-mc reviewed Jul 31, 2019

View reviewed changes

Fix contract_owner_button onClick indentation

3a57279

PierrickGT dismissed thelostone-mc’s stale review via 3a57279 July 31, 2019 14:04

octavioamu approved these changes Jul 31, 2019

View reviewed changes

thelostone-mc approved these changes Jul 31, 2019

View reviewed changes

danlipert approved these changes Jul 31, 2019

View reviewed changes

Merge branch 'master' into fixes/grant-edit_info

60f8772

danlipert merged commit 90bb0ff into gitcoinco:master Jul 31, 2019

PierrickGT deleted the fixes/grant-edit_info branch July 31, 2019 14:37

danlipert added a commit that referenced this pull request Jul 31, 2019

Revert "Fix edit info on grant page (#4793)"

7c097b8

This reverts commit 90bb0ff.

danlipert mentioned this pull request Jul 31, 2019

Revert "Fix edit info on grant page" #4921

Merged

danlipert added a commit that referenced this pull request Jul 31, 2019

Revert "Fix edit info on grant page (#4793)" (#4921)

3cdc50e

This reverts commit 90bb0ff.

PierrickGT restored the fixes/grant-edit_info branch July 31, 2019 18:07

danlipert mentioned this pull request Aug 7, 2019

Fixes/grant edit info #4975

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix edit info on grant page #4793

Fix edit info on grant page #4793

PierrickGT commented Jul 13, 2019 •

edited

Loading

codecov bot commented Jul 13, 2019 •

edited

Loading

codecov bot commented Jul 13, 2019

thelostone-mc left a comment

PierrickGT commented Jul 17, 2019

PierrickGT commented Jul 20, 2019 •

edited

Loading

thelostone-mc commented Jul 22, 2019

PierrickGT commented Jul 22, 2019

thelostone-mc commented Jul 22, 2019

thelostone-mc commented Jul 24, 2019

PierrickGT commented Jul 24, 2019

danlipert left a comment

PierrickGT commented Jul 30, 2019

thelostone-mc left a comment

PierrickGT commented Jul 31, 2019

thelostone-mc Jul 31, 2019

PierrickGT Jul 31, 2019

danlipert commented Jul 31, 2019

PierrickGT commented Jul 31, 2019

PierrickGT commented Jul 31, 2019

octavioamu commented Jul 31, 2019

PierrickGT commented Jul 31, 2019 •

edited

Loading

Fix edit info on grant page #4793

Fix edit info on grant page #4793

Conversation

PierrickGT commented Jul 13, 2019 • edited Loading

Description

Screencast

Refers/Fixes

codecov bot commented Jul 13, 2019 • edited Loading

Codecov Report

codecov bot commented Jul 13, 2019

Codecov Report

thelostone-mc left a comment

Choose a reason for hiding this comment

PierrickGT commented Jul 17, 2019

PierrickGT commented Jul 20, 2019 • edited Loading

thelostone-mc commented Jul 22, 2019

PierrickGT commented Jul 22, 2019

thelostone-mc commented Jul 22, 2019

thelostone-mc commented Jul 24, 2019

PierrickGT commented Jul 24, 2019

danlipert left a comment

Choose a reason for hiding this comment

PierrickGT commented Jul 30, 2019

thelostone-mc left a comment

Choose a reason for hiding this comment

PierrickGT commented Jul 31, 2019

thelostone-mc Jul 31, 2019

Choose a reason for hiding this comment

PierrickGT Jul 31, 2019

Choose a reason for hiding this comment

danlipert commented Jul 31, 2019

PierrickGT commented Jul 31, 2019

PierrickGT commented Jul 31, 2019

octavioamu commented Jul 31, 2019

PierrickGT commented Jul 31, 2019 • edited Loading

PierrickGT commented Jul 13, 2019 •

edited

Loading

codecov bot commented Jul 13, 2019 •

edited

Loading

PierrickGT commented Jul 20, 2019 •

edited

Loading

PierrickGT commented Jul 31, 2019 •

edited

Loading