Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only save session vars to call_logs #931

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Mar 17, 2023

Changes the call_logs.js_context column to a BLOB instead of TEXT, and leverages term_to_binary/1 and binary_to_term/1 to serialize the Erlang terms to binary and back.

Note: I assume that the commit was never actually released to production, as we can't save anything. I'm still cleaning up the column to NULL, not that it would break the text to binary migration but to make sure the Broker won't receive invalid data.

The call log no longer saves the js context but only the session vars, and only when needed to (otherwise it's an empty proplist). The session vars are serialized to YAML so we can process them in Ruby as well as Erlang.

The active GC now reconstructs the callback url from the project associated to the call log, and authenticates the callback (this was previously overlooked).

The js_context and callback_url columns have been dropped from the call_logs table as we don't need them anymore.

fixes #921
fixes instedd/surveda#2220

@ysbaddaden ysbaddaden added the bug label Mar 17, 2023
@ysbaddaden ysbaddaden self-assigned this Mar 17, 2023
@ysbaddaden ysbaddaden marked this pull request as draft March 17, 2023 09:26
@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Mar 17, 2023

Hum, I can do much better here:

  • We can save the session vars only + only when we have to report them (other let's not bother);
  • We can construct the callback URL from the project attributes so we don't need to save the session's callback_url, which isn't enough anyway: it's missing basic http authentication (username, password).

@ysbaddaden ysbaddaden changed the title Fix: save call_logs.js_context as binary Only save session vars to call_logs Mar 17, 2023
@ysbaddaden
Copy link
Contributor Author

Done. Now, let's test it.

@ysbaddaden ysbaddaden force-pushed the fix/921-failure-to-persist-call-logs-js-context branch from 96e68df to 67f6ea5 Compare March 21, 2023 18:05
@ysbaddaden ysbaddaden marked this pull request as ready for review March 21, 2023 18:05
Copy link
Member

@matiasgarciaisaia matiasgarciaisaia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@leandroradusky
Copy link
Contributor

Except for the comment that I've left, and as far as I can review this, looks good to me :)

Changes the call_logs.js_context column to a BLOB instead of TEXT, and
leverages `term_to_binary/1` and `binary_to_term/1` to serialize the
Erlang termns to binary and back.
The call log now longer saves the js context but only the session vars,
and only when needed to (otherwise it's an empty proplist). The session
vars are serialized to YAML so we can process them in Ruby as well as
Erlang.

The active GC now reconstructs the callback url from the project
associated to the call log, and authenticates the callback (this was
previously overlooked).

The `js_context` and `callback_url` columns have been dropped from the
`call_logs` table as we don't need them anymore.
@ysbaddaden ysbaddaden force-pushed the fix/921-failure-to-persist-call-logs-js-context branch from c7c5c3b to 835fe2f Compare March 28, 2023 10:04
@ysbaddaden ysbaddaden merged commit af30799 into master Mar 28, 2023
@ysbaddaden ysbaddaden deleted the fix/921-failure-to-persist-call-logs-js-context branch March 28, 2023 10:22
@ysbaddaden ysbaddaden restored the fix/921-failure-to-persist-call-logs-js-context branch March 28, 2023 12:00
@ysbaddaden ysbaddaden deleted the fix/921-failure-to-persist-call-logs-js-context branch March 28, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verboice # 921 (fails to save call_logs.js_context) Saving calls fails due to non-serializable js_context
3 participants