[IN-54][Ember-OSF] Update ember-osf to work with reviews and preprints #327

sheriefvt · 2017-12-08T01:43:44Z

Purpose

Update ember-osf to have a unified version that works with preprints and reviews.

Summary of Changes

Merge changes and resolve conflicts between develop and feature/reviews branches
Update dependencies
Update translation service
Update ember-cli-build
Add new service for preprint-words with a unit test
Refactor/update existing tests

Side Effects / Testing Notes

This PR needs to be merged before the ember-preprints PR.

Ticket

https://openscience.atlassian.net/browse/IN-54

Another ticket is created to handle the needed changes for registries

Reviewer Checklist

* Update models for reviews API * ReviewLog adapter and serializer * Fix log creator model

…Science#3) * Include reviewable when creating review logs * Add title/contributors fields to preprint model * Clean up serializing relationships

…nScience#2) * Rename `review-log` to `action` * plac8 flake8 * Fix test errors

…rmission

…r's add page (CenterForOpenScience#11) * Apply requested changes * rename * Apply requested changes * Fix * minor change * Apply requested change * Restore comment * Update search link

* Only use SCSS * Use/Upgrade ember-bootstrap * Update EmberCLI * Remove postinstall * Remove i18n hackery

aaxelb · 2017-12-11T17:25:41Z

CHANGELOG.md

 - dropzone-widget now has custom dropzone class that extends dropzone functionality to conditionally disallow folders and multiple files from being dropped
 - 'Search' button in navbar to link to search page
+- Consolidate logic for serializing dirty relationships into `osf-serializer`
+  - Override `relationshipTypes` in a serializer to include `fieldName: 'apiType'` pairs of all relationships which may be included when saving updates


These changes are already in develop (see three lines down), could you make sure this PR is based on the right branch/commit of ember-osf?

Delete these lines

* update translation and total-share-results component

aaxelb

Looks pretty good, had a few thoughts and questions

aaxelb · 2017-12-12T14:31:53Z

CHANGELOG.md

 - dropzone-widget now has custom dropzone class that extends dropzone functionality to conditionally disallow folders and multiple files from being dropped
 - 'Search' button in navbar to link to search page
+- Consolidate logic for serializing dirty relationships into `osf-serializer`
+  - Override `relationshipTypes` in a serializer to include `fieldName: 'apiType'` pairs of all relationships which may be included when saving updates


Delete these lines

aaxelb · 2017-12-14T16:13:32Z

addon/helpers/build-secondary-nav-links.js

+                {
+                    name: session.get('isAuthenticated') ? 'eosf.navbar.myProjects' : 'eosf.navbar.browse',
+                    href: session.get('isAuthenticated') ? serviceLinks.myProjects : serviceLinks.exploreActivity,
+                },


Did you intend to add navbar links? That doesn't seem like it belongs in this PR.

aaxelb · 2017-12-14T16:14:05Z

addon/helpers/build-secondary-nav-links.js

