Feature/Node-Preprint Divorce Ember-OSF [IN-98] #417

pattisdr · 2018-09-23T22:02:39Z

Related PR's

Back End
Ember-OSF
Front End

TODO List

Purpose

Divorce changes to enable the front-end to talk to the modified back end.

Summary of Changes/Side Effects

Contributor model shared between nodes and preprints. "preprint" relationship added to contributor model. I don't think this should be a problem, because the contributor ids will be unique, as they are a combination between the preprint or node guid and the user id.
Contributor mixin added to share contributor-related methods between the node and the preprint
Public, files, currentUserPermissions, contributors fields added to the preprint model
Contributor adapter modified to talk to either the node contributor endpoint or preprint contributor endpoint
Preprint adapter modified to be able to send a request to the preprint's node relationship instead of the preprint when the user is attempting to remove the node from the preprint.
Fix for building mfrUrls
Default message if no files found in old-file-browser

Testing Notes

Ticket

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

Notes for Reviewer

Can you think of any issues that sharing the contributor model between the node and preprint would cause?

Reviewer Checklist

…the Preprint model. - Consolidate contributor-related methods to a ContributorMixin to be shared between the node and preprint models. - Add a preprint relationship to the contributor model - tentatively trying to share this between nodes and preprints? - Update contributor adapter so requests can be made to a preprint or a node.

…tributor. - If a contributor is deleted, and then added, you get a record like "Assertion Failed: 'contributor' was saved to the server, but the response returned the new id 'abcde', which has already been used with another record.'"

…ndpoint to unset the node.

pattisdr · 2018-09-30T21:15:10Z

addon/adapters/contributor.js

            let node = this.store.peekRecord('node', nodeId);
