Adding overage alert to advanced payout. #2386

Anish-Agnihotri · 2018-10-09T02:16:24Z

Description

This PR resolves #2294 by introducing:

Alerts upon advanced payment > 100%.
Other misc. payout preview cleaning (removing refunded amount in payout preview).

A lot of the changes that were made in the last PR were addressed by @thelostone-mc in his PR introducing user profiles. Thus, code that I had previously written for that purpose (removing users, bounty stake, etc.) has been removed from this revised PR.

Checklist

Add an alert (bg color #E66700, #FFF for copy) when user exceeds 100% or Bountied value in ETH.
Add wallet information and overage amount to the alert.
Display the overage amount on the payout preview in #E66700
Remove refunded Eth
This issue is tested

Affected core subsystem(s)

UI is the only core system affected, since all changes are aesthetic.

Testing

Tested across two environments across two platforms (W10, Mojave). Feel free to test yourself as well.

Currently, this PR is not functional since the Payout Preview paid and net values are broken (cc @thelostone-mc). Once that is functioning again this PR will be fully functional.

Refers/Fixes

Fixes #2294.

Anish-Agnihotri · 2018-10-09T02:17:40Z

CC @pinkiebell, @thelostone-mc, @mbeacom, @SaptakS, @PixelantDesign for re-review.

Thanks :).

codecov · 2018-10-09T02:27:53Z

Codecov Report

Merging #2386 into master will decrease coverage by 0.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2386      +/-   ##
==========================================
- Coverage   29.29%   29.08%   -0.22%     
==========================================
  Files         147      148       +1     
  Lines       11852    11928      +76     
  Branches     1602     1616      +14     
==========================================
- Hits         3472     3469       -3     
- Misses       8262     8341      +79     
  Partials      118      118

Impacted Files	Coverage Δ
app/bounty_requests/views.py	`33.33% <0%> (-5.38%)`	⬇️
app/dashboard/embed.py	`28.16% <0%> (-3.45%)`	⬇️
app/avatar/models.py	`26.31% <0%> (-0.47%)`	⬇️
app/retail/emails.py	`19.76% <0%> (-0.36%)`	⬇️
app/marketing/utils.py	`31.2% <0%> (-0.26%)`	⬇️
app/retail/views.py	`29.88% <0%> (-0.12%)`	⬇️
app/dashboard/models.py	`52.78% <0%> (-0.01%)`	⬇️
...keting/management/commands/send_quarterly_stats.py	`0% <0%> (ø)`	⬆️
...d/management/commands/post_leaderboard_to_slack.py	`0% <0%> (ø)`	⬆️
...board/management/commands/cleanup_dupe_profiles.py	`0% <0%> (ø)`
... and 2 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 ca18edd...adf13df. Read the comment docs.

app/assets/v2/js/pages/bulk_payout.js

thelostone-mc

Doesn't seem to be working from my end 😓

Anish-Agnihotri · 2018-10-09T15:57:39Z

Just pushed out fix. Codecov error is not due to this edit.

Let me know if it's good to go :).

app/dashboard/templates/bulk_payout_bounty.html

app/assets/v2/js/pages/bulk_payout.js

Anish-Agnihotri · 2018-10-11T15:57:44Z

@thelostone-mc @SaptakS

Feel free to re-review when you have time! Thanks :).

app/assets/v2/js/pages/bulk_payout.js

Anish-Agnihotri · 2018-10-12T17:33:18Z

@thelostone-mc

Although I've restructured the commits to include tbody, I'm not really happy with where it went. nth-child() is a function that doesn't follow the usual [0, 1, 2, 3] scheme but rather [1, 2, 3, 4].

Because of that, simply changing the i variable to 0 would not work, since nth-child(0) would not work. But, if I made set i = 1, although nth-child(1) would work, it would break the function since i < num_rows would no longer be valid.

The only way to fix this issue was to append +1 to the num_rows variable, which would then allow you to use i = 1 and receive nth-child(1), nth-child(2), and so on (since in this case num_rows would always be one more than i).

Of course, this might actually be a more complicated solution than simply not using tbody in the first place.

I hope @SaptakS or you have some better idea at how to combat this, since this doesn't seem to be the easiest way to do this. Possibly my JS is just all over the place?

thelostone-mc · 2018-10-13T06:33:22Z

Ah 😓 meh ! I'm good with what we have if that's the case

nth-child() is a function that doesn't follow the usual [0, 1, 2, 3] scheme but rather [1, 2, 3, 4].

did now know about that 😅

SaptakS · 2018-10-13T06:57:32Z

