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

bugfix: only allow results that are within specified season/year and nulls #448

Merged
merged 7 commits into from
Nov 3, 2023

Conversation

irfan-dahir
Copy link
Contributor

Issue/Discussion: https://discord.com/channels/460491088004907029/1119461866322800662

There are a handful of entries like 52991 start on September 29th which is technically in the range of the summer season (which is why it returns in the results for summer 2023). However, it's officially listed under the Fall season in premiered.


This PR attempts to resolve this issue by filtering down the results to

  1. It contains the premiered string (e.g "Summer 2023") from a specific seasonal endpoint request
  2. It contains null values (for OVA, ONA, Movie, etc that have premiered set to null)
  3. It contains entries within the range given by the current season's start_date and end_date (Existing behavior)

@irfan-dahir irfan-dahir requested a review from a team as a code owner October 21, 2023 04:53
Copy link
Collaborator

@pushrbx pushrbx left a comment

Choose a reason for hiding this comment

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

I tried to add my comments in top down order, so you would read them after each other and they are connected.

In summary:

  • getSeasonItems function of QueryAnimeSeasonHandlerBase class should not change
  • getAiredBetween function in AnimeRepository class should handle the edge case if "premiered" date should always take precedence. Ideally this function should have the business logic to handle the situation. I would also add a new function maybe with the arguments you added, which wraps around getAiredBetween and adds the extra filters based on premiered, if it's possible to override the query builder that way. (haven't checked yet)
  • Existing behaviour of resolving season based on date, and the current way of resolving request params to a date range should remain as is. We would want to deal with the inconsistency at the database query level: if request was for season:spring year:2023
    then the query should first consider the premiered string then the aired date range based on the resolved date range.

app/Features/QueryAnimeSeasonHandlerBase.php Outdated Show resolved Hide resolved
app/Features/QueryCurrentAnimeSeasonHandler.php Outdated Show resolved Hide resolved
app/Features/QuerySpecificAnimeSeasonHandler.php Outdated Show resolved Hide resolved
app/Repositories/DefaultAnimeRepository.php Outdated Show resolved Hide resolved
app/Repositories/DefaultAnimeRepository.php Outdated Show resolved Hide resolved
@irfan-dahir
Copy link
Contributor Author

irfan-dahir commented Oct 24, 2023

@pushrbx

I would also add a new function maybe with the arguments you added, which wraps around getAiredBetween and adds the extra filters based on premiered, if it's possible to override the query builder that way. (haven't checked yet)

I tested this but getAiredBetween returns with the orderBy query afterwards, so any added query does not work.

So I added premiered as an argument for getAiredBetween and it handles the logic inside it instead. Let me know if that works here?

Copy link
Collaborator

@pushrbx pushrbx left a comment

Choose a reason for hiding this comment

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

Now it looks ok.

app/Features/QueryUpcomingAnimeSeasonHandler.php Outdated Show resolved Hide resolved
@irfan-dahir irfan-dahir merged commit 3d822c2 into master Nov 3, 2023
@irfan-dahir irfan-dahir deleted the bugfix/seasonal-102123 branch November 3, 2023 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants