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

BNF only : arret sur images #283

Merged
merged 3 commits into from
Jan 18, 2025

Conversation

JeremieGiffard
Copy link
Contributor

@JeremieGiffard JeremieGiffard commented Jan 17, 2025

BNF developpement specifique

#22

Ajout du site arretsurimages pour les abonné.e.s BNF .
Le bouton s'affiche en haut de l'article et sur le bandeau paywall en bas.

je ne suis pas un expert front, ni js : si vous voyez des optimisations possible, je suis prenneur.

tests effectués

exemple article payant arretSurImages

firefox

type test result
Partenaire europresse selectionné : BNF le bouton s'affiche bien sur article payant
Partenaire europresse autre que BNF Le bouton ne s'affiche pas sur article payant
click button Redirection vers mirroir bnf ok
Visuel du bouton affichage ok sur mobile et pc

@JeremieGiffard JeremieGiffard marked this pull request as ready for review January 17, 2025 16:36
@Write Write requested a review from lovasoa January 17, 2025 17:18
@Write
Copy link
Collaborator

Write commented Jan 17, 2025

Hello, merci pour le PR.

J'assigne @lovasoa le fondateur du projet pour les choix plus complexe dans l'architecture du code.

À première vue, je dirais que c'est un bon premier PR.

Il pourrait être plus approprié de déplacer la logique de l'await ophirofox_config dans le fichier https://github.com/lovasoa/ophirofox/blob/master/ophirofox/content_scripts/config.js, afin de suivre la pratique consistant à regrouper des codes similaires des checks dans la config. Cela permettrait d'avoir la logique de vérification des configurations spécifiques uniquement dans le fichier config.js (centralisé).

Pour l'URL Host du proxy spécifique pour la BNF, peut-être rajouter une clé spécifique dans le manifest.json plutôt que de le hardcoder dans le code.
Par ex dernièrement j'ia rajouté une clé optionnel PROXY_URL pour les besoins du matching Sciences Po Paris, attendre l'avis de lovasoa pour une proposition de structure.

Je ne peux pas tester le reste, mais j'imagine que pour le moment le code ne redirige pas vers l'article spécifique une fois sur le proxy arrêt sur images, mais uniquement vers la page de proxy ?

@lovasoa
Copy link
Owner

lovasoa commented Jan 17, 2025

Je n'ai pas beaucoup de temps à consacrer à ce projet en ce moment et je ne voudrais pas l'immobilier. @Write , je te fais toute confiance pour les décisions importantes !

@JeremieGiffard
Copy link
Contributor Author

Je regarde pour prendre en compte tes conseils. Merci pour les exemples ! je vais regarder comme ça a été implementé.

Je ne peux pas tester le reste, mais j'imagine que pour le moment le code ne redirige pas vers l'article spécifique une fois sur le proxy arrêt sur images, mais uniquement vers la page de proxy ?

Si ça redirige bien directement sur l'article. :)

@Write
Copy link
Collaborator

Write commented Jan 17, 2025

Je regarde pour prendre en compte tes conseils. Merci pour les exemples ! je vais regarder comme ça a été implementé.

Je ne peux pas tester le reste, mais j'imagine que pour le moment le code ne redirige pas vers l'article spécifique une fois sur le proxy arrêt sur images, mais uniquement vers la page de proxy ?

Si ça redirige bien directement sur l'article. :)

Ok super, désolé en lisant le code ça ne m'a pas sauté aux yeux ^^

@JeremieGiffard
Copy link
Contributor Author

@Write nouveau commit prenant en compte tes retours.

  • j'en ai profité pour rajouter un com pour expliciter la gestion de la redirection de l'url

proposition de gestion des configs specifiques dans config.js

j'ai rajouté la methode configurationsSpecifiques(configNames)
qui prend une liste de nom de partenaires en entrée, plutot qu'un simple string.

Ce qui permet par exemple d'avoir dans manifest.js des configs pour le meme site suivant le partenaire.

{
  "name": "Bibliotheque municipale de Lyon",
  "AUTH_URL": "https://connect.bm-lyon.fr/get/login?blablabla",
  "AUTH_URL_ARRETSURIMAGES" : "www.URL-SPECIFIQUE-LYON.COM" 
},
{
  "name": "BNF",
  "AUTH_URL": "https://bnf.idm.oclc.org/login?url=https://nouveau.eublablabla",
  "AUTH_URL_ARRETSURIMAGES" : "www-arretsurimages-net.bnf.idm.oclc.org" 
},

Le code /content_scripts/arret-sur-images.js va checker si le current user est BNF ou biblio Lyon et remplacer le lien du bouton avec le domaine fourni dans manifest.json. On a juste à rajouter le nom du partenaire dans la liste, et respecter la nomenclature AUTH_URL_ARRETSURIMAGES. Sans toucher au reste de la classe

async function onLoad() {
    const config = await configurationsSpecifiques(['BNF',  'Bibliotheque municipale de Lyon'])
//[.....]
  for (const balise of reserve) {
   // URL va se etre gerée suivant la valeur dans manifest. Pour bibliotheque de lyon, comme BNF.
    balise.parentElement.appendChild(await createLink(config.AUTH_URL_ARRETSURIMAGES));
  }
}

optimisation ?

Dans configurationsSpecifiques, j'ai mis un return que si le if() est true. Aucune idée si c'est un possible memory leak d'avoir une promesse sans return.

@Write
Copy link
Collaborator

Write commented Jan 18, 2025

Super, je pense que c'est bien plus propre comme ça. Merci !

@Write Write merged commit 4f921ae into lovasoa:master Jan 18, 2025
1 check passed
@Write Write assigned Write and unassigned lovasoa Jan 18, 2025
@Write Write requested review from Write and removed request for lovasoa January 18, 2025 18:07
@Write Write added the enhancement New feature or request label Jan 18, 2025
@JeremieGiffard JeremieGiffard deleted the feature/BNF-arret-sur-images branch January 29, 2025 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Question] Bien-fondé d'ajouter des fonctionnalités supplémentaires liées spécifiquement à la BNF ?
3 participants