Branding Phase 2 - Provider Domains [PREP-259] #201

binoculars · 2016-11-29T19:01:57Z

See the spec for details.

Companion to:

[OSF-7411] Branded Preprints Phase 2 osf.io#6793 (Adds domain field, redirects)
[EOSF-477] Branded Preprints Phase 2 ember-osf#173 (Adds domain field to the model)

Ticket

https://openscience.atlassian.net/browse/EOSF-327

TODO:

Make the routes under /preprints/:provider/:whatever redirect to provider.domain/:whatever
Update all redirects to go to the correct place, the first time (no multiple redirects, where possible).
Get tests passing
Provide guidance for staging and prod (Update: DevOps ticket in Jira)

Notes for Reviewers:

If testing this PR without the changes from OSF, set environment variable ENABLE_PROVIDER_DOMAINS='true'

See the README section on how to populate the domains locally. The domains are defined in the ember config (config/environment.js). TLDR: sudo ./scripts/add-domains.js

You should be able to go to a provider such as http://local.socarxiv.org:4200/ (preprints container or ember serve must be running.
Going to http://localhost:5000/preprints/socarxiv should redirect you to http://local.socarxiv.org:4200/
If you create a preprint under a provider and try to go to it from http://localhost:5000/abc12, you should be redirected to http://local.socarxiv.org:4200/abc12. Note: GUID redirects will be handled by osf.io in [OSF-7411] Branded Preprints Phase 2 osf.io#6793 and /preprints/socarxiv should be redirected by the proxy in staging/production. The redirects here are primarily to handle anything that falls through the cracks.
Please click through each portion of the site and verify that the links work correctly.
If you comment out one of the domains (other than osf.io) in config/environment.js, you should be able to go back to using http://localhost:5000/preprint/provider style routing. Please verify that everything still works as expected here.

Notes for QA

This is mostly a code change with the exception of how the routing is handled. Once this is up on staging, all of the redirects should work as expected.

…ints into feature/branding-phase2

Created error-page component and migrated forbidden, page-not-found, and resource-deleted to use error-page component Updated english translation to match error-component migration Removed unused reroute-guid template

Update provider route to use the redirect if the domain exists in the configuration Fix for asset loading in index.html\

Update default ember cli settings

Add translation with 'or' from index page

…ints into feature/branding-phase2

abought

This is not a complete review, but I have been called away to a number of Lookit tasks, and am submitting any notes so far "as is". No functional review has been performed yet.

Per discussion with Barrett today, first impressions are all around maintainability:

Please add a few short YUIDoc strings on helpers to help document specific requirements being met
Very strong preference for at least some tests. If we can fast track the factories PR, that would help with this. Maintainability is a challenge due to complexity of 4-state router requirements.
If possible, a CDN for css assets would help us avoid some of the scattered code for asset paths. May be out of scope for this PR but would help with maintainability.

abought · 2017-03-14T19:35:35Z

