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

Created 'OR' rule #48

Merged
merged 8 commits into from
Aug 14, 2023
Merged

Created 'OR' rule #48

merged 8 commits into from
Aug 14, 2023

Conversation

Bullrich
Copy link
Contributor

@Bullrich Bullrich commented Aug 11, 2023

Hello, and welcome to Javier's list of new changes. Today, we have a new feature: The OR rule

List of changes

  • Created OR rule
    • This resolves Create OR rule #12
    • The OR rule verifies that one of the conditions have been fulfilled.
      • When a condition was fulfilled, it continues the loop to the next rule
        • Should this be a "break" instead?
      • When the OR rule fails, it shows all the users it needs a review but it only shows the amount of missing reviews using the sub condition with the minimum amount of reviews needed.
      • Created lots of test.
        • This are mainly a copy and modification of AND as they share logic and structure.

@Bullrich Bullrich added this to the Project launch milestone Aug 11, 2023
@Bullrich Bullrich requested a review from a team as a code owner August 11, 2023 15:51
@Bullrich Bullrich self-assigned this Aug 11, 2023
@Bullrich Bullrich linked an issue Aug 11, 2023 that may be closed by this pull request
@rzadp
Copy link
Contributor

rzadp commented Aug 14, 2023

When a condition was fulfilled, it continues the loop to the next rule
Should this be a "break" instead?

If I understand the context, I think it shouldn't be a break, continue is correct.
I guess the natural assumption is that all of the top-level rules are implicitly treated as AND.

} else if (type === "and") {
validatedConfig.rules[i] = validate<AndRule>(rule, andRuleSchema, { message });
} else if (type === "and" || type === "or") {
validatedConfig.rules[i] = validate<AndRule>(rule, otherRulesSchema, { message });
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda feels like these rules can be joined so the validation would work in one execution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am afraid I don't follow. What do you mean by joined? Could you please show me an example?

I want to keep the "else" to throw if the rule is invalid, but overall, I don't worry much about removing the hardcoded code as, maybe with one or two exception, I don't think we will be adding more cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, you validate config, and then validate each rule independently.
Can't we just join it all in a single schema to validate in one call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I see your point.

It is because the "Basic" rule has different parameters, so I thought it would be easier to do:

if (rule.type === "basic")
    basicCheck();
else if (rule.type !== "basic")
    notBasicCheck();

And that's is simply because I don't know how to make a conditional rule with JOI in which:

  1. If the type string has the value basic it should use a particular schema type
  2. if the type string does not have the value basic it needs to have a different schema validating it.

I don't know how to do a conditional check like that. If you do know, please let me know, as I have spend some time researching that and decided to go for the simplest solution instead of fighting for the most elegant one.

I'll be merging this PR as the union should be handled in a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean like Joi.alternatives()?

const BasicSchema = Joi.object({
   type: "basic",  
   data: Joi.object({a: Joi.string()})
  });

const NonBasicSchema = Joi.object({
    // I'm personally more inclined to have positive checks instead
    type: Joi.any().not("basic"), 
    data: Joi.object({b: Joi.string()})
  });

Joi.alternatives().try(BasicSchema, NonBasicSchema);

@Bullrich Bullrich enabled auto-merge (squash) August 14, 2023 09:33
Co-authored-by: Przemek Rzad <roopert7@gmail.com>
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.

Create OR rule
3 participants