Skip to content

Conversation

@eduzen
Copy link
Member

@eduzen eduzen commented Apr 30, 2018

Aca agregamos los jobs sponsoreados!

@eduzen
Copy link
Member Author

eduzen commented May 1, 2018

@cmdelatorre Perdimos la historia del pull request, pero acá estan los cambios que habías pedido para cerrar el ticket #417

@eduzen
Copy link
Member Author

eduzen commented May 1, 2018

@edvm @gilgamezh Cuando este pull se mergee hay que setearle desde el admin a las empresa que son benefactoras del asoc. civil un 1 en el campo que agregamos! Asi figuran los jobs promocionados.
Y el logo de la asoc cuando entras al perfil de la empresa.

Copy link
Contributor

@edvm edvm left a comment

Choose a reason for hiding this comment

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

En general se ve muy bien! Nomas hice algunos comments de cosas que tengo dudas y pedí algunos fixes muy menores :)

'-company__rank', '-created')[:sponsored_size]
return sponsored_jobs

def non_sponsored(self, from_date, sponsored_size):
Copy link
Contributor

Choose a reason for hiding this comment

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

Pregunta @eduzen , les parece filtrar los non_sponsored usando company__rank==0 ? como para que sea mas barata la query? :) Veo en el jobs/views.py que non_sponsored toma sponsored_size como argumento, para poder obtener la lista de, digamosle sponsored jobs. Existe una relacion entre la lista de jobs non_sponsored y sponsored_size? o es para poder llamar al queryset sponsored() y de ahi hacer el exclude(pk_in=sponsoredjobs) ? Si fuese esto ultimo, me parece es mas conveniente filtrar por company_rank=0, que opinan?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hola @edvm, creo que no podemos filtrar por company_rank == 0 solamente.

sponsored_size remite a la cantidad de trabajos sponsoreados:

captura de pantalla 2018-05-05 17 16 58

Ahí en la imagen podes ver que son 2 los que puse para probar. Ahora esos dos no son quizás los únicos jobs sponsoreados que hay, entonces los que restan no son solamente los de ranking == 0.

Espero poder explicarme!

Copy link
Contributor

Choose a reason for hiding this comment

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

Gracias por la explicación @eduzen !

self.assertEqual(len(response.context["job_list"]), 1)
self.assertIn(sponsored_job, response.context["sponsored_jobs"])
self.assertIn(sponsored_job2, response.context["sponsored_jobs"])
self.assertEqual(len(response.context["sponsored_jobs"]), 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Esto esta genial! @eduzen pueden agregar un test para un company con rank=0 que cree jobs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dale ahí agrego!

</div>
{% if object.rank > 0%}
<hr/>
<p> Socia Benefactora de la <a href="https:/ac.python.org.ar">Asociación Civil Python Argentina</a></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

mini typo, falta un / en el anchor tag (dice `https:/ac.python.org.ar').

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@edvm edvm requested a review from cmdelatorre May 4, 2018 17:23
@eduzen
Copy link
Member Author

eduzen commented May 4, 2018

@edvm el finde reviso los cambios sugeridos, y contesto pto por pto, igual el rank==0 pinta bien! Pero lo tengo que pensar porque quizas queden algunos afuera, algunos sponsoreados pero que no entran el top 3 que se rankean arriba.
Digamos que si no aparecen en ese sponsored_size (en ese slot) no queden excluidos de la lista de jobs de abajo!

@cmdelatorre
Copy link
Contributor

@eduzen tal como charlamos en el PyCamp, no es trivial el manejo de los "sponsored slots".
Te sugiero que describas el algoritmo actual en #417 para que todos sepan lo que se hizo.
Depués, podemos ir planteando modificaciones y ponerlas en marcha.
Pero por lo pronto, calculo que lo que está alcanza. El tema es que los colaboradores puedan entender lo que se hizo. A veces leer eso en el código no es fácil.

Copy link
Contributor

@edvm edvm left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Muchas gracias!

@edvm edvm merged commit 5256a9b into PyAr:dev May 6, 2018
gilgamezh added a commit that referenced this pull request Jun 2, 2018
* add flake8 to tests.

* pep8 police

* move flake8 config to .flake8

* fix pep8 args

* add python3.5 and 3.6 to travis-ci

* speed up tests

* Add a general word-wrap style rule

* Use docker image from dockerhub. Makefile with command alias

* Adds gender field to EventRegistration model

* Adds migration related to new gender field

* Adds gender field to corresponding forms

* Show the gender in registered list and CSV.

* fix pep8

* Change the homepage context

* Replaces Ultimas Noticias and Últmos Trabajos with a new section

* Refactor the homepage as a class-based view

* Added factories for testing

* Added tests for the new homepage

* Sanitize html inputs on Events and Jobs (#412)

* fix autosuggest js on jobs form

* refs #409 #410 add html_sanitizer to description field of Job and Event

* refs #409 410 add tests to Job and Event views

* remove duplicate requeriment

* adjust code to pycodestyle

* fix wrong exception class

* Update job_form.html

* added normalize_tags function on job utils (#418)

* 407 migration to fix tags (#419)

* Arreglo error de linter

* Added new makemigrations shortcut

* added missing migration

* Adds a migration to clean the tags

* split function to use ir

* Use tag normalization function in migration

* #346 autoslug (#420)

* added autoslug fields to events

* fixed url position

* Fixed migrations (#423)

* Resolviendo issue #383 (#425)

* Resolviendo issue #383

Agregue el EMAIL_CONFIRMATION_LA_DOMAIN con el valor "python.org.ar". Probado en un servidor debug funciona correctamente el link.

* arreglando error de flake8

* 417 sponsored jobs (#424)

* Added rank for pycompanies, updated joblist view with sponsored jobs, added ac.civil badget to pycompanies

* Changes after code review

* moved css styles and other html stuff

* Fixed typo and added one test with rank==0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants