MNTOR-3848: Fix invalid assertions #5521
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
Knex's returning() API is intended for INSERT, UPDATE and DELETE calls, not SELECTs: https://knexjs.org/guide/query-builder.html#returning I've updated the .where() queries to replace the returning() calls, i.e. by making them select all columns. I then also removed the type assertions, so as to not indicate that rows will always be returned even in cases where they might not be available. The type assertion also pretended that the `monthly_monitor_report_free` column could only be `boolean | undefined`, even though it can be `null` too (see `addUnsubcribeTokenForSubscriber`, which sets it to `null`).
Vinnl
force-pushed
the
MNTOR-3848-invalid-assertions
branch
from
dd9d2cf to
abe2549
Compare
|
Preview URL 🚀 : https://blurts-server-pr-5521-mgjlpikfea-uk.a.run.app |
4 tasks
Base automatically changed from
MNTOR-3848/optional-chaining-for-monthly_monitor_report
to
main
January 22, 2025 18:10
rhelmer
approved these changes
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References:
Jira: MNTOR-3848
Figma:
Description
Builds on #5511, but addressing the root cause.
Knex's returning() API is intended for INSERT, UPDATE and DELETE calls, not SELECTs:
https://knexjs.org/guide/query-builder.html#returning
I've updated the .where() queries to replace the returning() calls, i.e. by making them select all columns.
I then also removed the type assertions, so as to not indicate that rows will always be returned even in cases where they might not be available.
Also, the type assertion pretended that the
monthly_monitor_report_freecolumn could only beboolean | undefined, even though it can benulltoo (seeaddUnsubcribeTokenForSubscriber, which sets it tonull).And finally, the actual root cause of the issue, the return type indicated that it would never return
undefined, even though it's returningres?.[0]. I've updated that, and then fixed type errors to deal with the potentially undefined value. Question: I assumed that users without stored preferences are defaulted to be opted-in, is that correct?(And actually I think they used to default to opted out, through undefined resolving to
false. My?? truemakes them default totrue.)How to test
I'm not sure how to reproduce the original issue, so I don't know how to verify that this fixes it, but I'm fairly sure that the unit tests show what was reported.
But if someone has reproduced the issue and could verify that this fixes it, that'd be great.
Checklist (Definition of Done)