[EMB-170] Removing and refurbishing cruft #118

aaxelb · 2018-03-13T21:58:43Z

Purpose

Remove as much custom logic from our ember-data classes as possible, and clean up anything left.

Summary of Changes

Remove dirty relationship tracking, automatic updates
- Use built-in relationship serialization instead of making separate requests
- Use some ugly private API to preserve the only thing of value: Serializing changed relationships only
Remove contributor management methods on user model
Refactor embed handling
Update new project task to create a node with institutions in one request

Side Effects / Testing Notes

No visible changes, at most would require a regression test.

For future development, updating relationships works differently, since they can now be included in a normal POST/PATCH on an object. It should be a little less magic/opaque now.

When creating new objects for a relationship, save them separately.
When adding existing objects to a relationship, just save the one object. e.g. adding institutions to a node just requires node.save()

Ticket

https://openscience.atlassian.net/browse/EMB-170

Reviewer Checklist

coveralls · 2018-03-14T00:28:04Z

Coverage remained the same at 22.045% when pulling 0f90bee on aaxelb:emb-170--dearchitecting into 6006989 on CenterForOpenScience:develop.

binoculars

+:100: for removing cruft. A few minor syntax suggestions.

binoculars · 2018-03-15T16:04:04Z

app/adapters/osf-adapter.ts

 * @uses GenericDataAdapterMixin
 */
-export default class OsfAdapter extends JSONAPIAdapter.extend(GenericDataAdapterMixin, {
+export default class OsfAdapter extends JSONAPIAdapter.extend(GenericDataAdapterMixin).extend({


Double extend? I haven't seen it before. Maybe add a comment for why?

Hm, for some reason I thought that was required for typescript to get the types from the mixin properly, but that's not true. Reverting...

binoculars · 2018-03-15T16:09:52Z

app/serializers/osf-serializer.ts

+            this.get('store').pushPayload(embeddedObj);
+
+            // Merge links on the embedded object with links on the relationship, so all returned links are available
+            const embeddedLinks = Object.assign(


You could use the object spread operator here.

binoculars · 2018-03-15T16:14:02Z

app/serializers/osf-serializer.ts

+            // Construct a new relationship in JSON API format
+            if (Array.isArray(embeddedObj.data)) {
+                relationships[relName] = {
+                    data: embeddedObj.data.map(o => ({ id: o.id, type: o.type })),


Can use destructuring and then take advantage of shorthand property names

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#Description

jamescdavis

💯 this is really great!

jamescdavis · 2018-03-21T18:29:59Z

.eslintrc.js

        strict: 0,
        indent: 0,
        'indent-legacy': 'error',
+        'function-paren-newline': ['error', 'consistent'],


jamescdavis · 2018-03-21T18:30:33Z

app/adapters/osf-adapter.ts

+        id: string,
+        snapshot: Snapshot,
+        requestType: string,
+    ): string {


yay for readability

jamescdavis · 2018-03-21T19:36:11Z

app/styles/_variables.scss

 $color-bg-blue-dark: #337ab7;
 $color-bg-blue-light: #def;
-$color-bg-red: #f00;
+$color-bg-red: #a00;


I'm assuming this makes a11y happy?

Also, the deeper red is more friendly.

TBH #f00 hurt my eyes :)

aaxelb force-pushed the emb-170--dearchitecting branch from 07b5c72 to 538534e Compare March 13, 2018 22:05

binoculars reviewed Mar 15, 2018

View reviewed changes

aaxelb added 4 commits March 21, 2018 12:50

Drop dirtyRelationships, other unnecessary cruft

e8bcf0f

Refactor embed handling

07c502b

Fix some tests

cab3b6c

Responding to review

399e43a

aaxelb force-pushed the emb-170--dearchitecting branch from c9f2243 to 0958e16 Compare March 21, 2018 17:56

Use ember-decorators

08485d9

aaxelb force-pushed the emb-170--dearchitecting branch from 0958e16 to 08485d9 Compare March 21, 2018 17:58

Update changelog

0f90bee

jamescdavis approved these changes Mar 21, 2018

View reviewed changes

jamescdavis merged commit 2f97e1b into CenterForOpenScience:develop Mar 21, 2018

aaxelb deleted the emb-170--dearchitecting branch March 21, 2018 20:32

jamescdavis added this to the 0.3.0 milestone May 7, 2019

[EMB-170] Removing and refurbishing cruft #118

[EMB-170] Removing and refurbishing cruft #118

Uh oh!

Conversation

aaxelb commented Mar 13, 2018 • 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!

coveralls commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aaxelb commented Mar 13, 2018 •

edited

Loading

coveralls commented Mar 14, 2018 •

edited

Loading