Create chamber_of_deputies app

[giovanisleite]

What is the purpose of this Pull Request?
Remove chamber of deputies exclusive stuff from core and put it on a proper app, chamber_of_deputies. it is part of #216

What was done to achieve this purpose?
It was created chamber_of_deputies app. The Reimbursement and Tweet models was moved from core.models to chamber_of_deputies.models. Then, all commands and tests related to these models was moved too. The import paths were corrected.

From #216 description:

• create a new app called chamber_of_deputies

• move jarbas.core.models to jarbas.chamber_of_deputies.models

• change jarbas.core.management.commands to jarbas.chamber_of_deputies.management.commands

• change APIs URLs ⚠️

• split api serializers between chamber_of_deputies.serializers and core.serializers

Who can help reviewing it?
@cuducos and @jtemporal

I'm no sure about this point
The LoadCommands class from core management commands is being reused at chamber_of_deputies app. #265

[cuducos]

That looks really good, @giovanisleite! I haven't had time to test it all but from a first run I'd say it's a great start. By now I'd like to ask you for one specific improvement: from your migration files I see you're actually throwing away existing databases (and data) and building new tables. This causes a loss of data that is unnecessary and undesired (we keep records of historical changes for each reimbursement because the Chamber of Deputies doesn't).

So may I ask you: are you interested in studying and proposing a better way to handle this migration/refactor without loosing data?

[giovanisleite]

So may I ask you: are you interested in studying and proposing a better way to handle this migration/refactor without loosing data?

I had never done this before, so I would appreciate a careful review/feedback/tips/options about these migrations.

I read some contents about it, some of them using South (if someone knows how and the benefits/ if its worth it adding a dependence, says hello). But the Simple is better than complex way of it, I found on this blog and a simpler (and appropriate because we don't have relations between models from different apps and etc) version on this answer (yes, it is the second answer 😮 ).

What I don't know yet is if we need to create again those indexes.

[cuducos]

Many thanks, @giovanisleite — it's indeed a really good blog post from Mikalai, thanks for sharing!

And yep, it looks like we're not loosing data now. Thing are quite hectic this week, gonna try to take a look on the weekend, ok?

[cuducos]

Meanwhile there are some warnings from Codeclimate that you could try to target… adding the new migration directory to the excludes paths at.codeclimate.yml, clean up some lines that are almost blank but still have a space or something etc… what do you think?

I wouldn't be strict (for example, model declarations usually are longer than 80 or even 120 chars and I personally don't care), but there are some room to improvements in IMHO — at leats properly setting up to exclude migration files from the report.

[giovanisleite]

Thank you, @cuducos and good luck =))

adding the new migration directory to the excludes paths at codeclimate.yml, clean up some lines that are almost blank but still have a space or something etc…

Sure, I'll do it =))

[cuducos]

Hi @giovanisleite! Many thanks for this PR.

In general this is a really good contribution and a huge PR. Before merging it I'm afraid I'll need to ask for some changes.

1. Merging Delegate reimbursement import to Celery #240 caused some conflicts in your branch, we need to sor that out. If you need help I fixed conflicts in a separate branch, in this commit
2. In core/app.py I think we need a different verbose_name, don't we? In that sense I think that even if the app is called chamber_of_deputies the verbose name in the Dashboard should mention Cota para Exercício da Atividade Parlamentar
3. Finally and most important, API needs new paths (as suggested, something like /api/chamber_of_deputies/reimbursements/ instead of /api/reimbursements/) and we need to update that in the docs (README.md)

Once more many thanks for handling this quite delicate re-architecturing ; )

[giovanisleite]

Merging #240 caused some conflicts in your branch, we need to sor that out. If you need help I fixed conflicts in a separate branch, in this commit

Thanks, I will look into it.

In core/app.py I think we need a different verbose_name, don't we?

Yes, core contains companies and activities, should we use this name? "Empresas e atividades"?

In that sense I think that even if the app is called chamber_of_deputies the verbose name in the Dashboard should mention Cota para Exercício da Atividade Parlamentar

