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 tax management #5072

Merged
merged 31 commits into from
May 22, 2020
Merged

Add tax management #5072

merged 31 commits into from
May 22, 2020

Conversation

PumpkingWok
Copy link
Contributor

@PumpkingWok PumpkingWok commented Aug 26, 2019

Description
  • Added settings/tax, the user can set own location in it, it is prepopulated with the last location registered from ip.
  • Develop management command to create a csv for every funder
  • Create 1099s only for US based users that reach at least 600$ per year.
  • Send email to all funders.
  • Add address field
Refers/Fixes

Fixes: #4806

Testing

I tested it locally using docker to setup the environment, i made the db migration manually.

@PumpkingWok PumpkingWok changed the title frontend/backend tax settings [WIP] Add tax management Aug 26, 2019
@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #5072 into master will decrease coverage by 0.11%.
The diff coverage is 8.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5072      +/-   ##
==========================================
- Coverage   31.03%   30.91%   -0.12%     
==========================================
  Files         218      218              
  Lines       17515    17575      +60     
  Branches     2404     2414      +10     
==========================================
- Hits         5436     5434       -2     
- Misses      11856    11918      +62     
  Partials      223      223
Impacted Files Coverage Δ
app/app/urls.py 89.36% <ø> (ø) ⬆️
app/dashboard/models.py 56.62% <100%> (+0.02%) ⬆️
app/marketing/views.py 10.79% <2.17%> (-1.07%) ⬇️
app/dashboard/views.py 14.17% <23.07%> (+0.01%) ⬆️
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 0e14c7a...2572c6e. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #5072 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5072   +/-   ##
=======================================
  Coverage   26.79%   26.79%           
=======================================
  Files         293      293           
  Lines       27538    27538           
  Branches     4069     4069           
=======================================
  Hits         7380     7380           
  Misses      19890    19890           
  Partials      268      268           

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 1f4e302...1f4e302. Read the comment docs.

@PumpkingWok
Copy link
Contributor Author

Hi @thelostone-mc @octavioamu,

I'm concluding this issue, right now i'm testing the env using docker and i have imported bounty from rinkeby but I would like to ask you how i can import to db some users profile info. I did it some months ago but unfortunately i did not remember how. For testing it properly in theory i should retrieve all user profile and bounties, tips, grants data from mainnet.

I'm waiting to terminate the command send_tax_report.py because i have to test tips and grants but i could not retrieve them from bounty data on rinkeby.
Thanks in advance.

csv_record = []
us_workers = {}
# Bounties
for b in p.get_sent_bounties:
Copy link
Contributor

Choose a reason for hiding this comment

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

are we still planning on using Earnings instead?!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi kevin, if you want i can change it into the code without problems but it requires a little more time to make some tests. I could do it during next week.
One question: In Earning the source_id field correspond to id in BountyFulfillment, for instance in bounty case ? Because i try to compare Earning source_id -> BountyFulfillment bounty_id -> Bounty id and for instance the value_usd does not seem to match between earning and bounty. Could you check it ? thanks

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

scripts/crontab Outdated
@@ -67,6 +67,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/us
#0 0 1 */3 * cd gitcoin/coin; bash scripts/run_management_command_if_not_already_running.bash send_quarterly_stats --live >> /var/log/gitcoin/send_quarterly_stats.log 2>&1
00 10 * * 5 cd gitcoin/coin; bash scripts/run_management_command_if_not_already_running.bash post_leaderboard_to_slack >> /var/log/gitcoin/post_leaderboard_to_slack.log 2>&1
0 9 * * * cd gitcoin/coin; bash scripts/run_management_command_if_not_already_running.bash no_applicants_email >> /var/log/gitcoin/no_applicants_email.log 2>&1
0 10 15 1 * cd gitcoin/coin; bash scripts/run_management_command_if_not_already_running.bash send_tax_report >> /var/log/gitcoin/send_tax_report.log 2>&1
Copy link
Contributor

Choose a reason for hiding this comment

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

