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

Version updater fixes #3061

Merged
merged 4 commits into from
Jan 11, 2017
Merged

Version updater fixes #3061

merged 4 commits into from
Jan 11, 2017

Conversation

maheshp
Copy link
Contributor

@maheshp maheshp commented Jan 3, 2017

  • Version update check was enabled on new SPA pages.
  • New Go version notifications are displayed on new SPA pages.
  • Version check is skipped on DevelopmentServer
  • Using route helper with _path rather than _url to fix One resource being loaded over HTTP #2350

@@ -22,4 +22,9 @@
</span>
</div>
</div>
<script type="application/javascript">
require(['models/shared/version_updater'], function(VersionUpdater) {
Copy link
Member

Choose a reason for hiding this comment

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

See the production build does not package this file. So this will fail in production. Additionally even if you do package the file in production, you're likely not going to get the correct hash for version_updater in production. Post that, this will cause additional http requests to the server. Making for a less-than-ideal page load experience. Any possibility that we add this to each SPA?

We already require foundation.dropdownMenu and a few other friends for the navigation header on top, so don't mind putting that in the top level require itself

Copy link
Member

Choose a reason for hiding this comment

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

Additionally the check_go_updates? guard exists on old pages, but not in this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was looking at avoiding adding this script in each SPA, so was trying this option. Let me think bout and it we can look at the right way to do this.

check_go_updates? needs to be added, will add it.

var thirtyOneMinutesBack = new Date(Date.now() - 31 * 60 * 1000);

spyOn(localStorage, 'getItem').and.returnValue(JSON.stringify({last_updated_at: thirtyOneMinutesBack}));
spyOn(m, 'request').and.returnValue(deferred.promise());
Copy link
Member

Choose a reason for hiding this comment

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

Instead of stubbing m.request any possibility of directly using mock-ajax — makes for cleaner behaviorial tests. Here you're effectively testing what attributes you set on the deferred, aka — the internal implementation of m.request. If that changes, we'd have to re-write a whole bunch of tests, in addition to changing the actual code.

See this for reference.

@maheshp maheshp force-pushed the version_updater_fixes branch 5 times, most recently from 1e28f80 to 7b5a0c4 Compare January 11, 2017 03:59
@maheshp maheshp merged commit d95a388 into gocd:master Jan 11, 2017
@zabil zabil removed the in progress label Jan 11, 2017
@maheshp maheshp added this to the Release 17.1 milestone Jan 11, 2017
@maheshp maheshp deleted the version_updater_fixes branch January 12, 2017 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

One resource being loaded over HTTP
3 participants