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.

Sign up for GitHub

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

Jump to bottom

Expose domains in matching Api #446

Closed
zhinu opened this issue Sep 8, 2021 · 0 comments · Fixed by #449
Closed

Expose domains in matching Api #446

zhinu opened this issue Sep 8, 2021 · 0 comments · Fixed by #449
Assignees
@lutangar @Goutte
Labels
enhancement

Comments

@zhinu
Copy link
Collaborator

zhinu commented Sep 8, 2021 •
edited by lutangar
Loading

In order to solve #433 #405 #429 and dis-moi/extension#277, @lutangar suggested to create a dedicated field for the domain, and allow the client to choose it.

Today the backend provides the extension and the app a concatenated regex which includes both the domain and the matching-context regex string.

"urlRegex": "(coffeeritz\\.fr|maxicoffee\\.com|sensaterra\\.com).*([^A-Za-zÀ-ÖØ-öø-ž]café?e?([^A-Za-zÀ-ÖØ-öø-ž].*[^A-Za-zÀ-ÖØ-öø-ž]|[^A-Za-zÀ-ÖØ-öø-ž])(grains?|moulus?)([^A-Za-zÀ-ÖØ-öø-ž]|$))"

The idea is to have two separate fields :

"domains": ["coffeeritz.fr", "maxicoffee.com", "sensaterra.com"], 
"pathRegex": "([^A-Za-zÀ-ÖØ-öø-ž]café?e?([^A-Za-zÀ-ÖØ-öø-ž].*[^A-Za-zÀ-ÖØ-öø-ž]|[^A-Za-zÀ-ÖØ-öø-ž])(grains?|moulus?)([^A-Za-zÀ-ÖØ-öø-ž]|$))"

The client has a way of detecting domain, so this will help in accuracy. It will also probably improve its efficiency (less latency), and make it more organized as we move towards a complicated features.
@zhinu zhinu assigned zhinu and MaartenLMEM and unassigned zhinu Sep 8, 2021
@zhinu zhinu changed the title Add a specific domain check to the back end. Add a specific domain check to the backend. Sep 8, 2021
@MaartenLMEM MaartenLMEM assigned Goutte and lutangar and unassigned MaartenLMEM Sep 8, 2021
@lutangar lutangar changed the title Add a specific domain check to the backend. Expose domain in matching Api Sep 13, 2021
@lutangar lutangar changed the title Expose domain in matching Api Expose domains in matching Api Sep 13, 2021
This was referenced Sep 13, 2021
Open
Closed
lutangar added a commit that referenced this issue Sep 13, 2021
@lutangar
feat(matching-context): expose list of domains in API
4897c4c 
Exposes the list of domains on which a matching context is applicable. It also exposes the *path* part of the regexp uses in mathching context...

Some current and previous vocabulary seems dodgy... maybe a `DomainName` should be renamed to `Domain` so we can use `domainName` only for the actual string version of the domain name...

Also `urlPathRegex` is not truly a *path* __regex__ since the user can choose to match all domains... is that still supposed to be possible?

> fix #446
@lutangar lutangar mentioned this issue Sep 13, 2021
Merged
lutangar added a commit that referenced this issue Sep 13, 2021
@lutangar
feat(matching-context): expose list of domains in API
a417211 
Exposes the list of domains on which a matching context is applicable. It also exposes the *path* part of the regexp uses in mathching context...

Some current and previous vocabulary seems dodgy... maybe a `DomainName` should be renamed to `Domain` so we can use `domainName` only for the actual string version of the domain name...

Also `urlPathRegex` is not truly a *path* __regex__ since the user can choose to match all domains... is that still supposed to be possible?

> fix #446
lutangar added a commit that referenced this issue Sep 14, 2021
@lutangar
feat(matching-context): expose list of domains in API
6f24b8d 
Exposes the list of domains on which a matching context is applicable. It also exposes the *path* part of the regexp uses in mathching context...

Some current and previous vocabulary seems dodgy... maybe a `DomainName` should be renamed to `Domain` so we can use `domainName` only for the actual string version of the domain name...

Also `urlPathRegex` is not truly a *path* __regex__ since the user can choose to match all domains... is that still supposed to be possible?

> fix #446
lutangar added a commit that referenced this issue Sep 14, 2021
@lutangar
feat(matching-context): expose list of domains in API
03b4f84 
Exposes the list of domains on which a matching context is applicable. It also exposes the *path* part of the regexp uses in mathching context...

Some current and previous vocabulary seems dodgy... maybe a `DomainName` should be renamed to `Domain` so we can use `domainName` only for the actual string version of the domain name...

Also `urlPathRegex` is not truly a *path* __regex__ since the user can choose to match all domains... is that still supposed to be possible?

> fix #446
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lutangar lutangar

@Goutte Goutte

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(matching-context): expose list of domains in API dis-moi/backend
4 participants
@lutangar @Goutte @MaartenLMEM @zhinu