@Anish-Agnihotri can't you just do i <= num_rows? That way you don't have to hard code the +1 to the number of rows.
Secondly, I don't think nth-child is even needed. You can get all the rows by var rows = $('#payout_table tbody').find('tr'). Then you can just loop like rows.forEach(function(row) { // code here }); . That way you can completely avoid using nth-child. Using nth-child is over engineering I feel.

Anish-Agnihotri · 2018-10-13T16:40:59Z

Lol, I was completely overthinking myself. Thanks @SaptakS. I've pushed out the new (hopefully ready to merge, now) commit.

I realized that there was a lot of over-engineering and removed the need for num_rows, i, and the nth-child altogether. Now we are just going through a simple function that runs on each instance of #payout_table tbody tr.

Do you think this is a better idea?

SaptakS · 2018-10-13T17:39:30Z

app/assets/v2/js/pages/bulk_payout.js

    var is_error = !$.isNumeric(amount) || amount <= 0 || username == '' || username == '@';

    if (!is_error) {
      total += amount;
    }
-  }
+  });
+
  return total;


@Anish-Agnihotri okay. Sorry but .each() function won't be very helpful here because callback functions will be executed asynchronously, and hence when you return total outside, no guarantee that it will give the total.

Something like this will be good.

var rows = $('#payout_table tbody tr'); for (i = 0; i < rows.length; i += `) { var $rows = $(rows[i]); var amount = parseFloat($row.find('.amount').text()); // and all the other code } return total;

Ah, yes, async execution. Thanks for the reviews, always a good learning experience. Pushing it out according to your recommendation now.

@SaptakS @Anish-Agnihotri Hey guys, chiming in to clarify something.

Sorry but .each() function won't be very helpful here because callback functions will be executed asynchronously,

jQuery each function is synchronous. You can verify this by adding a console.log(total) after the each function call. You can also refer to this SO thread for more information.

pinkiebell

Left some comments 👍

@thelostone-mc
What is the status of the Payout Preview paid and net values mentioned in the description?
(Want to try that out locally ;))

pinkiebell · 2018-10-19T16:40:28Z

app/assets/v2/css/bulk_payout_bounty.css

+  padding-left: 15px;
+}
+
+.overageAlert>div:first-child {


Cosmetic / Space to breath 😄
.overageAlert > div:first-child

pinkiebell · 2018-10-19T16:40:40Z

app/assets/v2/css/bulk_payout_bounty.css

+  padding-right: 10px;
+}
+
+.overageAlert>div:last-child {


pinkiebell · 2018-10-19T16:40:50Z

app/assets/v2/css/bulk_payout_bounty.css

+  padding-top: 6.5px;
+}
+
+.overageAlert>div:first-child>img {


pinkiebell · 2018-10-19T16:44:46Z

app/assets/v2/js/pages/bulk_payout.js

@@ -162,7 +162,7 @@ $(document).ready(function($) {
      <tr>
        <td class="pl-0 pb-0">
          <div class="pl-0">
-            <select id="username" onchange="update_registry()" class="username-search custom-select" style="width: 100%; margin-left: auto; margin-right: auto;"></select>
+            <select onchange="update_registry()" class="username-search custom-select" style="width: 100%; margin-left: auto; margin-right: auto;"></select>


Add w-100 ml-auto mr-auto to the class attribute and we can get rid of the style=... attribute. :)

pinkiebell · 2018-10-19T16:48:48Z

app/assets/v2/js/pages/bulk_payout.js

-    var $row = $('#payout_table tobdy').find('tr:nth-child(' + i + ')');
-    var amount = parseFloat($row.find('.amount').text());
-    var username = $row.find('.username-search').text();
+  for (i = 0; i < rows.length; i += 1) {


Maybe we want to use let / ES5/6 style ? cc @thelostone-mc @SaptakS
Like:

for (let i = 1; i < num_rows; i++) { let row = rows[i]; // XXX: Do we need $(rows[i])?

I think Gitcoin is moving towards ES6 styling if I remember correctly, so I'd be glad to switch these over.

It's okay for now. We will anyways do a complete migration from old-school to ES6 soon.

Anish-Agnihotri · 2018-10-20T02:03:57Z

Thanks @pinkiebell for the review 🙌 ! Just pushed out new commit.

Anish

PixelantDesign · 2018-10-20T19:21:35Z

Thanks @pinkiebell!

thelostone-mc

code looks lgtm ! @SaptakS good for merge ?

SaptakS

LGTM

Anish-Agnihotri · 2018-10-21T16:31:46Z

🎉 Perfect! We should be good for merge then :)

Adding overage alert to advanced payout.

07a04a4

Removing id to avoid id repetition.

2996a15

thelostone-mc reviewed Oct 9, 2018

View reviewed changes

app/assets/v2/js/pages/bulk_payout.js Outdated Show resolved Hide resolved

thelostone-mc requested review from thelostone-mc and owocki and removed request for thelostone-mc October 9, 2018 04:32

thelostone-mc added enhancement This is a feature to be enhanced or improved. frontend This needs frontend expertise. labels Oct 9, 2018

thelostone-mc reviewed Oct 9, 2018

View reviewed changes

Fixing get_total_cost.

6594e32

thelostone-mc reviewed Oct 11, 2018

View reviewed changes

app/dashboard/templates/bulk_payout_bounty.html Outdated Show resolved Hide resolved

SaptakS reviewed Oct 11, 2018

View reviewed changes

app/dashboard/templates/bulk_payout_bounty.html Outdated Show resolved Hide resolved

SaptakS suggested changes Oct 11, 2018

View reviewed changes

app/assets/v2/js/pages/bulk_payout.js Outdated Show resolved Hide resolved

Restructuring advanced payout.

345e5dc

thelostone-mc reviewed Oct 12, 2018

View reviewed changes

app/assets/v2/js/pages/bulk_payout.js Outdated Show resolved Hide resolved

thelostone-mc reviewed Oct 12, 2018

View reviewed changes

app/assets/v2/js/pages/bulk_payout.js Outdated Show resolved Hide resolved

thelostone-mc reviewed Oct 12, 2018

View reviewed changes

app/assets/v2/js/pages/bulk_payout.js Outdated Show resolved Hide resolved

Refactoring for tbody inclusion.

33abb80

Rewriting get_total_cost function.

b3f01e5

SaptakS reviewed Oct 13, 2018

View reviewed changes

Fixing for callback.

4304e8e

mbeacom requested a review from thelostone-mc October 14, 2018 19:50

mbeacom requested a review from SaptakS October 14, 2018 19:50

pinkiebell reviewed Oct 19, 2018

View reviewed changes

Adding changes according to @pinkiebell

adf13df

thelostone-mc reviewed Oct 20, 2018

View reviewed changes

SaptakS approved these changes Oct 21, 2018

View reviewed changes

SaptakS merged commit 317954c into gitcoinco:master Oct 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding overage alert to advanced payout. #2386

Adding overage alert to advanced payout. #2386

Anish-Agnihotri commented Oct 9, 2018

Anish-Agnihotri commented Oct 9, 2018

codecov bot commented Oct 9, 2018 •

edited

Loading

thelostone-mc left a comment

Anish-Agnihotri commented Oct 9, 2018

Anish-Agnihotri commented Oct 11, 2018

Anish-Agnihotri commented Oct 12, 2018

thelostone-mc commented Oct 13, 2018

SaptakS commented Oct 13, 2018

Anish-Agnihotri commented Oct 13, 2018

SaptakS Oct 13, 2018

Anish-Agnihotri Oct 13, 2018

arku Oct 22, 2018

pinkiebell left a comment

pinkiebell Oct 19, 2018

pinkiebell Oct 19, 2018

pinkiebell Oct 19, 2018

pinkiebell Oct 19, 2018

pinkiebell Oct 19, 2018

Anish-Agnihotri Oct 20, 2018

SaptakS Oct 21, 2018

Anish-Agnihotri commented Oct 20, 2018

PixelantDesign commented Oct 20, 2018

thelostone-mc left a comment •

edited

Loading

SaptakS left a comment

Anish-Agnihotri commented Oct 21, 2018

Adding overage alert to advanced payout. #2386

Adding overage alert to advanced payout. #2386

Conversation

Anish-Agnihotri commented Oct 9, 2018

Description

Checklist

Affected core subsystem(s)

Testing

Refers/Fixes

Anish-Agnihotri commented Oct 9, 2018

codecov bot commented Oct 9, 2018 • edited Loading

Codecov Report

thelostone-mc left a comment

Choose a reason for hiding this comment

Anish-Agnihotri commented Oct 9, 2018

Anish-Agnihotri commented Oct 11, 2018

Anish-Agnihotri commented Oct 12, 2018

thelostone-mc commented Oct 13, 2018

SaptakS commented Oct 13, 2018

Anish-Agnihotri commented Oct 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinkiebell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anish-Agnihotri commented Oct 20, 2018

PixelantDesign commented Oct 20, 2018

thelostone-mc left a comment • edited Loading

Choose a reason for hiding this comment

SaptakS left a comment

Choose a reason for hiding this comment

Anish-Agnihotri commented Oct 21, 2018

codecov bot commented Oct 9, 2018 •

edited

Loading

thelostone-mc left a comment •

edited

Loading