id prefer to just send this manually instead of via cron

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok no problem, I'm going to delete it.

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

@owocki
Copy link
Contributor

owocki commented Dec 16, 2019

just did a pass ... code is looking good so far!

@owocki
Copy link
Contributor

owocki commented Dec 18, 2019 via email

@PumpkingWok
Copy link
Contributor Author

Hi @owocki,
Yes i understood it, my question was little different.
In the case of bounty for instance, i followed in Earning the source_id (bounty fulfillment id) to the corresponding bountyfulfillment record, in this record there is bounty_id field, then i looked the record in bounty model for this bounty_id and the value_usd does not seem to match. Maybe i have to update the usd price with some commands.
Can you confirm that from Earning i can retrieve the correct bounty info ?
Thanks in advance.

@owocki
Copy link
Contributor

owocki commented Dec 19, 2019 via email

@PumpkingWok
Copy link
Contributor Author

Hi @owocki ,
I have just committed the send_tax_report command refactor for retrieve all info from Earning. I left rinkeby as network.

Question: if a US worker has set only the location via UI and not the address, right now the command sets the address as Location not found inside 1099 form.
Do you prefer that it leaves the field empty ? or maybe it does not create any 1099 at all if the worker did not set both location and address fields via UI tax setting.
Thanks

@PumpkingWok
Copy link
Contributor Author

Hi @owocki,
If you want you can start to test it and after then i can make edit in the code without problems if there is anything that you would like to change.

You can test directly the command send_tax_report to create for each funder a csv file (bounties, tips and grants info) and a 1099s for each us worker that earned more than 600$ from him/her. Lastly it sends an email to all funders with a zip file attached.
Also via UI you can find a new Tax section in Manage Settings for set both address and location.

From comment above:
Question: if a US worker has set only the location via UI and not the address, right now the command sets the address as Location not found inside 1099 form.
Do you prefer that it leaves the field empty ? or maybe it does not create any 1099 at all if the worker did not set both location and address fields via UI tax setting.
I left rinkeby as network inside send_tax_report code, you should change into mainnet.
Thanks.

@owocki
Copy link
Contributor

owocki commented Dec 31, 2019

Do you prefer that it leaves the field empty ? or maybe it does not create any 1099 at all if the worker did not set both location and address fields via UI tax setting.

yeah sure leave it empty

testing now

@owocki
Copy link
Contributor

owocki commented Dec 31, 2019

i get an exception when testing the mgmt command

root@7748ecbafc92:/code/app# ./manage.py send_tax_report
Traceback (most recent call last):
  File "./manage.py", line 59, in <module>
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python3.6/dist-packages/django/core/management/__init__.py", line 381, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.6/dist-packages/django/core/management/__init__.py", line 375, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.6/dist-packages/django/core/management/base.py", line 323, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.6/dist-packages/django/core/management/base.py", line 364, in execute
    output = self.handle(*args, **options)
  File "/code/app/marketing/management/commands/send_tax_report.py", line 338, in handle
    zip_file_path = zip_dir(p.username, username_path)
  File "/code/app/marketing/management/commands/send_tax_report.py", line 236, in zip_dir
    zipf.write(abs_path, abs_path.split('code/tax_report')[1])
IndexError: list index out of range


if request.POST:

if 'preferred_payout_address' in request.POST.keys():
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need all of these other settings inside of the tax settings update method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @owocki , I left them in this method because i see them in many other setting methods. I can remove this code part without problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think remove them. if its copy/pasted elsewhere then thats something that should be removed in another PR

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

<script>
var csrftoken = jQuery("[name=csrfmiddlewaretoken]").val();
</script>
<script async defer src="https://maps.googleapis.com/maps/api/js?key=AIzaSyBaJ6gEXMqjw0Y7d5Ps9VvelzOOvfV6BvQ&libraries=places&callback=initPlacecomplete"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

might make sense to move the API KEY somewhere else to DRY it??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, i just copied the approach used in job.html. how would you like to do it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the api key on backend view. do you agree or you mean something different. sorry if i did not understand it.

