Skip to content

[IN-91][IN-101][Preprints] Update EmberCLI version #553

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

Conversation

baylee-d
Copy link

@baylee-d baylee-d commented Mar 29, 2018

Purpose

To update Preprints to use Ember 2.18

Summary of Changes

  • Update Ember to 2.18 and update other packages
  • Remove almost all bower dependencies
  • Update syntax to match 2.18 standards

TODO

This is a work in progress. Still to complete before it can be merged:

Edit page:

  • Fix Upload section to be editable (can open primary collapsible-panel but none of the secondary panels
  • Fix License section to only include Copyright holder box if the license requires it (currently displays it in all cases and won't let user save until they put something inside it)
  • Editing disciplines currently overrides previously selected disciplines. Should only add to the array of current disciplines
  • Moderation message at the bottom of the edit page is currently broken

Detail page:

  • Citation widget's placeholder has a broken translation (fix in separate ember-osf PR)

Submit page:

  • Stop the user from going between panels when they shouldn't be allowed to (ex. going from disciplines to provider carousel shouldn't be allowed, but currently is)
  • Selected disciplines do not show when the panel is collapsed
  • Copyright holder field shows up even when it isn't needed (and is required to save the section)
  • Abstract field is too small
  • License text doesn't show
  • Selected basics fields do not show when the panel is collapsed
  • Can create a preprint to branded providers, but fails to transition to the new preprint

Routing:

  • Can not transition to a branded provider. Will put the user through an infinite transition loop.
  • Can not go to a branded preprint detail page. The route on transition adds the guid a second time at the end of the url and makes a broken link

Side Effects / Testing Notes

This changes a lot of the syntax and packages being used in preprints. Because of that, it will need a full regression to make sure that things are still working the way they currently are on production.

Ticket

https://openscience.atlassian.net/browse/IN-91
https://openscience.atlassian.net/browse/IN-101

Reviewer Checklist

  • meets requirements
  • easy to understand
  • DRY
  • testable and includes test(s)
  • changes described in CHANGELOG.md

sheriefvt and others added 30 commits November 30, 2017 15:25
…er-preprints into feature/EOSF_915

# Conflicts:
#	CHANGELOG.md
#	package.json
#	yarn.lock
…er-preprints into feature/EOSF_915

# Conflicts:
#	app/locales/en/translations.js
#	app/templates/index.hbs
#	package.json
#	yarn.lock
…er-preprints into feature/EOSF_915

# Conflicts:
#	public/assets/osf-assets
#	yarn.lock
Small fixes and preventing an error
…rOpenScience/ember-osf-preprints into feature/IN-91-update-cli-version

# Conflicts:
#	app/components/preprint-form-header.js
#	app/components/preprint-form-project-select.js
#	app/components/preprint-title-editor.js
#	app/components/provider-carousel.js
#	app/components/subject-picker/component.js
#	app/controllers/content/index.js
#	app/controllers/index.js
#	app/controllers/submit.js
#	app/mixins/setup-submit-controller.js
#	app/templates/components/provider-carousel.hbs
#	app/templates/index.hbs
#	package.json
#	public/assets/osf-assets
#	tests/integration/components/supplementary-file-browser-test.js
#	tests/unit/controllers/content/index-test.js
#	tests/unit/controllers/submit-test.js
#	yarn.lock
<link href="https://maxcdn.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css" rel="stylesheet" type="text/css">
<link href="https://cdnjs.cloudflare.com/ajax/libs/dropzone/5.1.1/min/basic.min.css" rel="stylesheet" type="text/css">
<link href="https://cdnjs.cloudflare.com/ajax/libs/dropzone/5.1.1/min/dropzone.min.css" rel="stylesheet" type="text/css">
<script src="https://cdnjs.cloudflare.com/ajax/libs/dropzone/5.1.1/min/dropzone.min.js"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

These script-tag style import of dependencies doesn't look right. Talked to @baylee-d and it seems that Dropzone.js does not work without these imports. Same thing with tagsinput. Somehow ember-osf-web is also importing Dropzone.js this way, in addition to including them in package.json.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ember-cli-dropzonejs could be a future solution.

@@ -1,4 +1,5 @@
import Ember from 'ember';
import { inject } from '@ember/service';
Copy link
Member

Choose a reason for hiding this comment

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

The recommended way to do this is:

import { inject as service } from '@ember/service';

i18n: Ember.inject.service(),
valid: Ember.computed.alias('newContributorId'),
i18n: inject(),
valid: computed.alias('newContributorId'),
Copy link
Member

@jamescdavis jamescdavis Apr 18, 2018

Choose a reason for hiding this comment

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

can also import { alias } from '@ember/object/computed'; and just do:

valid: alias('newContributorId'),

import Component from '@ember/component';
import { computed } from '@ember/object';
import { observer } from '@ember/object';
import { get } from '@ember/object';
Copy link
Member

Choose a reason for hiding this comment

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

You can roll these into one.

@@ -1,4 +1,6 @@
import Ember from 'ember';
import { get } from '@ember/object';
import { computed } from '@ember/object';
Copy link
Member

Choose a reason for hiding this comment

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

combine with line above

Copy link
Contributor

Choose a reason for hiding this comment

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

Those are all caught by ESLint and will go into the same release. There is another PR branched off this one that is partially done.

});
this.set('currentPanelName', nextPanel);
},
*/
Copy link
Member

