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

Feature/jobology catch profile #206

Merged
merged 12 commits into from
Jan 8, 2024

Conversation

Abdellahitech
Copy link
Contributor

No description provided.

Copy link
Collaborator

@the-forest-tree the-forest-tree left a comment

Choose a reason for hiding this comment

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

Merci pour le travail @Abdellahitech . Il y'a des modifications a apporter.

Comment on lines 36 to 40
tags = []
for key, value in profile_tags.items():
if value:
tags.append({"name": key, "value": value})
return tags
Copy link
Collaborator

Choose a reason for hiding this comment

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

C'est bien machaa Lah regarde si tu peux pas le faire en une seule ligne

return [dict(name=key, value=value) for key, value in profile_tags.items() if value]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parfait!

Comment on lines 19 to 31
"first_name": jobology_profile.get("firstName", None),
"last_name": jobology_profile.get("lastName", None),
"phone": jobology_profile.get("phone", None),
"email": jobology_profile.get("email", None),
"coverText": jobology_profile.get("coverText", None),
"profile-country": jobology_profile.get("profilecountry", None),
"profile-regions": jobology_profile.get("profileregions", None),
"profile-domains": jobology_profile.get("profiledomains", None),
"job-lien_annonce_site_carriere": jobology_profile.get(
"joblien_annonce_site_carriere", None
),
"statistic-source": jobology_profile.get("statisticsource", None),
"statistic-jbsource": jobology_profile.get("statisticjbsource", None),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tu peux enlever tous les get("xxxx", None) parce que c'est le comportement par default. get("xxx") c'est la même chose

