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!

[p1gp1g]

@jrconlin For information, how much time do you give them to review it ?

[jrconlin]

Well, there's an outstanding conflict in the PR right now, so it's not going to land until that's resolved. Since this is isolated anyway, I'll land it after that's done and I give it a quick re-review.

[p1gp1g]

I have rebased the branch

[jrconlin]

Sigh. Just noticed that these commits aren't signed. We require all commits to be signed before we can accept them. (I've tried to make that very clear in the docs and PR templates, but I guess there's still work to do)

Sadly, that means we can't accept this PR as is (even one unsigned commit corrupts things.)

You can either close this PR and resubmit it or I can submit it on your behalf, but I'll need one of the other core folk to sign off on it.

[p1gp1g]

I have squashed the branch and signed the commit

[jrconlin]

Closing this in favor of #841 because of CI and signature issues.