Skip to content

JITM: Fix JITM display when the first result has been dismissed.#8245

Merged
oskosk merged 1 commit intomasterfrom
try/jitm-fix
Dec 20, 2017
Merged

JITM: Fix JITM display when the first result has been dismissed.#8245
oskosk merged 1 commit intomasterfrom
try/jitm-fix

Conversation

@kwight
Copy link
Copy Markdown
Contributor

@kwight kwight commented Nov 24, 2017

On a page where two JITMs exist, but the first has been dismissed, the response back from the API here will be an object with a key of 1:

{ 1: [...] }

...instead of the zero-indexed array expected in the code handling the response, giving an uncaught type error (and no JITM displayed).

screen shot 2017-11-24 at 4 25 59 pm

This PR hacks Humpty back together again, but I'm not sure how better to handle it. It seems we should be leaving the response coming from the API alone, to keep open the possibility of handling multiple JITMs on the client in the future. This PR also doesn't take into account any other variations that may creep in to the client somehow. I'm also unclear as to how dismissals are handled, or where the variation between the return of get_jitm() on the server side (an array with two values) and the client-side response (object with only one value) happens.

Any suggestions @withinboredom ?

@kwight kwight requested a review from a team as a code owner November 24, 2017 23:39
@kwight kwight requested a review from withinboredom November 24, 2017 23:44
@kwight kwight added [Feature] JITM Just In Time Messages - pop-up tips and suggestions that appear on the dashboard and sidebar. Bug When a feature is broken and / or not performing as intended labels Nov 24, 2017
@kwight kwight requested a review from a team November 24, 2017 23:46
@dereksmart dereksmart added the [Status] Needs Review This PR is ready for review. label Nov 28, 2017
@kwight
Copy link
Copy Markdown
Contributor Author

kwight commented Dec 11, 2017

@withinboredom any chance to look at this?..

Copy link
Copy Markdown
Contributor

@withinboredom withinboredom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

@kwight kwight added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Dec 11, 2017
@oskosk oskosk added this to the 5.7 milestone Dec 20, 2017
Copy link
Copy Markdown
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@oskosk oskosk merged commit 11164da into master Dec 20, 2017
@oskosk oskosk deleted the try/jitm-fix branch December 20, 2017 15:58
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug When a feature is broken and / or not performing as intended [Feature] JITM Just In Time Messages - pop-up tips and suggestions that appear on the dashboard and sidebar.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants