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

[55] Filter on lessons title #78

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

danielferecatu
Copy link

What does it fix?

Issue #55

How has it been tested?

From GraphiQL with seed data:

query Themes {
    lessons(term: "ip") {
        items {
            title
        }
    }
}
{
  "data": {
    "lessons": {
      "items": [
        {
          "title": "Consectetur adipiscing elit"
        },
        {
          "title": "Nemo enim ipsam voluptatem"
        }
      ]
    }
  }
}

@danielferecatu danielferecatu force-pushed the feature/issue-55 branch 2 times, most recently from a490c4e to e667b8d Compare November 25, 2023 14:23
@@ -32,6 +32,11 @@ public PaginatedResult<Lesson> PaginatedList(LessonFilter filter)
result = result.Where(l => l.Theme.Id == filter.ThemeId);
}

if (!string.IsNullOrEmpty(filter.Term))
{
result = result.Where(l => l.Title.Contains(filter.Term) || l.Description.Contains(filter.Term));
Copy link
Contributor

@nicolaes nicolaes Nov 27, 2023

Choose a reason for hiding this comment

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

Consider ignoring case and accents (e.g. diacritice): EF.Functions.ILike(l.Title, $"%{filter.Term}%"))

Edit: this only does case-insensitivity. For the accent part you can move to Postgres query or remove accents manually.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the remarks. I will update the code. For the accent case, I noticed that EF has a function EF.Functions.Unaccent() but only works with UTF-8 dbs, I'll check that and treat accents also.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think it will be better to do this from the query level so we can easily reuse it for all filter terms.

I propose creating a custom string scalar type

public class FilterTermStringGraphType : StringGraphType
{
    public override object ParseValue(object value)
    {
        // Your preprocessing logic
        string processedValue = value.ToString().ToLower(); // Example: convert to lowercase
        // Add more cleaning/preprocessing steps as needed

        return base.ParseValue(processedValue);
    }
}

And use this as the argument type.

The benefit of this approach is that we can have a simple way to update cleaning for this terms no matter of the query they are used in.

Copy link
Contributor

@alexandru-calinoiu alexandru-calinoiu left a comment

Choose a reason for hiding this comment

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

Proposed alternative to term filtering

@@ -32,6 +32,11 @@ public PaginatedResult<Lesson> PaginatedList(LessonFilter filter)
result = result.Where(l => l.Theme.Id == filter.ThemeId);
}

if (!string.IsNullOrEmpty(filter.Term))
{
result = result.Where(l => l.Title.Contains(filter.Term) || l.Description.Contains(filter.Term));
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think it will be better to do this from the query level so we can easily reuse it for all filter terms.

I propose creating a custom string scalar type

public class FilterTermStringGraphType : StringGraphType
{
    public override object ParseValue(object value)
    {
        // Your preprocessing logic
        string processedValue = value.ToString().ToLower(); // Example: convert to lowercase
        // Add more cleaning/preprocessing steps as needed

        return base.ParseValue(processedValue);
    }
}

And use this as the argument type.

The benefit of this approach is that we can have a simple way to update cleaning for this terms no matter of the query they are used in.

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.

3 participants