Notify the user when resetting the hawk session token for non-accounted clients

RESOLVED WORKSFORME

Status

defect
P3
normal
Rank:
35
RESOLVED WORKSFORME
5 years ago
Last year

People

(Reporter: standard8, Unassigned)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=1][string landed, patch todo][loop-uplift])

Attachments

(1 attachment)

Reporter

Description

5 years ago
In bug 1038699 & others, we reset the hawk session token if there's a problem with the token, and we can't recover. This will mean that the user's previously sent urls will be invalid.

Whilst this generally shouldn't happen we should still have a notification to the user if it does happen.

From bug 1038699, the recommended UX is:

- Display a warning icon in the toolbar
- In the panel, display a warning.
Also if the SimplePush URL is still the same, we could get notified of a call without seeing it to the GET /calls endpoint.

I suppose in case of new hawkToken we should register a new SimplePush URL.
This feels like an edge case, so I'd like to move it to fx35, but I want to talk to Mark first.
Flags: needinfo?(standard8)
Priority: P2 → P5
Reporter

Comment 3

5 years ago
Hmm, this is pretty bad if we don't it and manage to hit it - the user would have no idea that their previously sent urls are no longer valid.
Flags: needinfo?(standard8)
Thanks for the info, Mark.  I think we could ship without this, but it would good to get this into Fx34.  So I'm changing the priority to P2.
Priority: P5 → P2
I'd like to get this into Fx 34 if possible.  

Darrin -- Would we require new strings for this or can we reuse strings from other mock ups?
Flags: needinfo?(dhenein)
Priority: P2 → P1
Whiteboard: p=1
I don't think I had any real errors in the spec/mockup, as none were enumerated in the original stories.

I'm thinking we'll need something like "Session Expired - Please login again."... does this make sense as a user-facing error for this issue?
Flags: needinfo?(dhenein)

Comment 7

5 years ago
HI Mark - does the wording Darrin put into comment 6 work for this bug?  if yes then we have the missing info for when this bug is ready.
Flags: needinfo?(standard8)
Reporter

Comment 8

5 years ago
Not quite, the text is assuming we're logged in, but this is for the anonymous case.

I would suggest something like "Session Expired, all previously generated urls are no longer valid".
Flags: needinfo?(standard8) → needinfo?(dhenein)
Reporter

Comment 9

