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

feat: Add configurable auto-approval #1190

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

Conversation

augustuen
Copy link

Description

Add the ability to define "rules" for auto-approval, instead of the simple yes/no system from before.

Features:

  • Approve requests based on defined conditions
  • Modify users' requests according to the set rule before approving them
    • Ex: If a movie has the christmas tag, change the root folder to christmas_movies

Screenshot (if UI-related)

bilde
bilde

To-Dos

  • Successful build pnpm build
  • Translation keys pnpm i18n:extract
  • Database migration (if required)

@fallenbagel
Copy link
Owner

fallenbagel commented Dec 28, 2024

Suggestion: Move this to settings page as a tab. As this is 100% a setting

@gauthier-th
Copy link
Collaborator

It could be cool to have the same rule system as the override rules one: #945
With that we could improve/edit the "rules editor" at the same time for both rules system.

(But we could do it in another PR)

@augustuen augustuen force-pushed the feature-auto-approval-system branch from eb7cd47 to b2351d5 Compare December 29, 2024 19:38
@augustuen
Copy link
Author

augustuen commented Dec 29, 2024

Oh, I wasn't aware that existed! That looks really awesome. One thing I would've liked is the ability to send a notification without approving the request, but I don't see it as an absolute necessity.

With "same rule system", do you mean to use the same component for both? Or something in the backend?

@gauthier-th
Copy link
Collaborator

With "same rule system", do you mean to use the same component for both? Or something in the backend?

Probably more for the frontend, but if you can think of something clean for the backend it could be useful too (as long as the abstraction for the "rule system" doesn't make things more complicated)

@augustuen
Copy link
Author

augustuen commented Jan 2, 2025

Idk if this is genius or moronic (though I'm leaning towards the latter), but: The edit modal has basically a list of variable length of different kinds of conditions, which have different UIs (some have text boxes, others have multi-selects), and I'm trying to make adding new conditions later on as simple as possible, so I had the idea of putting the UI and the logic together in the same class.

Basically, it's this:

  [...]
  public boolean isSatisfiedBy(MediaRequest request){
    (...)
  }
  public Component = () => {
    return(
      <GenreSelector
        type='tv'
        defaultValue={value}
        isMulti
        onChange={...}
       />
    );
  }
}

Both isSatisfiedBy and Component are inherited from the base, with isSatisfiedBy being called by the API/backend to process an incoming request, and Component being used in the edit modal. This would in theory mean that the core code wouldn't have to be changed to accomodate new condition types (specifications) later on, since it would focus on AutoApprovalSpecificationBase, and leave it up to the specifications themselves to decide whether a request satisfies it, and to provide the UI components that they need.

IMO, this would consolidate it into a neat little package, but I can also see it being convoluted/messy/just generally bad practice to mix logic and UI in this way.

…fications to be based on

Adds the AutoApprovalSpecificationBase entity to base other specifications on to allow generic
interfacing. Also adds UI list items for each specification.
@gauthier-th
Copy link
Collaborator

To be honest i'm not exactly sure what you mean by this mix of UI and logic, but feel free to send the updates in this PR and i'll review it.
Just my 2c about the system to better handle conditions later: i don't think this is necessary, as i doubt we'll add new conditions very often.

@augustuen
Copy link
Author

To be honest i'm not exactly sure what you mean by this mix of UI and logic, but feel free to send the updates in this PR and i'll review it.

Basically, instead of having two parts, e.g. UserSpecification and UserSpecificationComponent, one for the backend logic and one for the UI component, we'd just have UserSpecficication with a Component member (i.e. UserSpecification.Component). That way we'd be able to just assign the specific values to one object, and we wouldn't have to have a check in every UI bit for what sort of specification/condition it is and which UI component should be used.

Unfortunately, I've already moved away from this, because I tried it myself and got an issue that seemed to relate to trying to use UI components in a server object.

Just my 2c about the system to better handle conditions later: i don't think this is necessary, as i doubt we'll add new conditions very often.

That's fair, but to be fair I also figured it could simplify adding the conditions currently, instead of having to go through and update the UI components with every added condition.

Also, if possible, I'd like to ask you some questions on Discord about how Jellyseerr works/is built. I don't think they're necessary to include here in the PR.


public value: string;

public comparator = 'is';

Check warning

Code scanning / CodeQL

Useless assignment to property Warning

This write to property 'comparator' is useless, since
another property write
always overrides it.

class ReleaseYearSpecification extends AutoApprovalSpecificationBase {
public implementationName = 'releaseyear';
public comparator = 'equals';

Check warning

Code scanning / CodeQL

Useless assignment to property Warning

This write to property 'comparator' is useless, since
another property write
always overrides it.
@gauthier-th
Copy link
Collaborator

That's fair, but to be fair I also figured it could simplify adding the conditions currently, instead of having to go through and update the UI components with every added condition.

If the result is simpler yes go ahead.

Also, if possible, I'd like to ask you some questions on Discord about how Jellyseerr works/is built. I don't think they're necessary to include here in the PR.

Sure, feel free to reach us in the #development channel

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