Closed
Bug 1039987
Opened 8 years ago
Closed 7 years ago
Notify the user when resetting the hawk session token for non-accounted clients
Categories
(Hello (Loop) :: Client, defect, P3)
Hello (Loop)
Client
Tracking
(Not tracked)
RESOLVED
WORKSFORME
backlog | backlog+ |
People
(Reporter: standard8, Unassigned)
References
Details
(Whiteboard: [p=1][string landed, patch todo][loop-uplift])
Attachments
(1 file)
1.58 KB,
patch
|
NiKo
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•8 years ago
|
||
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.
Comment 2•8 years ago
|
||
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•8 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)
Comment 4•8 years ago
|
||
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
Comment 5•8 years ago
|
||
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
Comment 6•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
(needinfo to Darrin to see what he thinks of the suggested text).
Comment 10•8 years ago
|
||
Please needinfo Arcadio or Matej for UI copy related questions.
Flags: needinfo?(dhenein) → needinfo?(Mnovak)
Comment 11•8 years ago
|
||
(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•8 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•8 years ago
|
||
Waiting on bug 1002416 to do this, as that has restructured the error handling and notifications.
Depends on: 1002416
Reporter | ||
Comment 14•8 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•8 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•8 years ago
|
Whiteboard: p=1 → [p=1][string landed, patch todo]
Reporter | ||
Updated•8 years ago
|
Keywords: leave-open
Updated•8 years ago
|
Whiteboard: [p=1][string landed, patch todo] → [p=1][string landed, patch todo][loop-uplift]
Target Milestone: mozilla34 → mozilla35
Comment 18•8 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•8 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)
Comment 20•8 years ago
|
||
(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•8 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•8 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•8 years ago
|
backlog: Fx37? → Fx37+
Priority: -- → P2
Updated•8 years ago
|
Priority: P2 → P1
Updated•8 years ago
|
Priority: P1 → P2
Updated•8 years ago
|
backlog: Fx37+ → Fx38?
Priority: P2 → P3
Comment 23•8 years ago
|
||
#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•8 years ago
|
backlog: Fx38? → backlog+
Rank: 35
Flags: firefox-backlog+
Comment 24•7 years ago
|
||
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•7 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: 7 years ago
Flags: needinfo?(standard8)
Resolution: --- → WORKSFORME
Comment 26•5 years ago
|
||
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.
Description
•