🌐 AI搜索 & 代理 主页
Skip to content

Conversation

@chillenberger
Copy link
Contributor

@chillenberger chillenberger commented Jun 14, 2024

  • make session cookie compatible with old cookie and recover no matter what
  • use serde json to string
  • add tests

@chillenberger chillenberger requested a review from levkk June 14, 2024 23:54
@chillenberger chillenberger merged commit 23b243b into master Jun 16, 2024
@chillenberger chillenberger deleted the dan-notificatioins-backwards-compatible branch June 16, 2024 18:21
// Get the notification that triggered this call.
// Guaranteed to exist since it built the component that called this, so this is safe to unwrap.
// Get the notification that triggered this call..
// unwrap notifications if fine since we should panic if this is missing.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not be panicking if anything optional like this is missing. We should default to vec![].

];

let response = client
.get("/notifications/product/modal/remove_modal?id=1")
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like the wrong place to implement this. This should be in our web app, and we should pass it through the context into the dashboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean here. The test, or the endpoint should be in web app? I felt like both are needed here since notifications include marketing notifications.

@chillenberger chillenberger restored the dan-notificatioins-backwards-compatible branch June 17, 2024 16:11
chillenberger added a commit that referenced this pull request Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants