Conversation
Allow clients to define the minimum urgency of messages they wish to receive. For example, a client with low battery can request a high urgency message only until their battery is good again. Note: this is a resubmit of #815.
|
Anything is blocking ? |
|
Yes, there was a minor conflict in Cargo.toml. Just resolved it. We're a little thin right now, so getting review time is a challenge. I'll poke the other two again to see if they've got time. |
|
Forgetten? Could you take another look for a timeslot? |
jrconlin
left a comment
There was a problem hiding this comment.
I know I am probably seeming like a stickler about it, but putting the urgency elements behind a non-default feature flag means that it's not supported, meaning that we do not need to have unit and integration tests for this since it's not part of the critical code path.
| use autoconnect_settings::{AppState, Settings}; | ||
| use autopush_common::{ | ||
| db::User, | ||
| db::{Urgency, User}, |
There was a problem hiding this comment.
Not fully isolated:
| db::{Urgency, User}, | |
| #[cfg(feature = "urgency" )] | |
| use autopush_common::Urgency; | |
| use autopush_common::{ | |
| db::User, | |
| notification::Notification, | |
| util::{ms_since_epoch, user_agent::UserAgentInfo}, | |
| }; |
| /// First time a user has connected "today" | ||
| pub emit_channel_metrics: bool, | ||
| /// Minimum urgency | ||
| pub min_urgency: Urgency, |
There was a problem hiding this comment.
| pub min_urgency: Urgency, | |
| #[cfg(feature = "urgency")] | |
| pub min_urgency: Urgency, |
| check_storage: false, | ||
| old_record_version: false, | ||
| emit_channel_metrics: false, | ||
| min_urgency: Urgency::VeryLow, |
There was a problem hiding this comment.
| min_urgency: Urgency::VeryLow, | |
| #[cfg(feature = "urgency")] | |
| min_urgency: Urgency::VeryLow, |
| use crate::error::{SMError, SMErrorKind}; | ||
| use crate::{ | ||
| error::{SMError, SMErrorKind}, | ||
| identified::Urgency, |
There was a problem hiding this comment.
#[cfg(feature = "urgency")]
use crate::identified::Urgency;
| messages.retain(|msg| { | ||
| if !msg.expired(now_sec) { | ||
| return true; | ||
| if let Some(headers) = msg.headers.as_ref() { |
There was a problem hiding this comment.
#[cfg(feature = "urgency")]
if let Some(headers) = msg.headers.as_ref() {
return Urgency::from(headers.get("urgency")) >= self.flags.min_urgency;
} else {
return true;
}
}
#[cfg(not(feature="urgency"))]
return true;
| use autoconnect_settings::{AppState, Settings}; | ||
| use autopush_common::{ | ||
| db::{User, USER_RECORD_VERSION}, | ||
| db::{Urgency, User, USER_RECORD_VERSION}, |
There was a problem hiding this comment.
#[cfg(feature = "urgency")]
use autopush_common::db::Urgency
use autopush_common::{
db::{User, USER_RECORD_VERSION},
| .record_version | ||
| .is_none_or(|rec_ver| rec_ver < USER_RECORD_VERSION), | ||
| emit_channel_metrics: user.connected_at < ms_utc_midnight(), | ||
| min_urgency: user.urgency.unwrap_or(Urgency::VeryLow), |
There was a problem hiding this comment.
| min_urgency: user.urgency.unwrap_or(Urgency::VeryLow), | |
| #[cfg(feature = "urgency")] | |
| min_urgency: user.urgency.unwrap_or(Urgency::VeryLow), |
| use std::cmp::min; | ||
| use std::collections::HashMap; | ||
| use validator::Validate; | ||
| use validator::{Validate, ValidationError}; |
There was a problem hiding this comment.
Again, we want to isolate the urgency stuff to a feature flag, so the bits that are urgency based need to be behind that flag.
|
Sorry that i am so annoying. Is there anything blocking? Could i somewhere help to get this over the finish line? Is there any reason why this is not merged? |
|
Well, currently, one of the biggest blockers is probably just me. Mostly because I've got a few higher priority things I am working on that I really need to get resolved. (I'm very close to doing that, and yes, I'm also very frustrated that I'm not able to get them finished, but complex systems are complex and there are blockers outside of my control). Once those things are done, I can look at the conflicts and try to resolve them (or you can, I don't expect any other conflicts), and after that's done I can merge, tag, and hopefully release shortly afterward. |
|
Hmm, so circling back on this and it looks like the redis library changed how it recommends async connections, which will require redoing some of the reliability stuff as well. |
|
Thank you for this feedback. |
|
Ok, so took a swing at merging and updating a few things to get this up. I'm going to convert to a draft because I'm not sure that I got things right, and I'd like to have some testing as well. |
|
Could i help out? |
Allow clients to define the minimum urgency of messages they wish to receive. For example, a client with low battery can request a high urgency message only until their battery is good again.
Note: this is a resubmit of
#815.
cc https://github.com/p1gp1g