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

Code erroné depuis plusieurs années (is_user_in_followers?(page['next_page']) #3431

Closed
thbar opened this issue Aug 31, 2023 · 7 comments · Fixed by #3497
Closed

Code erroné depuis plusieurs années (is_user_in_followers?(page['next_page']) #3431

thbar opened this issue Aug 31, 2023 · 7 comments · Fixed by #3497
Assignees
Labels
dette technique Entretien & maintenance générale, nécessaire pour que le code reste de bonne qualité

Comments

@thbar
Copy link
Contributor

thbar commented Aug 31, 2023

Comme relevé par @AntoineAugusti ici:

Le reformattage apporté par Elixir 1.15 permet de voir qu'on accède à une clé de payload datagouv par 'next_page' et non "next_page".

Du coup cela retourne nil systématiquement.

J'allais corriger dans la PR #3429 mais j'ai eu un doute en lisant le code.

Je préfère éviter de modifier là-bas, car il n'y a pas de tests, et le bug étant ancien, je préfère éviter de modifier sans vérification plus précise (éviter une boucle infinie, des lenteurs ou autre, car il y a des requêtes HTTP supplémentaires).

Je pense qu'il vaut mieux ajouter des tests tranquillement, le bug étant par ailleurs très ancien (plus de 5 ans)

Voir aussi:

# Check if user_id is in followers, if it's not, check in next page if there's one
@spec is_user_in_followers?({:ok, map()} | binary(), binary(), Plug.Conn.t()) :: boolean()
defp is_user_in_followers?({:ok, %{"data" => followers} = page}, user_id, conn) when is_list(followers) do
Enum.any?(
followers,
&(&1["follower"]["id"] == user_id)
) or is_user_in_followers?(page['next_page'], user_id, conn)
end
defp is_user_in_followers?(page_url, user_id, conn) when is_binary(page_url) do
conn
|> OAuthClient.get(page_url)
|> is_user_in_followers?(user_id, conn)
end
defp is_user_in_followers?(_, _, _), do: false

@thbar thbar added the dette technique Entretien & maintenance générale, nécessaire pour que le code reste de bonne qualité label Aug 31, 2023
@AntoineAugusti
Copy link
Member

Peut-être qu'on peut avoir un per_page pour cette requête plus grand pour éviter de faire trop de requêtes pour voir si la personne est abonnée ou pas. Je sais pas s'il y a des JDD avec beaucoup de followers

@thbar
Copy link
Contributor Author

thbar commented Aug 31, 2023

J'irai regarder un peu la data pour comprendre, mais vu que c'est vieux j'ai préféré ne pas hâter les choses et me planter un jeudi :-)

@thbar
Copy link
Contributor Author

thbar commented Aug 31, 2023

Différent et sans le même impact mais surprenant quand même:

@AntoineAugusti AntoineAugusti self-assigned this Oct 2, 2023
@AntoineAugusti
Copy link
Member

Je vais le traiter

@thbar
Copy link
Contributor Author

thbar commented Oct 2, 2023

@AntoineAugusti un point auquel j'avais pensé c'est qu'il faut faire un peu gaffe aux effets de bords du type "volume retourné beaucoup plus importants" potentiellement

@AntoineAugusti
Copy link
Member

Ouais je vais regarder le temps de réponse et s'il vaut mieux avoir plus d'éléments par page (per_page) pour éviter qu'on fasse 3-4 requêtes si il y a beaucoup de résultats

@thbar
Copy link
Contributor Author

thbar commented Oct 2, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dette technique Entretien & maintenance générale, nécessaire pour que le code reste de bonne qualité
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants