nusmodifications / nusmods

🏫 Official course planning platform for National University of Singapore.
https://nusmods.com
MIT License
562 stars 271 forks source link

Prerequisite check for AT LEAST n prefixed modules not working as intended #3740

Open Wxy2003-xy opened 1 week ago

Wxy2003-xy commented 1 week ago

Describe the bug

A clear and concise description of what the bug is.

To Reproduce

Steps to reproduce the behavior:

  1. Plan for a course with prereq tree contain "nOf": [ n, [ "PREFIX%:GRADE" ] ] example: JS4207
  2. fill in prerequisite modules accordingly example: Japanese 1 - 5, any 3 JS coded modules

    Expected behavior

JS4207 should pass prerequisite check

Screenshots

Image 6-7-24 at 09 21

Desktop (please complete the following information):

Additional context

Found out this when testing our orbital project nusplanner

ravern commented 1 week ago

I can reproduce this.

We just merged in a PR to update the pre-requisite check (#3737), but seems like it doesn't work specifically in the AT LEAST N prefixed modules case.

Thanks for filing this.

ChillinRage commented 1 week ago

Hi can I check if there are any modules that has prerequisites that contains both specific and prefix% codes in one nOf? (i.e. at least 3 of CS3123/CS2% mods) ?

Because if the only combination is "at least N of " followed by only 1 prefix% code (like in JS4207), then I can try to come up with a solution for it without having to change the walkTree function too much.

Wxy2003-xy commented 5 days ago

Hi can I check if there are any modules that has prerequisites that contains both specific and prefix% codes in one nOf? (i.e. at least 3 of CS3123/CS2% mods) ?

Because if the only combination is "at least N of " followed by only 1 prefix% code (like in JS4207), then I can try to come up with a solution for it without having to change the walkTree function too much.

I'm not sure if there exists such case, but I'd like to propose a solution we used, which is to separate the match checking logic from the recursive check tree function, and handle wildcard case there.

ChillinRage commented 5 days ago

I'm not sure if there exists such case, but I'd like to propose a solution we used, which is to separate the match checking logic from the recursive check tree function, and handle wildcard case there.

Hm if you would like, you can try to create a PR for the proposed solution.