solymosi / npu

Neptun PowerUp! - Felturbózza a Neptun-odat
MIT License
274 stars 47 forks source link

Subject which is successfully passed but retake is subscribed gets hidden #28

Closed whisperity closed 6 years ago

whisperity commented 6 years ago
  1. Take an exam. Get a passing grade.
  2. Because passing grade exists, the subject is marked completed, the hide filter filters all exams from it out.
  3. Sign up for another exam from the same subject (with the intent of improving your grade)

After this, the exam you signed up for will be yellow, and the subject in the drop-down will be yellow. This is expected behaviour.

Other exams from this subject in particular which you did not sign up for will be either green (because completed) or no highlight at all. (I cannot confirm which happens because we have a Neptun shutdown for the weekend. Will confirm later, if neccessary). This isn't that much of a problem, the good thing is that the exam you are retaking is properly highlighted.

However, when someone clicks Filter completed subjects, the following happens:

I think the fact that someone is having an exam from a subject not yet taken should override the "completed" factor when it comes to filtering. So in this retake case, the completion filter should not filter out any of the remaining exam dates from said subject.

whisperity commented 6 years ago

@solymosi I'd sit down to fix this but unfortunately the exams themselves are pretty hard and my exam period is pretty packed with other things. 🙁

solymosi commented 6 years ago

I think I know why this is happening, it should be simple to fix. I'll have a go at it.

whisperity commented 6 years ago

Considering this issue, I was looking at the code yesterday (literally can't study the afternoon after taking an exam...) and this popped up:

https://github.com/solymosi/npu/blob/e2f5ef91c4bad2d7ab40ecfbd2ff70f448de5241/npu.user.js#L1164-L1165

It might be my misunderstanding to JavaScript, but these lines look packed. Is this a clever if statement, or what are they supposed to do? The expressions' value is discarded here. Even if it is clever, does this result in a more optimised execution? Lines like these sound the alarm in me, because at first glance they feel unreadable.

solymosi commented 6 years ago

Yeah, this is a "clever" if statement using the short-circuit mechanism of logical operators. It seems to be commonly used in various JS projects that I've seen, although I do agree that it hurts readability somewhat.

solymosi commented 6 years ago

You mentioned that the drop-down filtering works fine. This is because its logic follows these rules:

  1. Establish the final subject class using the precedence rules
  2. Once we know the final class, hide the entry if it's npu_completed

If the subject as at least one subscribed exam, the final class will be npu_subscribed and we don't hide it.


However, here this rule is not followed:

/* The class 'npu_subscribed' has a higher precedence than these, so it
   will not get overwritten */
rowClass = selectImportantClass(rowClass, npu.isPassingGrade(grade) && "npu_completed");
rowClass = selectImportantClass(rowClass, npu.isFailingGrade(grade) && "npu_failed");
npu.isPassingGrade(grade) &&
    row.add(subRow)[filterEnabled ? "addClass" : "removeClass"]("npu_hidden");

Note how we calculate the correct rowClass based on the preference rules, but then not use it to determine if the row should be hidden. So despite rowClass containing npu_subscribed, if the last mark row has a passing grade, we hide the row anyway.

I've pushed a simple fix to #27. Can you please test it (and perhaps also the other fixes)?

solymosi commented 6 years ago

By the way, I never would have guessed that such a trivially-sounding feature will have so many edge cases and would therefore require so many little fixes 😄 🔨

whisperity commented 6 years ago

(Yeah, that's what you get for parsing HTML and no useful API 😦 )

I've tested the feature but it doesn't seem to work very well.

What works: the exam I am taking is not filtered, as expected. What does not work: the other possible exams from the subject are still green and hidden if I turn on the filtering.

whisperity commented 6 years ago

But if it is too much of an extra cycle to "save" that we have an exam to be taken (and use this information to override the other exam dates from this subject), then I'm okay with this.

solymosi commented 6 years ago

Why would the other possible exams for a subject not be green? The yellow mark is supposed to show that you are actually subscribed to that exam - otherwise how could you tell? Hiding the rest is the expected behavior in my view since the subject is already completed after all.

solymosi commented 6 years ago

Release done 🎉 Thanks for reporting this bug!