+            if (!node) {


Contributor adapter attempts to fetch the node from the store. If no node exists, attempts to fetch the preprint from the store. These changes allow the contributor model to be shared between the node and the preprint. This will either build a URL for node contributors or preprint contributors.

pattisdr · 2018-09-30T21:17:55Z

addon/adapters/preprint.js

-
-        var id = snapshot.id;
-        var url = this.buildURL(type.modelName, id, snapshot, 'updateRecord');
+        if (snapshot.record.get('_dirtyRelationships')['node'] && snapshot.record.get('_dirtyRelationships')['node']['remove'].length && !snapshot.record.get('_dirtyRelationships')['node']['add'].length) {


A node relationship endpoint has been added to the preprint serializer that is always visible, whether the node exists or not. If you want to delete the supplemental project off the node, the request needs to be sent to this relationship link instead of the preprint detail link.

Only PATCH to this relationship endpoint when attempting to remove the node from the preprint; not used for adding a node or updating the node.

It seems with this setup, updateRecord() can remove a node relationship of a preprint, or it can update a preprint's other attributes and non-node relationships, but not both at the same time. Does that have any side effects for how we use .save() on a preprint instance?

In my opinion, this is only an issue if we need to update a preprint attribute and remove a node simultaneously, which we do not. (This is also new functionality; it was previously impossible to remove the node relationship from the preprint). You can still update a preprint attribute and add/update a node relationship simultaneously, as before. However, to unset a node, this has to be a request to a different endpoint.

In ember-osf-preprints, there is one location where we can remove the node from the preprint. It is unset locally and then immediately persisted to the server.

That's fine. We just need to keep in mind that we cannot remove a node relationship and update other attributes at the same time. Should we add such a notice somewhere to make sure other developers won't run into this problem in the future if they ever try to do this?

yes, good idea.

pattisdr · 2018-09-30T21:18:36Z

addon/mixins/contributor-mixin.js

@@ -0,0 +1,142 @@
+import Ember from 'ember';
+
+export default Ember.Mixin.create({


Contributor methods on node model are identical to the methods needed on the preprint model, so they have been consolidated into this mixin.

pattisdr · 2018-09-30T21:19:13Z

addon/models/contributor.js

@@ -54,5 +54,8 @@ export default OsfModel.extend({

    node: DS.belongsTo('node', {
        inverse: 'contributors'
+    }),
+    preprint: DS.belongsTo('preprint', {
+        inverse: 'contributors'


Giving the contributor model a preprint relationship as well as a node relationship, since we are sharing the model between nodes and preprints.

adlius · 2018-10-01T18:59:53Z

addon/mixins/contributor-mixin.js

+
+    removeContributor(contributor) {
+        return contributor.destroyRecord().then(rec => {
+            this.get('store')._removeFromIdMap(rec._internalModel);


Could you clarify why a call to _removeFromIdMap() is needed here while it was not needed in the removeContributor() method of the node model previously?

Ah, yes, I forgot to comment on this. When attempting to remove the contributor from the preprint, and then add the contributor back, I was getting the following in the console: Error: Assertion Failed: 'contributor' was saved to the server, but the response returned the new id '12345', which has already been used with another record.' . It didn't seem like all evidence of the deleted contributor was being removed? Seemed similar to this issue emberjs/data#5014.

The only contributor-related difference I can think of between nodes and preprints are that nodes were using a NodeMixin to make these contributor requests to the ember-osf node model. Preprints are calling the contributor methods on the ember-osf preprint model directly, but I don't know why this would make a difference. I'll make a note to look into this more over the next couple days. I don't want to call a private method, but haven't yet found a way around this.

I did some digging to little avail. It is definitely some ember-data related issue. Possible is that the different class structures result in race condition internal to core ember. I do not think it is worth spending more time looking into it as it would will be resolved by moving to newer version of ember-data once the root cause is fixed. A new ticket could be created to address this in the future.

adlius · 2018-10-01T19:04:07Z

First pass finished. Some clarifying questions, but nothing major that needs fixing.

[NPD] Add notes

Syntax cleanup

jamescdavis

👍

pattisdr added 5 commits September 12, 2018 19:38

Add supplemental project as a document type.

202c98b

Revert preprint-provider adapter change.

21930f9

Merge develop.

ccaa1d7

This was referenced Sep 23, 2018

[Need QA Prep] Feature/The Node-Preprint Divorce [PLAT-555][PLAT-480][PLAT-559][PLAT-561][PLAT-1168] CenterForOpenScience/osf.io#8640

Closed

[Need QA Prep] Feature/Divorce Front End [ember-osf-preprints] [IN-98] CenterForOpenScience/ember-osf-preprints#626

Merged

Modify preprint adapter to be able to construct a node relationship e…

878fb60

…ndpoint to unset the node.

pattisdr force-pushed the feature/node-preprint-divorce branch from c1a4863 to 878fb60 Compare September 25, 2018 16:18

Eslint errors.

3ccdfa1

pattisdr force-pushed the feature/node-preprint-divorce branch from 079efad to 3ccdfa1 Compare September 30, 2018 20:53

pattisdr commented Sep 30, 2018

View reviewed changes

pattisdr changed the title ~~[WIP] Node-Preprint Divorce Ember-OSF [IN-98]~~ [Ready for Review] Node-Preprint Divorce Ember-OSF [IN-98] Sep 30, 2018

adlius reviewed Oct 1, 2018

View reviewed changes

pattisdr and others added 8 commits October 1, 2018 19:50

Fix error building mfrUrl if it is a long form URL.

5b801af

Add no files found if no files returned in old-file-browser component.

5e299d9

Modify add contributors to allow adding unregistered information.

09a88b4

Remove unnecessary semicolon.

79eac1c

add notes

da7d224

Merge pull request #3 from adlius/feature/node-preprint-divorce

dcf190b

[NPD] Add notes

Syntax cleanup

c9f46a3

Merge pull request #4 from alexschiller/feature/node-preprint-divorce

0441332

Syntax cleanup

pattisdr changed the title ~~[Ready for Review] Node-Preprint Divorce Ember-OSF [IN-98]~~ Feature/Node-Preprint Divorce Ember-OSF [IN-98] Oct 24, 2018

jamescdavis approved these changes Nov 28, 2018

View reviewed changes

jamescdavis merged commit 3506566 into CenterForOpenScience:develop Nov 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/Node-Preprint Divorce Ember-OSF [IN-98] #417

Feature/Node-Preprint Divorce Ember-OSF [IN-98] #417

pattisdr commented Sep 23, 2018 •

edited

Loading

pattisdr Sep 30, 2018

pattisdr Sep 30, 2018

adlius Oct 1, 2018 •

edited

Loading

pattisdr Oct 1, 2018

adlius Oct 1, 2018

pattisdr Oct 1, 2018

pattisdr Sep 30, 2018

pattisdr Sep 30, 2018

adlius Oct 1, 2018

pattisdr Oct 1, 2018

adlius Oct 9, 2018

adlius commented Oct 1, 2018

jamescdavis left a comment

		let node = this.store.peekRecord('node', nodeId);
		if (!node) {

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

		export default Ember.Mixin.create({

Feature/Node-Preprint Divorce Ember-OSF [IN-98] #417

Feature/Node-Preprint Divorce Ember-OSF [IN-98] #417

Conversation

pattisdr commented Sep 23, 2018 • edited Loading

Related PR's

Purpose

Summary of Changes/Side Effects

Testing Notes

Ticket

Notes for Reviewer

Reviewer Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adlius Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adlius commented Oct 1, 2018

jamescdavis left a comment

Choose a reason for hiding this comment

pattisdr commented Sep 23, 2018 •

edited

Loading

adlius Oct 1, 2018 •

edited

Loading