[EOSF-574] controller unit tests #319
Conversation
…ints into feature/controller-unit-tests
…ints into feature/controller-unit-tests
…computed properties, add tests
There was a problem hiding this comment.
Nice work @binoculars. These are big contributions to the submit/content controller tests as well as submit controller cleanup. I tried to go over this pretty thoroughly and only found a couple of bugs introduced on the submit controller. Main thing I see for QA is testing the Add Preprints page.
app/controllers/submit.js
Outdated
|
|
||
| if (this.get('applyLicense')) { | ||
| if (node.get('nodeLicense.year') !== this.get('basicsLicense.year') || node.get('nodeLicense.copyrightHolders') !== newCopyrightHolders) { | ||
| node.set('nodeLicense', {year: this.get('basicsLicense.year'), copyright_holders: newCopyrightHolders}); | ||
| if (node.get('nodeLicense.year') !== this.get('basicsLicense.year') || node.get('nodeLicense.copyrightHolders').join() !== copyrightHolders.join()) { |
There was a problem hiding this comment.
Getting errors on this line. nodeLicense.copyrightHolders if undefined. Cannot read property 'join' of undefined. Happens if you go back to edit section on Submit form.
There was a problem hiding this comment.
@binoculars - this is still broken.
There was a problem hiding this comment.
My apologies. I misunderstood the test case you mentioned.
| }), | ||
|
|
||
| basicsDOI: Ember.computed.or('model.doi'), |
There was a problem hiding this comment.
Why 'or' here? Is there something else that gives you a one-liner checking for existence?
There was a problem hiding this comment.
For some reason, when using Ember.computed.alias the validation breaks, but Ember.computed.or works just fine--weird
app/controllers/submit.js
Outdated
| node.set('title', nodeTitle); | ||
| return node.save(); | ||
| }) | ||
| //.then(() => this.send(this.get('abandonedPreprint') ? 'resumeAbandonedPreprint' : 'startPreprint')) |
There was a problem hiding this comment.
This line is needed. Without it, you cannot initiate a preprint on an existing node with an existing file.
app/controllers/submit.js
Outdated
|
|
||
| // Cannot be called until a project has been selected! | ||
| // Cannot be called until a project has been selected! |
app/controllers/submit.js
Outdated
| node.set('title', currentTitle); | ||
| this.get('toast').error(this.get('i18n').t('submit.could_not_update_title')); | ||
| }); | ||
| // Ember.run(() => { |
app/controllers/submit.js
Outdated
|
|
||
| const tags = this.get('basicsTags'); | ||
| tags.removeAt(tags.indexOf(tag)); |
| _fileDownloadURL: Ember.observer('model.primaryFile', function() { | ||
| this.get('model.primaryFile').then(file => { | ||
| this.set('fileDownloadURL', fileDownloadPath(file, this.get('node'))); | ||
| fileDownloadURL: Ember.computed('model.primaryFile', 'node', function() { |
There was a problem hiding this comment.
Nice job getting rid of that observer.
| }); | ||
|
|
||
| // test('existingNodeExistingFile', function(assert) { |
There was a problem hiding this comment.
skip instead of commented out test? When I put the first draft of tests in here, I was asked to use skip instead of commenting so they were more visible.
| test('error', function(assert) { | ||
| // TODO How to properly test this? The error action creates an error | ||
| // toast message displaying the error message | ||
| // test('changesSaved temporarily changes currentPanelSaveState to true', function(assert) { |
app/controllers/submit.js
Outdated
| this.get('toast').error(this.get('i18n').t('submit.could_not_update_title')); | ||
| }); | ||
| // Ember.run(() => { | ||
| return Promise.resolve() |
There was a problem hiding this comment.
For my own understanding, can you explain why you used Promise.resolve() here?
There was a problem hiding this comment.
It just starts a promise chain, because we need it for the return on L520 so that it's a 'thenable'. It could also be written as:
(() => {
if (currentTitle === nodeTitle) {
return Promise.resolve();
}
node.set('title', nodeTitle);
return node.save(); // returns a promise
})() // immediately invoked function
.then( ... )
.catch( ... )Once you start the promise chain, you can just use return to go to the next .then without returning a value
|
Some of this is friday afternoon madness. Update forthcoming |
…ints into feature/controller-unit-tests
|
Looks good to me @binoculars |
Ticket
https://openscience.atlassian.net/browse/EOSF-574
Purpose
Adds/updates tests for controllers
Changes
Note: Holding off on tests for actions with promises in them until we figure out a good way to handle those without concurrency issues
Notes for QA
Side effects