app/locales/en/translations.js

    },
    application: {
-        // Nothing to translate
+        separator: ` | `


Is this something we really need to expose to translations?

If the goal is only to DRY this code, consider using the ember-page-title addon config:
https://github.com/tim-evans/ember-page-title#api

abought · 2017-03-14T19:55:04Z

app/routes/content.js

+                const configProvider = providers.find(p => p.id === providerId);
+
+                if (!configProvider)
+                    throw new Error('Provider is not configured properly. Check the Ember configuration.');


There does not appear to be any .catch statement associated with this promise. (aside from whatever ember will do internally if the afterModel hook returns a rejected promise)

Is this an error the user should be notified about?

If so, should there be a translation string here?

abought · 2017-03-14T20:04:05Z

app/routes/provider.js

+        if (providerIds.includes(slugLower)) {
+            const {domain} = providers.find(provider => provider.id === slugLower) || {};
+
+            // This should be caught by the proxy, but we'll redirect just in case it is not.


Note to self: "the proxy" refers to nginx handling domain redirect (before it hits ember)

abought · 2017-03-14T20:22:06Z

config/environment.js

+                continue;
+
+            if (provider.id === 'osf') {
+                provider.domain = OSF_URL || 'localhost:5000';


Please try to avoid hardcoding URLs and ports wherever possible, and strongly prefer fetching config from pre-existing sources. (ember osf config should already provide an OSF URL, for example)

abought · 2017-03-14T20:31:06Z

config/environment.js

+                continue;
+            }
+
+            const suffix = DOMAIN_PREFIX ? '' : `:${PORT ? PORT : '4200'}`;


I can't seem to find a good way to determine port number, alas. At very least this could be shortened to PORT || '4200'.

But to be honest, that's bikeshedding over syntax. It seems like this code is trying to do a very large number of things simultaneously, which is inevitably going to add complexity.

abought · 2017-03-16T17:56:36Z

ember-cli-build.js

+            assets: {
+                enabled: true,
+                content: `
+                    <script>


Future suggestion:

Consider using a CDN to serve assets like CSS. That would save us from this code because we would not need ember to construct CSS urls.

(ember locally would serve normally, and in production builds we'd use a CDN)

May not be in scope for this PR, but strongly worth future consideration. cos.io, for example, is already moving in this direction. This is a common pattern and I think any deployment challenges would be very solvable with automated DevOps support.

This code is a constructed string: it will be hidden from any tooling or bugfixing/ validation, and it causes url logic to be in many different places. By putting these specific assets on a different domain, the asset path wouldn't depend on URL- a CDN or "static asset domain" would help us avoid the necessity for some very gnarly code, and sidestep a potential long-term maintainability issue.

abought · 2017-03-16T18:04:26Z

app/services/theme.js

+    guidPathPrefix: Ember.computed('isProvider', 'isDomain', 'id', function() {
+        let pathPrefix = '/';
+
+        if (!this.get('isDomain') && this.get('isProvider')) {


See note elsewhere on CDN possibilities to simplify this code.

abought · 2017-03-16T18:07:13Z

app/helpers/route-prefix.js

@@ -0,0 +1,15 @@
+import Ember from 'ember';
+
+export default Ember.Helper.extend({


Per discussion- can you please add yuidoc comments to explain the purpose of this helper?

(eg driven by the requirements of the 4-state router)

abought · 2017-03-16T20:09:02Z

app/templates/components/taxonomy-top-list.hbs

        {{#each pair as |subject|}}
            <div class="col-xs-12 col-sm-6 subject-item {{if (get subjectTooltips subject.text) 'hint--bottom hint--large'}}" aria-label={{get subjectTooltips subject.text}}>
-                {{#link-to (if theme.isProvider 'provider.discover' 'discover') (query-params subjectFilter=subject.text) class='btn btn-primary p-sm' invokeAction=(action "click" "link" (concat 'Preprints - Index - Browse ' subject.text))}}
+                {{#link-to (route-prefix 'discover') (query-params subjectFilter=subject.text) class='btn btn-primary p-sm' invokeAction=(action "click" "link" (concat 'Preprints - Index - Browse ' subject.text))}}


This is a somewhat gnarly requirement. Have you taken a look at Route#renderTemplate? It seems to provide a way for one route to use the template or behavior of another. I'm not totally sure if it applies here, but might spark some ideas.

http://emberjs.com/api/classes/Ember.Route.html#method_renderTemplate

abought · 2017-03-16T20:11:19Z

scripts/add-domains.js

@@ -0,0 +1,48 @@
+#!/usr/bin/env node


No review performed on this script in initial round.

hmoco · 2017-03-30T20:12:02Z

app/router.js

+    .find(p =>
+        // Check if the hostname includes: the domain, the domain with dashes instead of periods, or just the id
+        hostname.includes(p.domain) ||
+        hostname.includes(p.domain.replace(/\./g, '-')) ||


When is this relevant, out of curiosity?

I had a conversation with @icereval a while back about how we might deploy this on staging/test. agrixiv-org.staging.osf.io was a likely naming scheme. Putting a dot in a subdomain creates another subdomain.

hmoco

Quick-ish run through. Looks good, just have a few questions about some choices made here. Nice work!

hmoco · 2017-03-30T20:14:30Z

app/router.js

-        this.route('content', {path: '/:preprint_id'});
-        this.route('discover');
+
+    if (provider) {


Does provider get recalculated properly when window.location changes? I would assume it does, since it works, but it seems weird that a const value changed would cause this Router.map to be rerun, assuming the preprint app is still a singleton across the different providers

Since changing the provider requires a full page reload, the entire app, including the router, will be evaluated each time.

hmoco · 2017-03-30T20:30:53Z

app/services/theme.js

    session: Ember.inject.service(),

+    // If we're using a provider domain
+    isDomain: false,


Not the biggest fan of this, could we possibly change it to something like hasDomain, isProvider, onProvider, onProviderDomain? Don't feel strongly about any of these, but was initially confused by isDomain (although the comment is super helpful)

hmoco · 2017-03-30T20:32:10Z

app/services/theme.js

+            pathPrefix += 'preprints/';
+
+            if (this.get('isProvider')) {
+                pathPrefix += `${this.get('id')}/`;


wait, just realized that there's also something called isProvider. When would isDomain and isProvider not both be the same?

Some of the providers will not use their own domain, and will stay under osf.io/preprints/whatever

isProvider is used to determine if the provider is OSF (for our default case) or not
isDomain is whether the provider is using a domain or not

Is there a case in which the provider being used has a domain but is not using it?

isProvider is true if, and only if, the current provider is not OSF?

If the domain exists in the config (and domains are enabled), then using the domain is used. It has to be one or the other due to the requirements.

Yes, isProvider basically means not OSF.

hmoco · 2017-03-30T20:34:27Z

app/templates/forbidden.hbs

+{{error-page
+    label='Forbidden'
+    translateKey='page-forbidden'
+}}


Really like this!

hmoco · 2017-03-30T20:38:04Z

scripts/add-domains.js

+const sectionFooter = '\n## /EMBER-PREPRINTS ##';
+const rgx = new RegExp(`(?:${sectionHeader})(.|\\s)*(?:${sectionFooter})`, 'm');
+const domainProviders = providers
+    .slice(1)


Throughout this PR, there seems to be a lot of providers.slice(1). Is there any use for a separate config var that defines non-OSF providers?

I was trying to avoid modifying the original config too much, which would have downstream changes--not ideal, but neither is creating a ton of merge conflicts if it can be avoided.

hmoco · 2017-03-30T20:42:48Z

Functional testing looked good, although I was unable to test the redirect from /preprints/<provider> to provider domain.

hmoco · 2017-03-30T20:48:18Z

Also, after reading through this, not really a request for a change, but a quick note for both future development and potential refactoring: our usage of the word provider seems to be incredibly broad, including OSF at times. It leads to slightly confusing code due to it not being descriptive or precise enough. @brianjgeiger, @binoculars any opinions on the matter or potential alternative language?

binoculars · 2017-03-31T16:47:59Z

I got set up locally and updated my osf.io PR. I would like avoid updating that again until we're serious about merging, because it needs a migration.

My local copy is redirecting, so I'm not sure what's causing it to not redirect. It could be a container sync issue.

I would like to come up with better semantics for provider. At this point, we've been using it in branded preprints since that was launched, so a refactor would need to be planned out.

brianjgeiger · 2017-03-31T19:32:25Z

I think in the OSF world, a Provider is a third party that has information, code, or service that is being served via the OSF but is not generally available to everyone. So Users aren't Providers because anyone can be a user, but PreprintProvider, AddonProvider, RegistryProvider, and so on would be valid Providers (whether or not all of those things currently exist). Do we have uses that are not currently following that convention?

binoculars · 2017-03-31T22:39:15Z

I'm not aware of any, but it would good to codify that somwhere

binoculars added 4 commits November 16, 2016 15:11

Route changes for phase2

4807b45

Merge branch 'feature/branding' into feature/branding-phase2

45ca1fb

Merge branch 'develop' of github.com:CenterForOpenScience/ember-prepr…

ad2668a

…ints into feature/branding-phase2

WIP on phase-2 of branded preprints

2e8f4da

binoculars changed the title ~~[WIP] Feature/branding phase2~~ [WIP] Feature/branding phase2 [PREP-259] Nov 29, 2016

binoculars added 9 commits January 24, 2017 10:48

Merge branch 'develop' of github.com:centerforopenscience/ember-prepr…

f226122

…ints into feature/branding-phase2

Update add-domains script

b671199

Update tests with their needed services

8f8d242

Created route-prefix helper

3499073

Created error-page component and migrated forbidden, page-not-found, and resource-deleted to use error-page component Updated english translation to match error-component migration Removed unused reroute-guid template

Add a utility for handling redirects and several tests

3d0b1eb

Update provider route to use the redirect if the domain exists in the configuration Fix for asset loading in index.html\

Move the assets script in to ember-cli-build (use es5 for without babel)

3041db0

Update default ember cli settings

Add domain redirects for content route

09bb9a1

Update preprint button on index page to use theme.pathPrefix

f28a3a4

Add translation with 'or' from index page

Fix path for redirect in content route

c9cd22c

binoculars mentioned this pull request Jan 30, 2017

[OSF-7411] Branded Preprints Phase 2 CenterForOpenScience/osf.io#6793

Merged

Add OSF host for branded navbar

d238433

binoculars force-pushed the feature/branding-phase2 branch from f2f9df6 to d238433 Compare January 30, 2017 21:30

binoculars and others added 13 commits January 30, 2017 17:49

Update 'edit preprint' button on content page with the correct link

287a2e7

Fix submit transition failure on submit/save

2b48c33

Merge branch 'develop' of github.com:centerforopenscience/ember-prepr…

f723673

…ints into feature/branding-phase2

Update add domains script

2d198ca

Merge branch 'develop' into feature/branding-phase2

e8fd5f3

Merge branch 'develop' of github.com:centerforopenscience/ember-prepr…

868ef36

…ints into feature/branding-phase2

Add domains for new providers, bug fix in add-domains script

d211a46

Update content route

ea435ad

Merge branch 'develop' of github.com:centerforopenscience/ember-prepr…

8e96e60

…ints into feature/branding-phase2

Remove BITSS domain and update engrXiv domain

1f72e0c

Temporarily disable some providers

2cc81a8

Merge branch 'develop' of github.com:centerforopenscience/ember-prepr…

8d3b63c

…ints into feature/branding-phase2

Enable AgriXiv domain

eb2a309

binoculars added 3 commits March 10, 2017 16:21

Use environment variable to enable provider domains

21e2f19

Enable and update PsyArXiv domain

1e6a1d7

Update add-domains script for enabling provider domains

b29b85f

binoculars changed the title ~~[WIP] Feature/branding phase2 [PREP-259]~~ Branding Phase 2 - Provider Domains [PREP-259] Mar 13, 2017

binoculars mentioned this pull request Mar 13, 2017

[HOLD] [OSF-7413] Feature/branding phase2 CenterForOpenScience/COSDev#51

Merged

2 tasks

abought reviewed Mar 16, 2017

View reviewed changes

Add docs and comments

867774c

hmoco reviewed Mar 30, 2017

View reviewed changes

brianjgeiger merged commit 1ec026f into CenterForOpenScience:develop Apr 3, 2017

		@@ -0,0 +1,15 @@
		import Ember from 'ember';

		export default Ember.Helper.extend({

Branding Phase 2 - Provider Domains [PREP-259] #201

Branding Phase 2 - Provider Domains [PREP-259] #201

Uh oh!

Conversation

binoculars commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

TODO:

Notes for Reviewers:

Notes for QA

Uh oh!

abought left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmoco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmoco Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmoco commented Mar 30, 2017

Uh oh!

hmoco commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

binoculars commented Mar 31, 2017

Uh oh!

brianjgeiger commented Mar 31, 2017

Uh oh!

binoculars commented Mar 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

binoculars commented Nov 29, 2016 •

edited

Loading

hmoco Mar 30, 2017 •

edited

Loading

hmoco commented Mar 30, 2017 •

edited

Loading