Improvement: if the Debug true disable the verify ssl in fetch content, That form is better to run local, I think :) #27
Conversation
…t, That form is better to run local, I think :)
|
The .env that is use for CI, is the same as for local development? |
There was a problem hiding this comment.
Many thanks for the contribution @elinaldosoft! I think you code have some issues, so I added minor inline comments to highlight what I think is important to be reviewed before we merge, ok?
victims/data.py
Outdated
| if DEBUG: | ||
| session = aiohttp.ClientSession( | ||
| connector=aiohttp.TCPConnector(verify_ssl=False) | ||
| ) |
There was a problem hiding this comment.
I think this is not the proper place to handle this kwarg. The session is defined a few lines above and you're overwriting it. Beyond that now there's two different places defining sessions for the same request, what is a bit confusing. I'm gonna suggest something different around line 40;
async def fetch_spreadsheets(self):
urls = (…)
kwargs = {'loop': asyncio.get_event_loop()}
if DEBUG:
kwargs = {'connector': aiohttp.TCPConnector(verify_ssl=False)}
async with aiohttp.ClientSession(**kwargs) as session:
…
victims/templates/home.html
Outdated
| @@ -22,7 +22,7 @@ | |||
| </div><!-- end .meta --> | |||
|
|
|||
| <h2> | |||
| <a class="no-underline" href="{{ case.main_story.url }}">{{ case.main_story.title|e }}</a> | |||
| <a class="no-underline" href="{{ case.main_story.url }}" target="_blank">{{ case.main_story.title|e }}</a> | |||
There was a problem hiding this comment.
This doesn't belong in this PR. Please, check #9.
victims/tests/test_data.py
Outdated
| data = Data() | ||
| loop = asyncio.get_event_loop() | ||
| cases = loop.run_until_complete(data.cases()) | ||
| assert len(cases) == 3 |
There was a problem hiding this comment.
This is not advisable. It makes tests depends on external connection and availability, and it also depends on envvar settings (have you seen it's failing on Travis? We use the production stylesheet there, probably that's why it's failing).
|
What would be the way to test the session (verify_ssl=false) of request? |
|
We need two mocks I guess:
|
|
@elinaldosoft We still have a review item to complete in this PR. Do you need any help? |
In my case it was giving error.