Remove higher-level topics from GCSE Maths Question Finder

[barna-isaac]

Despite there being possible follow-ups to this story that

• ensure context-specific question finders never show irrelevant topics, and
• disable empty topics from the main question finder as the user adds criteria,

I also just wanted to implement the changes requested by the content teams, which were to the GCSE maths question finder only.

I opted to use a list of allowed tags for filtering the possible filter choices, hoping this will be most compatible with a future implementation of those features (where these tags might come from the backend). This is a bit of a gamble: it's possible we'll never do those, and then there probably would have been easier implementations.

I started by fixing some style issues, and then added

• component tests for the Question Finder, which check the fields and topics on the A-level maths question finder (later added separate test for the GCSE maths question finder)
• unit tests for the updateTopicChoices function. Although these are a little more implementation-specific than I usually like, in this case I found they helped me understand what this function did. I also noticed that the return value of this function was a little surprising on context-specific question finders (the list of fields was duplicated in the first position, which is reserved for subjects on the main QF), so I changed this to return subjects, after a discussion with @sjd210. This is was just for the sake of consistency, as the first element is not used on context-specific question finders.

I then implemented the ability to hide disallowed topics in updateTopicChoices, and hid the higher-level topics from the GCSE Maths question finder in the last commit.

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.66%. Comparing base (99a861b) to head (82a3c11).
⚠️ Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
src/app/services/questionSearch.ts	83.33%	3 Missing ⚠️
src/mocks/filters.ts	94.73%	2 Missing ⚠️
src/app/services/constants.ts	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1802      +/-   ##
==========================================
+ Coverage   41.56%   41.66%   +0.10%     
==========================================
  Files         543      543              
  Lines       23784    23828      +44     
  Branches     7863     7035     -828     
==========================================
+ Hits         9885     9928      +43     
- Misses      13256    13857     +601     
+ Partials      643       43     -600     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[axlewin]

This works well, and fundamentally it would be fine to merge as-is. I agree that we should be aiming to make this compatible with some further improvements in the future, and it's nice to have some test coverage for this (fairly complicated) behaviour.

I'm not sure that the hardcoded exclusions list is in the most sensible place; I've tried to justify my thoughts in the comment, but let me know if you disagree.

I've also got some questions about the way the list of topics is handled in the tests, but admittedly I'm not sure that I understand the structure of these tests properly, so I might be completely wrong with my comments there.

[axlewin]

I'm not sure I understand the purpose of this enum. Is it just a subset of topic names that happen to be used in the tests?

Is there any reason we can't just use either this list or the tags service to get a list of topics when needed in the question finder tests? Hardcoding a subset of topic names here means one more place to keep updated if/when topics are added/removed/renamed, and we don't get the benefit of the tags structure to automatically find parents/children etc, which means yet more hardcoding when we want to use those relationships. It just feels quite fragile, susceptible to mistakes (see Probability typo), and introduces a lot of overhead for something that's only used in tests anyway.

I realise that not all of this was added in this PR, so maybe we do genuinely need this somewhere, in which case ignore this comment.

[axlewin]

See comment on filters.ts; can this not just be tags.getChildren(TAG_ID.number).map(t => t.title); (and similarly for the other usages)?

If this does need to use the Filter enum, I'd maybe advocate for not renaming the import to make it clearer where this comes from (I was confused about what F was when first reading this). I appreciate that would make some of these lines very long, though.

[barna-isaac]

@axlewin, I've addressed all your concerns, except the concern about the Filters enum being redundant in our tests. In person, we've already discussed that the reason I originally introduced this enum was so

• for crafting expectations, we avoid re-using code from our implementation in tagsPhy.ts, which implements the code that's being tested. If we used it for making expectations, we'd risk not noticing when the implementation in tagsPhy breaks. I'd be fine only reusing the labels though (I only wanted to avoid reusing functions like getChildren, etc.). However, I haven't found an easy way to only re-use the labels, so maybe this could remain a task for another day?
• we have a single enum for everything that might appear as a filter on the Question Finder. This includes book and difficulty selectors, which don't come from the tags.

It somewhat contradicts the argument I make above that in the tests for questionSerach.ts, I do use the tags, as well as the getById function from. I realise this is a contradiction, and can offer no resolution except that that's a unit test and I thought it was fine for that to be somewhat weaker. However, I still tried to cut down on the repetitions in that code by extracting commonly used tag sets.

Can you please review this again? If you still feel the Filters enum is something we can't live with, please feel free to play around and make any changes. I don't think I'd be against these -- it's just that I feel this is the best I could do for now.

[axlewin]

Looks good to me 👍 . I see your points about Filter, and I think it's fine to keep it in its current state; the tests already look a lot neater with the latest changes.

I'm still not sure that getContextSpecificTags really belongs in constants.ts, but that's a very minor point that I don't think it's worth blocking merging this for.