@owocki
Copy link
Contributor

owocki commented Dec 31, 2019

just read through the code; excited for this :)

@PumpkingWok
Copy link
Contributor Author

Hi @owocki ,
for the issue regard list index out of range, i assumed that the root path is code, testing it via docker. I'm going to fix it in these days. Sorry.

@PumpkingWok
Copy link
Contributor Author

Hi @owocki ,
I just committed and forced the rebase because i thought that there were some migrations to do (travis-ci failed) but even after the rebase the test failed again.I checked and it seems to failed in another command, test_assemble_leaderboards.py.

@frankchen07
Copy link
Contributor

@PumpkingWok - thanks for the work, I've paid you out! Let me know if you got it.

@PumpkingWok
Copy link
Contributor Author

Hi there,
Thanks a lot for everything, I got the payment.

I'm so sorry if it took so much time, it needs to be tested and maybe there could be very few fixes to do. If it is too late for this year, to calculate tax, we can keep in contact and i could refactor it for the next year, during this year. let me know.

Thank you very much in any case. have a nice weekend !!!

@thelostone-mc
Copy link
Member

@frankchen07 / @owocki Do we wanna get this in now or would it make sense to get a new PR setup with all the changes (if any needed) when it's needed ?

@owocki
Copy link
Contributor

owocki commented Apr 21, 2020 via email

@PumpkingWok
Copy link
Contributor Author

Hi there, @frankchen07 already tipped me via gitcoin.
If you need help to migrate it for manage next year taxes, feel free to contact me without problems, thanks in advance.

@thelostone-mc
Copy link
Member

@PumpkingWok I've resolved the conflcts but could you

  • recreate the migration file (to avoid squash migrations)
  • it seems like you've added in a bunch of pdf files under misc_1099_templates
    Are those needed ?

@PumpkingWok
Copy link
Contributor Author

Hi @thelostone-mc,
I'm so sorry for the late reply.

  1. I can look in it during this week
  2. In misc_1099_templates there are 4 templates, copy_1, copy_2, copy_b, copy_c.
    All of them will be filled with the same data. I talked with @owocki while i was developing this command, and i understood it, correct me if i'm wrong.
    I retrieved the templates from https://www.irs.gov/pub/irs-pdf/f1099msc.pdf (copy_a was provided in form only for informational purpose)

Also you could read more details about this issue in #4806
For any doubt, please feel free to contact me. Thanks in advance.

@thelostone-mc
Copy link
Member

thelostone-mc commented Apr 28, 2020

Ah gotcha ! Thanks for clarifying it
If you could recreate the migration file + fix conflicts
I can get this in :)

@thelostone-mc
Copy link
Member

thelostone-mc commented Apr 29, 2020

@PumpkingWok could we try & get this in next week ? ^_^

@PumpkingWok
Copy link
Contributor Author

Hi @thelostone-mc,
Do you mean if i can resolve the conflicts here with a rebase, as I always did while i was working on this PR ? Also i would recreate a new migration file for db. Thanks

@thelostone-mc
Copy link
Member

@PumpkingWok yup! I'll add this back to review for wed deploy!
Would be stoked if you could resolve the conflicts + recreate migration file by then

@PumpkingWok
Copy link
Contributor Author

Hi there,
@thelostone-mc, I could fix it tomorrow, during the day. Is it ok for you ?
Thanks in advance.

@PumpkingWok
Copy link
Contributor Author

Hi @thelostone-mc,
ok, i have done it, i resolved the migration conflicts two times because i did not notice that you commited new migration files during the day.
For any doubt feel free to contact me without problems.
If you prefer, at the end, i can squash all commits into one. Thanks in advance.

@thelostone-mc
Copy link
Member

@PumpkingWok nah i'll squash it at my end !
We're just rolling out some big changes this week so once those are stable -> I'll get this in

Appreciate it :)

@thelostone-mc thelostone-mc merged commit 2578544 into gitcoinco:master May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants