Feat: Add urgency support

[p1gp1g]

It allows 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.

[jrconlin]

Looks interesting, but my major concern is that we're changing the client protocol, and we'd want to make sure that we don't break anything on the current or legacy UA. I'd be more comfortable landing this if it was behind a feature flag so that we could test it in isolation from production or stage.

Obviously, this change would also require updates to the UA code to actually consume and utilize the new Urgency message, and I don't know what priority that might be with that team.

[jrconlin]

We shouldn't eat this error.

[p1gp1g]

I have done it to return a 404 if the user isn't saved yet (if they send urgency before registering to a channel). Do you have another way in mind ?

[jrconlin]

The problem is that if update_user(..) fails, we ought to know why (e.g. there may be a database issue happening that we should track. If the update fails, we should record the error and pass back a result that's meaningful to the caller. That's mostly handled by the existing error processing, but is_ok() interrupts that.

Granted, having this function return a context free status 500 doesn't really help much either. (How did we get this? How do we fix the problem? etc.)

If we get a 500, I'd say that all bets are off and that we return a proper 500 that closes the connection.

That leaves either a list of pending ServerMessages, or a single ServerMessage with a 404 status.

I'd probably do something like (the following is me just writing code. Please correct any glaring bugs):

if let Some(mut user) = self.app_state.db.get_user(&self.uaid).await? {
    // If the user hasn't set a minimum urgency yet, they receive all messages,
    // which is equivalent to setting very-low as a minimum
    let current_urgency = user.urgency.unwrap_or(Urgency::VeryLow);

    // We update the user
    user.urgency = Some(new_min);
    user.connected_at = ms_since_epoch();

    // if new urgency < previous: fetch pending messages
    self.app_state.db.update_user(&mut user).await?;

    let mut res = vec![ServerMessage::Urgency { status: actix_web::http::StatusCode::OK }];
    // if new urgency < previous: fetch pending messages
    if new_min < current_urgency {
          self.ack_state.unacked_stored_highest = None;
          self.current_timestamp = None;
          res.append(&mut self.check_storage().await?);
      }
      // We return the Urgency Ack + pending messages
      return Ok(res); 
} else {
      return Ok(vec![ServerMessage::Urgency { status: actix_web::http::StatusCode::NOT_FOUND }])
}

[p1gp1g]

OK I see, I've pushed a patch. Regarding the status code, I prefer not mixing types, all other ServerMessage's status are u32, and I think they should migrated to StatusCode in the same time.

[jrconlin]

You're right. That was more me thinking out loud that we should be consistent about using StatusCode when possible, but that should not be part of this PR.

[p1gp1g]

Thank you for your review, I've introduced a new feature for the urgency and addressed 2 other comments

[jrconlin]

Thanks! Looks good. I'm going to do a quick bit of research on the client code to see if there are any potential impacts, but I like the approach you took.

Sigh, Also just noted that this commit is not signed, (see our Contributing doc. I also just realized that we never linked to what "sign" means, which might explain a few things ( sighs ). Unfortunately, we cannot land unsigned commits, so you will need to close this PR in favor of one that is signed, or once we get approval, we can resubmit this for you. My apologies for not catching that earlier.

[p1gp1g]

I have signed the commits. Let me know how your tests go with the client, there shouldn't be any issue since the ServerMessage::Urgency is only send after a ClientMessage::Urgency

FYI, I've written a UnifiedPush (~webpush for mobile applications) distributor that uses autopush (Sunup), and I did the tests with it. By the way, I'll try to add UnifiedPush support to Firefox-Android soon

[jrconlin]

I'm going to give one of the other dev's a chance to look this over before I land it.

Thank you for this!