parameters.profile["cv"] = response.content
parameters.profile["content_type"] = response.headers["Content-Type"]
elif response.status_code == 400:
raise ErrorHandler(f"Error 400: Bad Request - {response.text}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ici ton ErrorHandler je vois pas à quoi il sert. C'est quoi l'avantage par rapport à juste raiser Exception ?

En général tu définis une classe d'erreur pour une valeur ajoutée. Parfois tu veux avoir des erreurs un peu riche. Du genre

class MyHTTPError(Exception):
    def __init__(self: t.Self, code: int, detail: str) -> None:
          self.code = code 
          self.detail = detail

# Ca te permet dans ton code mnt de faire un 
raise MyHTTPError(code=404, detail="Not Found")
# ou ailleurs 
raise MyHTTPError(code=403, detail="Forbidden")

# Et après rien t'empeche de faire quelque chose avec .code ou .detail

Ou parfois c'est parce que tu as un autre code qui va essaye de catch juste cette erreur en particulier donc tu peux rien mettre dans l'init dans ce cas parce que ton besoin c'est autre chose.

class MyError(Exception):
    pass

def my_function():
     .... 
     if condition: 
           raise MyError()

# Et ailleurs tu as fais
try:
     my_function()
except MyError:  # Ca te permet de catcher que ce qui t'interesse 
    ....

Mais ici je vois aucun des cas. Donc si ca sert à rien tu le fais pas et tu raises juste Exception.

Biensur le but c'est pas de dire dans ce cas je rajoute à ErrorHandler un __init__. Parce que tu fais pas les choses dans ce sens. Tu rajoutes pas du code pour justifier un autre code mais tu rajoutes du code quand il y'a un besoin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oui en fait c'est pour répondre à un besoin de jobology voici l'email. donc je voudrais que mon workflow catch puisse retrourner des messages spécifiques selon ce qu'on va s'accorder avec notre client commun fedgroup:
Bonjour Abdellahi,
Voici un exemple des différents types de réponse que nous pouvons recevoir sur d’autres ATS :

200 : candidature enregistrée

400 : problème avec le format de la requête ou de certains champs (détails dans la réponse)

403 : token partenaire absent ou non reconnu

409 : ce candidat a déjà candidaté à cette offre (vérification basée sur le mail)

410 : l'offre n'accepte pas de candidature : offre pas encore publiée ou recrutement suspendu / terminé.

412 : cette offre n'existe pas

500 : erreur interne, le support technique HrFlow est alerté

Nous n’avons obligatoirement besoin de ces informations, mais cela permet de s’assurer que les candidatures sont réceptionnées correctement côté ATS et de fournir des statistiques justes au recruteur.

Cordialement,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Je comprends mais du coup c'est possible que tu puisses par arriver à ce niveau de detail avec hrflow-connectors

Parce que toi quand tu vas utiliser le connecteur tu vas faire result = MyConnector.some_action(...) et ca va te rendre un RunResult comme expliqué ici

En fonction de ce qu'il yá dans le RunResult tu peux après envoyer un code d'erreur approprié dans un Workflow. Mais c'est limité au format dans le lien

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 ça marche. merci j'utilise donc juste Exception

Comment on lines 38 to 39
parameters.profile["cv"] = response.content
parameters.profile["content_type"] = response.headers["Content-Type"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

En fait ce genre de chose ca peut causer des bugs assez difficile à comprendre quand tu es dedans.
paramaters.profiles c'est un argument de ta fonction.
Imagine si le code qui appelle ta fonction ressemble à

parameters = ReadProfilesParamters(....)
# Ici il appelle ta fonction
result = read(logger, parameters, None, None)
parameters.profiles["content_type"] = "AUTRE CHOSE" # Pour une raison X ou Y 

Ici le code peut s'attendre à ce que dans result[0]["content_type"] correspondent à ce que tu as mis toi
à l'interieur de read mais non mnt quand il a fait parameters.profiles["content_type"] = "AUTRE CHOSE" il a aussi changer result[0]["content_type"].

Donc le truc à faire c'est d'essayer autant que tu peux de pas modifier tes arguments. Mais soit tu creer de nouveau object qui seront independant. Du genre

result = dict(cv=response.content, content_type=response.headers["Content-Type"])

Et si tu as besoin tu peux copier en faisant

result = {**parameters.profile}
# Puis rajouter ce que tu rajoutes dans ta fonction

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 je vais corriger merci

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarde Abdellahi ce que tu as fais c'est pas bon ici.

Je sais que tu peux avoir pas mal de pression pour livrer les choses rapidement. Mais apprendre à gérer la pression ca fait partie du travail de développeur. Parfois il faut assumer de rendre plus tard que demandé parce que tu sais que hadchi la ma7ala. Si tu fais trop vite ca sera pas bon. Ca c'est une sounnah de lkawn. Personne n'a le choix.

Les problèmes :

  1. Ta fonction faire deux fois la même chose
    cv_url = parameters.profile["cvUrl"]
    response = requests.get(cv_url)
    response.raise_for_status()
    parameters.profile["cv"] = response.content
    parameters.profile["content_type"] = response.headers["Content-Type"]

Puis juste après encore la meme requete pour get cvUrl

    cv_url = parameters.profile["cvUrl"]
    response = requests.get(cv_url)
    # Check for specific HTTP status codes and handle accordingly
    if response.status_code == 200:
        parameters.profile["cv"] = response.content
        parameters.profile["content_type"] = response.headers["Content-Type"]
    elif response.status_code == 400:
        raise ErrorHandler(f"Error 400: Bad Request - {response.text}")

Pour une PR avec une seule action. Quand même il faut juste relire.

  1. Tu fais dans ces deux fois deux choses differentes. La première fois raise_for_status la deuxieme tu donnes plus de détail ? Pourquoi la difference ?

  2. Le plus grave: Tu as pas tester ton code. Parce que si tu avais testé ton code comme il faut. Il faudrait a minimum que tu vérifie que ton raise ErrorHandler sert à quelque chose.

Sauf que a priori si tu t'es mis dans des conditions ou ca fail ca devrait failer à la ligne 37 ou tu fais response.raise_for_status() et ne jamais voir tes messages.

Le dernier point c'est le plus important. Il y'a un adage dans le monde du dev qui dit a peu près: Du code non testé c'est du code buggé.

Donc essaye la prochaine fois de faire plus attention avant de faire une PR stp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oui j'avoue que je suis allé plus vite je voudrais remplacer ce qui est après par ce qui est avant. je corrige, je test et je te dis. merci

Copy link
Contributor Author

Choose a reason for hiding this comment

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

j'ai bien testé maintenant. merci

Copy link
Collaborator

@the-forest-tree the-forest-tree left a comment

Choose a reason for hiding this comment

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

Merci pour le travail. C'est beaucoup mieux. Je t'ai rajouté un seul commentaire à regler. Après c'est bon pour merger incha Lah

Comment on lines 35 to 40
def add_tags(profile_tags: t.Dict) -> t.Dict:
return (
[dict(name=key, value=value) for key, value in profile_tags.items() if value]
if profile_tags
else []
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

En fait ici ce que tu fais dans le code n'est pas coherent avec ton typing. Ta fonction est déclaré comme acceptant profile_tags : t.Dict et pas profile_tags: t.Dict | None ou t.Optional[t.Dict]

Tu as le choix soit tu enleve la partie if profile_tags et conserve le typing de ton argument. Ou tu rajoute la possibilité que ce None dans la signature de la fonction et tu laisses le code tel quel.

La bonne option incha Lah correspond à ce qui est proche de la realité. Mais vu que ca vient de rename_profile_fields qui renvoit toujours un Dict et jamais None il faudrait plutot enlever le if profile_tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ça marche et aussi pour le outputs le typing doit être t.list inchallah

@the-forest-tree the-forest-tree merged commit 28ea90d into master Jan 8, 2024
4 checks passed
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.

2 participants