5 years ago
(needinfo to Darrin to see what he thinks of the suggested text).
Please needinfo Arcadio or Matej for UI copy related questions.
Flags: needinfo?(dhenein) → needinfo?(Mnovak)
(In reply to Mark Banner (:standard8) from comment #8)
> Not quite, the text is assuming we're logged in, but this is for the
> anonymous case.
> 
> I would suggest something like "Session Expired, all previously generated
> urls are no longer valid".

Maybe this could use a little more explanation:

Session expired. All URLs you have previously created and shared are no longer valid.

OR:

Session expired. All URLs you have previously created and shared will no longer work.

WDYT?
Flags: needinfo?(Mnovak)
Reporter

Comment 12

5 years ago
(In reply to Matej Novak [:matej] from comment #11)
> Session expired. All URLs you have previously created and shared will no
> longer work.

This sounds reasonable, lets go with this one.
Reporter

Comment 13

5 years ago
Waiting on bug 1002416 to do this, as that has restructured the error handling and notifications.
Depends on: 1002416
Reporter

Comment 14

5 years ago
This adds the required string for now. We should be able to land the full patch in the next day or so, once bug 1002416 lands.
Attachment #8482316 - Flags: review?(nperriault)
Comment on attachment 8482316 [details] [diff] [review]
[checked in] Add string for notifying the user that the session had expired.

Review of attachment 8482316 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8482316 - Flags: review?(nperriault) → review+
Reporter

Comment 16

5 years ago
Comment on attachment 8482316 [details] [diff] [review]
[checked in] Add string for notifying the user that the session had expired.

https://hg.mozilla.org/integration/fx-team/rev/1230a7a5c04b
Attachment #8482316 - Attachment description: Add string for notifying the user that the session had expired. → [checked in] Add string for notifying the user that the session had expired.
Reporter

Updated

5 years ago
Whiteboard: p=1 → [p=1][string landed, patch todo]
Reporter

Updated

5 years ago
Keywords: leave-open

Updated

5 years ago
Whiteboard: [p=1][string landed, patch todo] → [p=1][string landed, patch todo][loop-uplift]
Target Milestone: mozilla34 → mozilla35

Comment 18

5 years ago
this is marked leave open - are we still going to try and check in?  the string was added - so what else remains for the work?  is this fx35 or fx36
backlog: --- → Fx35?
Flags: needinfo?(standard8)
Target Milestone: mozilla35 → ---
Reporter

Comment 19

5 years ago
(In reply to sescalante from comment #18)
> this is marked leave open - are we still going to try and check in?  the
> string was added - so what else remains for the work?  is this fx35 or fx36

Well, it would be to do the work, but I think MattN did this in bug 1047164.

Matt, do you think there's anything else to do here? Was there a follow-up somewhere for making it stay on the screen for a longer time?
Flags: needinfo?(standard8) → needinfo?(MattN+bmo)
(In reply to Mark Banner (:standard8) from comment #19)
> (In reply to sescalante from comment #18)
> > this is marked leave open - are we still going to try and check in?  the
> > string was added - so what else remains for the work?  is this fx35 or fx36
> 
> Well, it would be to do the work, but I think MattN did this in bug 1047164.
> 
> Matt, do you think there's anything else to do here? Was there a follow-up
> somewhere for making it stay on the screen for a longer time?

Bug 1076652 was filed for having it stay around.

The following wasn't implemented  (only showing of the warning bar was) so this bug could morph to cover that:

(In reply to Rémy Hubscher (:natim) from comment #1)
> Also if the SimplePush URL is still the same, we could get notified of a
> call without seeing it to the GET /calls endpoint.
> 
> I suppose in case of new hawkToken we should register a new SimplePush URL.
Flags: needinfo?(MattN+bmo)

Comment 21

5 years ago
Mark, as we comfortable putting until 37 rather than uplifting for Fx35 based on infrequency.  Please let me know if we caused heartburn putting off uplifting this last piece Matt mentioned.
backlog: Fx35? → Fx37?
Flags: needinfo?(standard8)
Priority: P1 → --
Reporter

Comment 22

5 years ago
With the server guys talking about database migrations and potentially not migrating data etc, I'm not keen on delaying this, but given that they seem to be trying to preserve data, then hopefully its ok.
Flags: needinfo?(standard8)

Updated

5 years ago
backlog: Fx37? → Fx37+
Priority: -- → P2

Updated

5 years ago
Priority: P2 → P1

Updated

5 years ago
Priority: P1 → P2

Updated

5 years ago
backlog: Fx37+ → Fx38?
Priority: P2 → P3
#include <std/rant_about_loop_is_not_anonymous>
Summary: Notify the user when resetting the hawk session token for anonymous clients → Notify the user when resetting the hawk session token for non-accounted clients

Updated

4 years ago
backlog: Fx38? → backlog+
Rank: 35
Flags: firefox-backlog+
I'm a bit confused about the status on this one are we waiting on bug 1076652 to be implemented?
Flags: needinfo?(standard8)
Reporter

Comment 25

4 years ago
(In reply to Romain Testard [:RT] from comment #24)
> I'm a bit confused about the status on this one are we waiting on bug
> 1076652 to be implemented?

Re-reading this, I think we just need bug 1076652 implementing, so I'll close this as WFM.

The comment from Remy in comment 20 I don't think applies - if we register and find the token is invalid, then the registration won't have happened, so we'll clear the token and re-register with the same push url. However, as that hasn't registered, that should be fine (and even if it isn't we are able to cope with spurious push notifications).
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(standard8)
Resolution: --- → WORKSFORME
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.