Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce CLI update banner #3422

Merged
merged 5 commits into from Mar 23, 2017
Merged

Conversation

@nilbus
Copy link
Contributor

nilbus commented Mar 22, 2017

This branch introduces a banner asking people to update to [currently-unreleased] cli version 2.4.0.

screenshot

I'm guessing 2.4.0 will be the version number according to semantic versioning, because master contains a new configuration option from exercism/cli#366 and debatably the -m short option in exercism/cli#359.

Feature name: advertise_cli_update

Todo:

  • Conditionally show a banner message
  • Create User migration to track when the message was dismissed, forward-compatible with #3416
  • Limit after hook from running on each request multiple times
  • Complete ClientVersion tests
  • Update client_version_notification_dismissed_at when then flash notice is dismissed
  • Test manually
    • Does not appear when logged out (guest user)
    • Disappears permanently for any user that dismisses it
    • No visible change when the feature flipper is off
@nilbus nilbus changed the title This is a work in progress. Introduce CLI update banner Mar 22, 2017
@nilbus nilbus force-pushed the nilbus:please-update-cli-banner branch 2 times, most recently from 1579832 to b13d99c Mar 22, 2017
ClientVersion will eventually impement the functionality outlined in #3416.

This simply implements the ability to check for a dismissed flash notice
on the User record for a hard-coded version number.
@nilbus nilbus force-pushed the nilbus:please-update-cli-banner branch from b13d99c to b2d252a Mar 23, 2017

def notice_when_client_outdated
unless dismissed?
"A new version of the exercism client is available (cli-2.4.0). Run <code>exercism upgrade</code> to get the latest hotness!"

This comment has been minimized.

Copy link
@kytrinyx

kytrinyx Mar 23, 2017

Member

I'm hesitant to hard-code this. I'd rather either:

  • get the most recent CLI release when the app boots up and store that in a constant that we can refer to, or
  • have a cli_versions table and populate it based on what we get from the HTTP header when people submit (only storing each version once, and with a "first seen at" timestamp).

I think that the next CLI release will be a patch release, since changing where the CLI gets the text it displays is more of an implementation detail than a new feature.

This comment has been minimized.

Copy link
@nilbus

nilbus Mar 23, 2017

Author Contributor

I agree this must stay a short-term thing. I'm just running out of time this week and want to keep it simple, so I can get to the real experiment code. #3416 exists to fix this while the experiment runs. Keep in mind that, in a pinch, we can even just turn the banner off with the feature flag, without having to synchronize a deploy of this repo.

See my edited description for why I guessed at 2.4.0, but I'd be happy to make this 2.3.1 at your discretion.

This comment has been minimized.

Copy link
@kytrinyx

kytrinyx Mar 23, 2017

Member

Oh, no you're right. The config option means we'll end up at 2.4.0. Yepp, for the experiment, let's keep it simple.

This is defined in ExercismWeb::Routes::Profile, because it's the most
logical subclass of ExercismWeb::Routes::Core that runs on every normal
web page request. Defining this before filter in Core itself causes the
before filter to run once for each of its many subclasses that load for
the page: generally each router that has a partial displayed. Sinatra!
Since the user profile nav appears on every page, this is a candidate
that works.

Also, requests for /icon come through the normal rack stack, and each of
those requests also runs before filters. I want this to run and set the
flash notice once per page, so this also checks the HTTP_ACCEPT header
to see whether or not a text/html page is being loaded.
@nilbus nilbus force-pushed the nilbus:please-update-cli-banner branch from b2d252a to f391e0c Mar 23, 2017
@nilbus nilbus force-pushed the nilbus:please-update-cli-banner branch from 860dbee to 2795143 Mar 23, 2017
@nilbus
Copy link
Contributor Author

nilbus commented Mar 23, 2017

This has a green build, is manually tested, and should be ready to go. 🎉 We can ship this now, and turn on the advertise_cli_update feature after cli merges exercism/cli#377 and releases 2.4.0.

I recommend we release the updated cli now to get people a head start on updating, so we can have as many people on at the beginning of the week as possible with the right version. Next week on Monday morning, we can turn on the feature flag that updates its call to action.

@nilbus
Copy link
Contributor Author

nilbus commented Mar 23, 2017

I just read #3416 (comment). Consider pausing until we get the naming of these fields worked out.

Rename to client_version_update_notification_dismissed_at
as per discussion in #3416.
@nilbus nilbus force-pushed the nilbus:please-update-cli-banner branch from 8657539 to b0d521f Mar 23, 2017
@kytrinyx
Copy link
Member

kytrinyx commented Mar 23, 2017

I recommend we release the updated cli now to get people a head start on updating, so we can have as many people on at the beginning of the week as possible with the right version.

Agreed. I'll get that done today.

BTW, @nilbus, if you fear that a PR has dropped off my radar for some reason, please don't hesitate to send me an email directly about it. I don't want to accidentally leave something languishing.

@kytrinyx kytrinyx merged commit 55c705c into exercism:master Mar 23, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 85.395%
Details
@kytrinyx
Copy link
Member

kytrinyx commented Mar 23, 2017

Consider pausing until we get the naming of these fields worked out.

D'oh. Once I saw the names you suggested this felt resolved to me. I forgot that the change actually has to be made.

I have not deployed this. Would you mind making an additional PR to fix the names?

@nilbus
Copy link
Contributor Author

nilbus commented Mar 23, 2017

I actually did rename it already in the branch you merged, in b0d521f. 👍

@nilbus nilbus deleted the nilbus:please-update-cli-banner branch Mar 24, 2017
@nilbus
Copy link
Contributor Author

nilbus commented Mar 24, 2017

I did rename them, but with dyslexic typos! Fix coming shortly.

facepalm

@kytrinyx
Copy link
Member

kytrinyx commented Mar 24, 2017

We're all good. It wasn't an active code path (yet).

nilbus added a commit to nilbus/exercism.io that referenced this pull request Jun 23, 2017
…i-banner"

This reverts commit 55c705c, reversing
changes made to 57b2a2c.

Conflicts:

- app/routes/profile.rb [removed relevant lines]
- db/migrate/201703212321_add_client_version_notification_dismissed_at_to_users.rb [kept]
- lib/exercism/client_version.rb [removed]
- test/exercism/client_version_test.rb [removed]

The migration was kept and will be reversed in a subsequent commit.

Closes exercism#3416
nilbus added a commit to nilbus/exercism.io that referenced this pull request Jun 29, 2017
…i-banner"

This reverts commit 55c705c, reversing
changes made to 57b2a2c.

Conflicts:

- app/routes/profile.rb [removed relevant lines]
- db/migrate/201703212321_add_client_version_notification_dismissed_at_to_users.rb [kept]
- lib/exercism/client_version.rb [removed]
- test/exercism/client_version_test.rb [removed]

The migration was kept and will be reversed in a subsequent commit.

Closes exercism#3416
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.