Yes, I agree. Is "Câmara dos Deputados - Cota para Exercício da Atividade Parmalmentar" ok?

Finally and most important, API needs new paths (as suggested, something like /api/chamber_of_deputies/reimbursements/ instead of /api/reimbursements/) and we need to update that in the docs (README.md)

I created this PR before than I expected because it has become a huge PR. If it is not inconsistent, I would prefer create another PR with the API changes (URLs and Serializers). What do you think?

Thank you for all your help, @cuducos

And....
wooooooooow, why tests are taking three seconds instead of fourty?

[cuducos]

Yes, core contains companies and activities, should we use this name? "Empresas e atividades"?

I'd just named it Jarbas…

Yes, I agree. Is "Câmara dos Deputados - Cota para Exercício da Atividade Parmalmentar" ok?

Yep, I like it ; )

I created this PR before than I expected because it has become a huge PR. If it is not inconsistent, I would prefer create another PR with the API changes (URLs and Serializers). What do you think?

Ok, but can we have an issue so we don't forget about it? You can open an issue saying if #265 goes through we'll need to change the API endpoints etc. etc. etc.. However IMHO this should be done in this very PR. I mean… this is my opinion, but I really don't mind if you let it to another PR. I'll wait for your decision before code reviewing and this this PR, ok? LMK

wooooooooow, why tests are taking three seconds instead of fourty?

On Travis? Maybe because #245 removed Elm tests…

[giovanisleite]

Yes, I agree. Is "Câmara dos Deputados - Cota para Exercício da Atividade Parmalmentar" ok?
Yep, I like it ; )

Done 48aeac8

Ok, but can we have an issue so we don't forget about it? You can open an issue saying if #265 goes through we'll need to change the API endpoints etc. etc. etc.. However IMHO this should be done in this very PR. I mean… this is my opinion, but I really don't mind if you let it to another PR. I'll wait for your decision before code reviewing and this this PR, ok? LMK

I changed the api endpoints in 51922a0. I'm not sure about doing the other changes in api app that we had discussed in #216 (relocate serializers to each app) in this PR, but I can do it and edit the description.

On Travis? Maybe because #245 removed Elm tests…

Local, Elm tests ran with ... manage.py test?

[cuducos]

The LoadCommands class from core management commands is being reused at chamber_of_deputies app.

There's no problem IMHO. There's a reason why core is named core ; )

I'm not sure about doing the other changes in api app that we had discussed in #216 (relocate serializers to each app) in this PR

IMHO now we have the worst scenario: we started to move the API and stopped halfway there, without even an issue to remind us about the next steps. Either we should have what we had before (don't change the api app` and list that as an issue) or we move things completely…

Local, Elm tests ran with ... manage.py test?

Nope… there's no Elm test anymore (#272).

[giovanisleite]

There's no problem IMHO. There's a reason why core is named core ; )
IMHO now we have the worst scenario: we started to move the API and stopped halfway there, without even an issue to remind us about the next steps. Either we should have what we had before (don't change the api app` and list that as an issue) or we move things completely…

2x Fair enough 😄
Done e7bf9fb

Maybe api.views is too long and is not the best choice concentrate all those classes there. So, I want your opinion about these options:

1. Divide api.views.py in api.views.chamber_of_deputies.py, api.views.core.py and a future api.views.federal_senate.py? - Just organize
2. Remove api app, then core.urls or even jarbas.urls points api/chamber_of_deputies/ to chamber_of_deputies.urls or direct for the views and so on? - Each app would be responsible for his things.
3. Move each view for his respective app and the api be just a url wrapper? - Half of the way from 1 to 2 😕
4. Do nothing, its good for now

[cuducos]

Maybe api.views is too long and is not the best choice concentrate all those classes there.

I really liked number 2 ; )

[giovanisleite]

I really liked number 2 ; )

Ready to review 👍

Thanks, @cuducos

[cuducos]

Many thanks for this PR @giovanisleite — I'm pretty sure it meant a lot of work, but this change is very important for Jarbas! Personally I'm proud about the work you've done this PR, so glad, mate ; )