Choose a reason for hiding this comment

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

delete?

this.get('panelActions').open(this.get(`_names.${this.get('_names').indexOf(currentPanelName) + 1}`));
this.set('currentPanelName', this.get(`_names.${this.get('_names').indexOf(currentPanelName) + 1}`));
// this.set('currentPanelName', currentPanelName);
Copy link
Member

Choose a reason for hiding this comment

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

delete?

@@ -251,7 +256,7 @@ export default Ember.Route.extend(Analytics, ResetScrollMixin, SetupSubmitContro
actions: {
error(error) {
// Handle API Errors
if (error && error.errors && Ember.isArray(error.errors)) {
if (error && error.errors && isArray(error.errors)) {
Copy link
Member

Choose a reason for hiding this comment

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

Array.isArray() would work here too and save you an import

model() {
return Ember.RSVP.hash({
return hash({
Copy link
Member

Choose a reason for hiding this comment

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

This is a case where I'd import RSVP from 'rsvp'; and do RSVP.hash({

model() {
// Store the empty preprint to be created on the model hook for page. Node will be fetched
// internally during submission process.
return this.store.createRecord('preprint', {
return this.get('store').createRecord('preprint', {
Copy link
Member

Choose a reason for hiding this comment

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

The previous code will be fine when Preprints goes to Ember 3.1 😉

@@ -5,7 +5,7 @@ export default function buildProviderAssetPath(config, providerId, assetName, is
return pathJoin(config.providerAssetsURL, providerId, assetName);
}
if (config.ASSET_SUFFIX) {
assetName = assetName.replace(/\.[^\.]+$/, `-${config.ASSET_SUFFIX}$&`);
assetName = assetName.replace(/\.[^.]+$/, `-${config.ASSET_SUFFIX}$&`);
Copy link
Member

Choose a reason for hiding this comment

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

👍

"resolutions": {
"jquery": "2.2.4"
"sanitize.js": "^1.0.0",
"jquery.tagsinput": "1.3.6"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to remove this file completely now.

@@ -33,7 +33,7 @@ module.exports = function(defaults) {
}

// Reference: https://github.com/travis-ci/travis-web/blob/master/ember-cli-build.js
const app = new EmberApp(defaults, {
let app = new EmberApp(defaults, {
Copy link
Member

Choose a reason for hiding this comment

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

const app is appropriate here because app is never reassigned.

@@ -42,7 +41,7 @@ export default function(name, options = {}) {
afterEach() {
FakeServer.stop();
let afterEach = options.afterEach && options.afterEach.apply(this, arguments);
return Promise.resolve(afterEach).then(() => destroyApp(this.application));
return resolve(afterEach).then(() => destroyApp(this.application));
Copy link
Member

Choose a reason for hiding this comment

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

RSVP.resolve

Ember.run(() => {
application = Application.create(attributes);
return run(() => {
let application = Application.create(attributes);
Copy link
Member

Choose a reason for hiding this comment

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

const

@adlius
Copy link
Collaborator

adlius commented Apr 18, 2018

Also did a first pass. The only problem that stands out is the script-tag style imports. Maybe installing dropzone with bower could solve the problem, as that was what we did previously. If not, then we could probably address this later by using addons like ember-cli-dropzonejs.

@baylee-d
Copy link
Author

baylee-d commented May 2, 2018

Okay, I updated a few things:
~ Changed all metrics calls to use this.get('metrics') instead of get(this, 'metrics')
~ Changed injections to be referenced as service
~ Merged imports together to remove redundant calls
~ Removed all observers

I looked back into moving dropzone imports from index.html back to bower.json to remove the static import strings, but I was getting the same error about not knowing what Dropzone was. For now, I think it'll be okay as it is, but it can definitely be something that we look into in the future since we'll also have to revisit removing bower imports entirely.

Copy link
Contributor

@alexschiller alexschiller left a comment

Choose a reason for hiding this comment

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

Few minor things to look into/remove

@@ -26,6 +27,7 @@ export default Ember.Route.extend(Analytics, OSFAgnosticAuthRouteMixin, {
{
filter: {
domain: `${window.location.origin}/`,
/*domain: 'http://local.engrxiv:4201',*/
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

import EmberObject from '@ember/object';
import { get } from '@ember/object';
import { merge } from '@ember/polyfills';
import I18nService from 'ember-i18n/services/i18n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this file newly added? Or did it get accidentally revived?

Copy link
Author

Choose a reason for hiding this comment

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

This was added to fix the problems I was having with variables in translation strings. I think there was a fix for it at the time but it wasn't released yet. Updating to the latest next-interfaces should fix this.

@@ -92,8 +92,7 @@ module.exports = function(environment) {

if (environment === 'test') {
// Testem prefers this...
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment referred to the below iirc, so probably remove

package.json Outdated
"bugs": {
"url": "https://github.com/CenterForOpenScience/ember-osf-preprints/issues"
},
"homepage": "https://github.com/CenterForOpenScience/ember-osf-preprints#readme"
"homepage": "https://github.com/CenterForOpenScience/ember-osf-preprints#readme",
"dependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can any of these be moved to dev deps?

@@ -1,15 +0,0 @@
<?xml version="1.0"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

did we delete this or coming from elsewhere? Might need to get up to date with develop

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -1,11 +0,0 @@
import Resolver from '../../resolver';
Copy link
Contributor

Choose a reason for hiding this comment

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

same question as others, is this just no longer needed or what's up?

CHANGELOG.md Outdated
@@ -109,6 +109,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Wording on `Edit preprint` button to `Edit and resubmit` on preprint detail page if the preprint is rejected
and the workflow is pre-moderation.
- Removed footer styling
- Update preprints to work with a new unified version of ember-osf (works with both preprints and reviews apps)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to the correct spot and add CHANGELOG for 2.18 update

Baylee Swenson and others added 3 commits May 3, 2018 11:27
…rOpenScience/ember-osf-preprints into feature/TAKE_FIVE_IN-91-update-cli-version

# Conflicts:
#	app/components/preprint-form-header.js
#	public/assets/osf-assets
…e-cli-version

Feature/take five in 91 update cli version
Copy link
Contributor

@alexschiller alexschiller left a comment

Choose a reason for hiding this comment

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

go time

Copy link
Contributor

@alexschiller alexschiller left a comment

Choose a reason for hiding this comment

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

¯\_(ツ)_/¯

@alexschiller alexschiller merged commit c529336 into CenterForOpenScience:release/next-interfaces May 3, 2018
baylee-d pushed a commit to baylee-d/ember-osf-preprints that referenced this pull request Jun 27, 2018
…nce#553)

* Update dependencies

* Update translation service

* Update wording in changelog

* Update eslint version

* Use ember-tag-input instead of tags-widget

* Update tests

* Update confirm-share-modal to according to ember-bootstrap updates

* Update package and yarn.lock

* Update dependent ember-i18n

* restore translation

* Use preprint-word util from ember-osf

* avoid bare strings

* Apply requested changes

* Remove unused wait import

* Update packages, convert CLI things

* First commits, update dependencies

* Prevent errors

* upgrade/downgrade dependencies.  point to different ember-osf branch

* Fix import for submit controller

* Update dependencies, change provider-asset path

* Work on cp-panels

* Get submit page working without errors

* Work on submit page, get disciplines working, submit modal to display

* Work on updating disciplines

* Finish disciplines to work on edit and submit

* Fix some cp-panels on submit page, remove npm ember call from ember-cli-build to enable tests locally

* Fix tests

* Fix cp-panels

* Update package.json

* Finalize fixing of cp-panels

* Fix tests

* Get template working

* Revert to using 1.0.0 collapsible-panels

* Fix tests, linting, panels

* Update ember-cli-moment, fix import in i18n, add scss changes

* Fix imports, remove uneeded code

* Remove and replace observers

* Get disciplines to show and save

* Add to CHANGELOG, remove commented code, move dependencies to devdependencies
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.

7 participants