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

On mobile, some domains starting with "m dot" are not covered by bubbles whereas they should appear #429

Open
MaartenLMEM opened this issue Jul 30, 2021 · 9 comments
Assignees

Comments

@MaartenLMEM
Copy link
Contributor

Problème
Les sites web couverts ont parfois des versions mobile débutant par M point , et non www.
Par suite, les bulles n'apparaissent pas sur mobile, notamment dans les exemples phares.
Parmi les sites déjà identifiés dans ce cas :

  • m.youtube.com ... mais aussi YouTu.be
  • m.darty.com
  • m.boulanger.com
    et bien d'autres
@MaartenLMEM
Copy link
Contributor Author

MaartenLMEM commented Aug 3, 2021

Discussed with @JalilArfaoui yesterday :
Solution :

  • Add an alias domain field in BO (and contribution team will add these alias
  • Domain and its alias will be sent to the app as matching domains

Please Jalil add also a comments field for contributors. It will be usefull for us to explain choices we made for protodomains and alias.

@MaartenLMEM
Copy link
Contributor Author

Pour rappel, 2 risques si on ne specifie pas le www. ou le M. aujourd'hui :

  • matcher avec des urls qui n'ont rien à voir par ex : lemonde.fr/backmarket.fr-levee-de-fond qui matche avec le site regex
  • matcher avec des sous-domaines du même domaine : backoffice, espace client, etc.

@zhinu
Copy link
Collaborator

zhinu commented Aug 18, 2021

Pour Résoudre le risques 1 :
On peut penser à un régex qui cible le début de l'adresse :

^(https?\:\/\/)?([a-zA-Z]+\.)?domaine\.com

Pour les sites :
abc.domaine.com
www.domaine.com
domaine.com
http://www.domaine.com
https://abc.domaine.com

ça évite :
pasdomaine.com/domaine.com

Pour le risque 2 :
Quasiment aucun risque pour les Xpath
Risque très limité sur les regex (plus la regex est précise moins le risque est grand).

Si on souhaite viser un protodomaine précis, on pourrait penser à une regex telle que :

^(https?\:\/\/)?proto\.domaine\.com

Ça viserait les sites
proto.domaine.com
https://proto.domaine.com

@lutangar
Copy link
Member

lutangar commented Sep 2, 2021

matcher avec des urls qui n'ont rien à voir par ex : lemonde.fr/backmarket.fr-levee-de-fond qui matche avec le site regex
@zhinu suggest : On peut penser à un régex qui cible le début de l'adresse

Looks enough for me, but I'm probably missing something...

matcher avec des sous-domaines du même domaine : backoffice, espace client, etc.

Isn't it exactly what we're trying to achieve here? ex: match m subdomain of the same domain?

@MaartenLMEM MaartenLMEM assigned zhinu and lutangar and unassigned MaartenLMEM Sep 3, 2021
@lutangar
Copy link
Member

lutangar commented Sep 6, 2021

What do you think @JalilArfaoui ?

@MaartenLMEM
Copy link
Contributor Author

These are questions here about the need. @zhinu don't hesistate to answer or complete.

matcher avec des sous-domaines du même domaine : backoffice, espace client, etc.

Isn't it exactly what we're trying to achieve here? ex: match m subdomain of the same domain?

Yes and no.

  • Yes for m subdomain,
  • Yes for fr.siteweb.com subdomain
  • Yes for be.siteweb.com subdomain (really secondary)
  • No for admin.siteweb.com subdomain (nice to have)

@MaartenLMEM
Copy link
Contributor Author

MaartenLMEM commented Sep 6, 2021

Maybe the matter here is just
if i create a domain www.
And I add an alias m.
then it will match with m.

@JalilArfaoui
Copy link
Member

JalilArfaoui commented Sep 6, 2021

Yes, to me, the issue here is simply :

Add an alias field in the Domain entity so that if we add m.youtube.com to domain www.youtube.com and we set a matching context on domain www.youtube.com with regex .*something-in-the-url, the resulting served regex should be :

(www\.youtube\.com|m\.youtube\.com).*something-in-the-url

(instead of currently (www\.youtube\.com).*something-in-the-url)

Do we all agree of that ?

@MaartenLMEM
Copy link
Contributor Author

MaartenLMEM commented Sep 6, 2021

Yes, we agree on this solution @JalilArfaoui
After talking with @lutangar , here is summary :

Current situation

  • If I domain contains sncf.com , it matches with
www.sncf.com 
mobile.sncf.com 
m. sncf.com
fr.sncf.com 
admin.sncf.com
login.sncf.com
  • If domain conaints www.sncf.com , i matches with
    www.sncf.com
    but not mobile.sncf.com norm. sncf.comnor fr.sncf.com

Targeted functioning

New feature : ALIAS field that can contain multiple values in domains part

Operations

  • If I domain contains sncf.com , it will match with
www.sncf.com 
mobile.sncf.com 
m. sncf.com
fr.sncf.com 

(thus like currently)

  • If I domain contains www.sncf.com , and alias is mobile.sncf.com and m. sncf.com , it will match with :
www.sncf.com 
mobile.sncf.com 
m. sncf.com

but not :

fr.sncf.com 
admin.sncf.com
login.sncf.com

lutangar added a commit that referenced this issue Sep 7, 2021
Aliases are mapped to a simple php array, there is a little hack to overcome an issue with Doctrine's `simple_array` which maps the `[]` to `null` in database, hence the oddities you can see in the `DomainName` entity.

This also update the "big" regexp we expose with the possible aliases of domain. (see updated tests for example of generated regexp).

The same validation rule as a `Domain` `name` has been added to each `aliases` element but both seems to be ignored by __ZiziAdmin__...

> This fix issue #429 💋
lutangar added a commit that referenced this issue Sep 9, 2021
Aliases are mapped to a simple php array, there is a little hack to overcome an issue with Doctrine's `simple_array` which maps the `[]` to `null` in database, hence the oddities you can see in the `DomainName` entity.

This also update the "big" regexp we expose with the possible aliases of domain. (see updated tests for example of generated regexp).

The same validation rule as a `Domain` `name` has been added to each `aliases` element but both seems to be ignored by __ZiziAdmin__...

> This fix issue #429 💋
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

No branches or pull requests

4 participants