Introduce CLI update banner #3422

Merged
merged 5 commits into from Mar 23, 2017

Conversation

Projects
None yet
2 participants
@nilbus
Member

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 from This is a work in progress. to Introduce CLI update banner Mar 22, 2017

Implement a preliminary version of ClientVersion
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.
lib/exercism/client_version.rb
+
+ 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.

@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.

@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.

@nilbus

nilbus Mar 23, 2017

Member

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.

@nilbus

nilbus Mar 23, 2017

Member

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.

@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.

@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.

nilbus added some commits Mar 23, 2017

Display a flash notice from ClientVersion
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

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Mar 23, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Mar 23, 2017

Member

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

Member

nilbus commented Mar 23, 2017

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

Rename tracking column
Rename to client_version_update_notification_dismissed_at
as per discussion in #3416.
@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Mar 23, 2017

Member

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.

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

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 85.395%
Details
@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Mar 23, 2017

Member

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?

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

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Mar 23, 2017

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Mar 24, 2017

Member

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

facepalm

Member

nilbus commented Mar 24, 2017

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

facepalm

@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Mar 24, 2017

Member

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

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

Revert "Merge pull request #3422 from nilbus/please-update-cli-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 #3416

nilbus added a commit to nilbus/exercism.io that referenced this pull request Jun 29, 2017

Revert "Merge pull request #3422 from nilbus/please-update-cli-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 #3416
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment