feat: track allow list answers

[Mike-Heneghan]

What

• Track the node_fn in analytics_log table to tie together nodes and the passport variables they relate to
• Track allow_list_answers in analytics when a user directly answers a question

Why

• Adding node_fn makes it easier to reason about how nodes can affect the passport.
• Metrics requested the allow_list_answers

Note

• This supersedes feat: track the answers to allow_list questions in analytics_logs #2487

[github-actions]

🤖 Hasura Change Summary compared a subset of table metadata including permissions:

Updated Tables (1)

• public.analytics_logs permissions:

  	insert	select	update	delete
  public	➕/➖	➕/➖	➕/➖

  3 added column permissions

  	insert	select	update
  public	➕ node_fn	➕ node_id	➕ allow_list_answers

[Mike-Heneghan]

Track answers to allow-listed questions for guidance services (non save & return)

[github-actions]

Removed vultr server and associated DNS entries

[DafyddLlyr]

A few quick comments - can't currently test the pizza as I'm getting CORS issues - I suspect it just needs a rebase as this was something I broke when working on auth before Christmas I think! 👍

[DafyddLlyr]

nit: I'm not exactly sure how, but it would be nice to handle this repeated condition throughout this provider. Even if we just reverse it and make it a guard clause I think it would be a bit more readable / meaningful

[Mike-Heneghan]

I think this is a great shout, I think theres a lot of nesting in the file which can make it a bit harder to parse.

I've added more guard clauses and tried to see if I could update the conditionals to be slightly more readable.

It's on this commit and it would be great to get your opinion on the changes: 7bb5ac8

[DafyddLlyr]

This is nice and a good improvement - more readable for sure 👍

What do you think about a change along these lines -

function trackEvent(event: MyEnumOfEventTypes): void {
  skipUpdateIfNotTracking();

  switch (event) {
    case "helpClick":
      // Handle helpClick event
      break;
    case "nextStepsLinkClick":
      // Handle nextStepsLinkClick event
      break;
    // Add more cases as needed
    default:
      // Handle default case or throw an error
      break;
  }
}

The benefits I'd see of this would be -

• You can't miss the guard clause skipUpdateIfNotTracking() when adding a new event
• It's a bit more self documenting - when we come back and add a new event type it should be a simpler pattern to follow (add event enum, add case in this controller function)
• We could drop the "child" functions like trackHelpClick() from the createContext type definition and only expost the "parent" trackEvent() controller.

Super happy for the response to be "not worth it", just wanted to run the idea by you. This could also be a small refactor on a follow up PR! 👍

[Mike-Heneghan]

This is a really interesting suggestion! I'd noticed there was quite a lot of very similar patterns and something like this could potentially resolve that. Some of the events have specific requirements for further info but I'm sure this could be managed as well.

As this piece of work has been on the burner for a long time I think I'd opt to as you say leave it for now and potentially follow up with a refactor PR in the future. I feel like I'm almost too close to the analyticsProvider file and a bit of time away from it could really help to refactor with fresh eyes

[Mike-Heneghan]

A few quick comments - can't currently test the pizza as I'm getting CORS issues - I suspect it just needs a rebase as this was something I broke when working on auth before Christmas I think! 👍

Thank you very much for the review, I'll have a look at the comments now.

Strangely with testing on the pizza I wasn't seeing any CORS issues before the holidays and it still seems to be working for me 🤔 I'll double check once I've pushed commits and rebased though.

[DafyddLlyr]

Strangely with testing on the pizza I wasn't seeing any CORS issues before the holidays and it still seems to be working for me 🤔 I'll double check once I've pushed commits and rebased though.

Ah sorry about this - there's certainly a chance I had a local cookie or something else going on - tried again and it works a charm for me now! Will take another look very shortly 👍

[Mike-Heneghan]

Strangely with testing on the pizza I wasn't seeing any CORS issues before the holidays and it still seems to be working for me 🤔 I'll double check once I've pushed commits and rebased though.

Ah sorry about this - there's certainly a chance I had a local cookie or something else going on - tried again and it works a charm for me now! Will take another look very shortly 👍

Thanks for checking again @DafyddLlyr, absolutely no rush as I have some local setup issues to squash before I rebase and update as per your comments. As the testing is quite laborious it might be worth waiting until the branch is updated rather than having to do it twice 😅

[DafyddLlyr]

Changes look great! Once small comment on abstracting away a few things but it's non-blocking 👌