+            links.HOME.splice(1, 0,
+                {
+                    name: 'eosf.navbar.support',
+                    href: serviceLinks.osfSupport


Same, does this belong in this PR?

aaxelb · 2017-12-14T16:39:10Z

addon/utils/preprint-word.js

+}
+
+export default function preprintWord() {
+    const entries = ['document', 'paper', 'preprint', 'thesis'];


These should live in the translation file, so we can someday translate the site to other languages. I'd be in favor of updating the translation file to use the new keys (plural, singular, etc.), and maybe changing preprintWord to documentType.

aaxelb · 2017-12-14T16:45:43Z

index.js

            backendUrlConfig.accessToken = eitherConfig('PERSONAL_ACCESS_TOKEN');
            backendUrlConfig.isLocal = true;
+            if (eitherConfig('PERSONAL_ACCESS_TOKEN')) {
+                backendUrlConfig.accessToken = eitherConfig('PERSONAL_ACCESS_TOKEN');


This seems redundant with the assignment to accessToken three lines up.

aaxelb · 2017-12-14T16:48:54Z

index.js

+    // This allows apps using this addon to import all the scss they want using "@import 'ember-osf'"
+    // The actual 'ember-osf' namespace is exported by app/styles/_ember-osf.scss
+    treeForStyles: function(tree) {
+        tree = this._super.treeForStyles.apply(this, arguments);


Don't assign to function arguments. This should probably be function() { and const tree = ...

aaxelb · 2017-12-14T16:52:10Z

tests/.jshintrc

@@ -0,0 +1,52 @@
+{
+    "predef": [
+    "document",


Ironic that .jshintrc is formatted wrong.

aaxelb · 2017-12-14T16:53:00Z

tests/unit/helpers/build-secondary-nav-links-test.js

        `);

-    assert.equal(this.$()[0].innerText, 'My Projects Search Support');
+    assert.equal(this.$()[0].innerText, 'My Projects My Projects Search Support');


Why are there two "My Projects"s?

aaxelb · 2017-12-14T17:07:13Z

tests/unit/services/file-manager-test.js

-        assert.ok(true, 'promise rejects on error');
-        done();
-    });
+    assert.ok(stub.calledWithExactly('GET', request.url, {}), '_waterbutlerRequest was called with correct parameters');


Your changes make sense, and are more like actual unit tests, but there are no longer tests for the methods you consistently stub, like _waterbutlerRequest.

…er-osf into feature/EOSF-915 # Conflicts: # package.json # yarn.lock

…er-osf into feature/EOSF-915

…er-osf into feature/EOSF-915 # Conflicts: # yarn.lock

…er-osf into feature/EOSF-915 # Conflicts: # package.json # yarn.lock

aaxelb

It seems like a good chunk of this is reinventing the ember-component-css wheel. Could we use that addon instead of all the manually-updated lists of stylesheets?

aaxelb · 2018-02-05T15:58:00Z

app/styles/_ember-osf.scss

@@ -0,0 +1,21 @@
+// Internal Styles


So this is a second file that needs to be updated for each new component? If this exists, it seems like it should be imported by addon/styles/addon.scss, instead of having two things to update.

Or we should use ember-component-css's built-in addon support, and remove both these lists.

aaxelb · 2018-02-05T16:15:34Z

addon/styles/addon.scss

@@ -1,24 +1,27 @@
+// Internal Styles


Hm, I didn't realize we don't use ember-component-css. Maybe it didn't work with addons back when ember-osf was new, but it appears to support addons now. Not sure if this is beyond the scope of this ticket, but it would be good to use ember-component-css instead of this manual approach.

aaxelb · 2018-02-05T16:19:59Z

index.js

+
+    // Outputs all pod scss files into the style tree but prefixed with ember-osf
+    // This allows apps using this addon to import all the scss they want using "@import 'ember-osf'"
+    // The actual 'ember-osf' namespace is exported by app/styles/_ember-osf.scss


This (again) feels like we're reinventing ember-component-css. It's possible there's a good reason for that, but I suspect that good reason may have expired a year ago.

aaxelb · 2018-02-05T16:26:02Z

addon/services/preprint-word.js

+
+export default Ember.Service.extend({
+    i18n: Ember.inject.service(),
+    getPreprintWord() {


This service doesn't do/provide anything that i18n doesn't already. What's the point of it?

aaxelb · 2018-02-05T16:29:15Z

ember-cli-build.js

-        test: path.join(app.bowerDirectory, 'ember/ember-template-compiler.js')
-    });
+    app.import(path.join(app.bowerDirectory, 'ember/ember-template-compiler.js'));
+    app.import(path.join(app.bowerDirectory, 'jquery-mockjax/dist/jquery.mockjax.js'));


Does this add mockjax to our production builds? I think we want it only in development.

aaxelb

I'm fine with this as-is, given that it works with both preprints and reviews. I think there should be separate tickets to address a couple things (problems which existed before this PR):

Use ember-component-css instead of a custom half-solution (see index.js, addon/styles/addon.scss, app/styles/_ember-osf.scss)
Look at how "preprint words" works in reviews and preprints, figure out a unified solution to make sure translations are defined only once and (ideally) accessed in a consistent way

Otherwise, looks good.

(cc @alexschiller )

laurenbarker · 2018-02-05T21:58:46Z

Created IN-162 for the ember-component-css change and IN-163 for the preprint word improvement.

alexschiller

Alright, merging this into release/next-next-interfaces

Tickets have been created for the two things Abram was concerned about and all should be good 👍 and I've been briefed on all of the changes, so LGTM

aaxelb and others added 30 commits September 14, 2017 09:18

[Feature] Update for reviews (CenterForOpenScience#2)

fb5cdda

* Update models for reviews API * ReviewLog adapter and serializer * Fix log creator model

[Feature] Include reviewable when creating review logs (CenterForOpen…

c237ed7

…Science#3) * Include reviewable when creating review logs * Add title/contributors fields to preprint model * Clean up serializing relationships

Add dateLastTransitioned to preprint model (CenterForOpenScience#5)

429ce0e

[Fix] Only fetch the logged in user once

55e69ab

Allow reviews settings to be nullable

d433892

Fix infinite authentication

0cc20ec

Add support for using query parameters in findRecord

b844a34

[Fix] Use getRecords() instead of .record (CenterForOpenScience#8)

a7f14a0

Tweaks and updates for reviews

607311e

Fix typo

e416b28

[Feature][MOD-73] Rename log to action (minimal branch) (CenterForOpe…

9cc89c5

…nScience#2) * Rename `review-log` to `action` * plac8 flake8 * Fix test errors

[MOD-72][Feature] Add "My Reviews" for users with view_submissions pe…

838ad02

…rmission

Add actions property to user model

024aa2b

Add user property to current-user instead

e032fa4

Add DOI alias properties to preprint model

6ad3b89

Fix permissions check in navbar (CenterForOpenScience#6)

25e5913

Only serialize actually dirty relationships

c660ea5

Wait for currentUser.user to resolve (CenterForOpenScience#8)

447e5e4

[Feature][MOD-111] Add preprint" button direct to the current provide…

81a9ffa

…r's add page (CenterForOpenScience#11) * Apply requested changes * rename * Apply requested changes * Fix * minor change * Apply requested change * Restore comment * Update search link

Use forked/fixed ember-data-has-many-query

f1ebddf

[Improvement] Upgrade deps and clean up code

15ebd80

* Only use SCSS * Use/Upgrade ember-bootstrap * Update EmberCLI * Remove postinstall * Remove i18n hackery

Wait for currentUser.user to resolve (CenterForOpenScience#9)

56680df

Update ember-data-has-many-query dependency

5eccd08

Merge branch 'feature/reviews-minimal' into feature/reviews

6a1a03a

Revert to non-forked ember-data-has-many-query

6f49fc9

Merge branch 'feature/reviews-minimal' into feature/reviews

7fc25bd

Merge branch 'develop' into feature/reviews-minimal

4964ade

Merge branch 'feature/reviews-minimal' into feature/reviews

0d64fad

Merge branch 'develop' into feature/reviews-minimal

671c2b9

Update CHANGELOG

e7e565b

sheriefvt added 2 commits December 7, 2017 20:31

Update tests / settings

702b928

Refactor file-manager tests

ed439da

sheriefvt mentioned this pull request Dec 8, 2017

[IN-101][Preprints] Update preprints to work with the unified ember-osf versions CenterForOpenScience/ember-osf-preprints#493

Closed

5 tasks

update wording

f38159b

aaxelb reviewed Dec 11, 2017

View reviewed changes

sheriefvt added 2 commits December 12, 2017 11:47

* Update changelog

8a1606c

* update translation and total-share-results component

Update preprint-word util

e924ac9

aaxelb requested changes Dec 14, 2017

View reviewed changes

sheriefvt added 6 commits January 9, 2018 15:12

Merge branch 'develop' of https://github.com/CenterForOpenScience/emb…

df6eef2

…er-osf into feature/EOSF-915 # Conflicts: # package.json # yarn.lock

Update translations

b029ec7

Replace preprint word util with service

115d651

Merge branch 'develop' of https://github.com/CenterForOpenScience/emb…

36db7d6

…er-osf into feature/EOSF-915

fix failing tests

9ca5648

Merge branch 'develop' of https://github.com/CenterForOpenScience/emb…

f3d9428

…er-osf into feature/EOSF-915 # Conflicts: # yarn.lock

This was referenced Jan 23, 2018

[RFR] Trimming down #261

Closed

[WIP][Feature] Changes needed for reviews, plus cleanup courtesy of Chris #273

Closed

laurenbarker changed the title ~~[Improvement][EOSF-915] Update ember-osf to work with reviews and preprints~~ [Improvement][IN-54] Update ember-osf to work with reviews and preprints Jan 25, 2018

alexschiller changed the title ~~[Improvement][IN-54] Update ember-osf to work with reviews and preprints~~ [IN-54][Ember-OSF] Update ember-osf to work with reviews and preprints Jan 25, 2018

baylee-d mentioned this pull request Jan 29, 2018

[IN-90][Registries] Update to Ember 2.18 CenterForOpenScience/ember-osf-registries#53

Merged

5 tasks

sheriefvt added 5 commits January 29, 2018 12:54

Reverse changes not related to this PR

d251d6e

update code changes

076e586

Add test for waterbutler request

34b95d6

remove jshintrc

1ed5072

Merge branch 'develop' of https://github.com/CenterForOpenScience/emb…

864f091

…er-osf into feature/EOSF-915 # Conflicts: # package.json # yarn.lock

aaxelb reviewed Feb 5, 2018

View reviewed changes

aaxelb approved these changes Feb 5, 2018

View reviewed changes

alexschiller changed the base branch from develop to release/next-next-interfaces February 12, 2018 18:54

alexschiller reviewed Feb 12, 2018

View reviewed changes

alexschiller merged commit df77162 into CenterForOpenScience:release/next-next-interfaces Feb 12, 2018

[IN-54][Ember-OSF] Update ember-osf to work with reviews and preprints #327

[IN-54][Ember-OSF] Update ember-osf to work with reviews and preprints #327

Uh oh!

Conversation

sheriefvt commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Summary of Changes

Side Effects / Testing Notes

Ticket

Reviewer Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaxelb 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!

aaxelb Dec 14, 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!

aaxelb 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!

aaxelb left a comment

Choose a reason for hiding this comment

Uh oh!

laurenbarker commented Feb 5, 2018

Uh oh!

alexschiller left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sheriefvt commented Dec 8, 2017 •

edited

Loading

aaxelb Dec 14, 2017 •

edited

Loading