[ENG-6774] fix computing model / service names (and some citations, too!) #2437
Conversation
felliott
force-pushed
the
feature/fix-computing-names
branch
2 times, most recently
from
a058662 to
bbe7889
Compare
felliott
changed the title
| @@ -2350,7 +2350,7 @@ routes: | |||
| institution: Institution | |||
| email: Email | |||
| osf-components: | |||
| addons-srvice: | |||
| @belongsTo('user-reference', { inverse: 'authorizedComputingAccounts' }) | ||
| accountOwner!: AsyncBelongsTo<UserReferenceModel> & UserReferenceModel; | ||
|
|
||
| @belongsTo('external-computing-service') | ||
| computingService!: AsyncBelongsTo<ExternalComputingService> & ExternalComputingService; | ||
| externalComputingService!: AsyncBelongsTo<ExternalComputingServiceModel> & ExternalComputingServiceModel; |
There was a problem hiding this comment.
Ack, sorry for the inconsistent naming here!
There was a problem hiding this comment.
Again sorry for the inconsistent implementations! Should have taken the extra few seconds to update the computingService methods while I was in here last messing with the createAuthorizedXYZAccounts and createConfiguredXYZAddons
| get invalidDisplayName() { | ||
| return !this.displayName || this.displayName?.trim().length === 0; | ||
| } | ||
|
|
||
| get disableSave() { | ||
| return !this.selectedFolder || this.invalidDisplayName || this.args.onSave.isRunning; | ||
| return this.hasRootFolder && (!this.selectedFolder || this.invalidDisplayName || this.args.onSave.isRunning); |
There was a problem hiding this comment.
Would this condition always return false when this.args.authorizedAccount is an AuthorizedComputingAccount, meaning the save button is always enabled? Or have I confused myself with the logic?
I think it should be something like this (I've rearranged the checks to make it a little easier for myself):
| return this.hasRootFolder && (!this.selectedFolder || this.invalidDisplayName || this.args.onSave.isRunning); | |
| return this.invalidDisplayName || this.args.onSave.isRunning || (this.hasRootFolder && !this.selectedFolder) || ; |
There was a problem hiding this comment.
D'oh! Absolutely right. I will fix that up.
| {{t 'general.home'}} | ||
| </Button> | ||
| {{#each fileManager.currentPath as |pathItem|}} | ||
| {{#if this.hasRootFolder }} |
There was a problem hiding this comment.
I think this is a bit out of scope for this PR, but I'm wondering if hasRootFolder is false, we shouldn't need to invoke the FileManager component at all?
Meaning, right now, this template is structured (in pseudo-code) like
<FileManager>
{{#if hasRootFolder}}
...some file explorer stuff
<div>
<Button>Save</Button>
<Button>Cancel</Button>
</div>
{{/if}}
</FileManager>
But I think we can avoid a few unneeded (and possibly prone to fail) network calls by structuring it like:
{{#if hasRootFolder}}
<FileManager>
...some file explorer stuff
</FileManager>
{{/if}}
<div>
<Button>Save</Button>
<Button>Cancel</Button>
</div>
I don't want to scope-creep this PR, so I can make these changes in a separate PR if you would prefer!
There was a problem hiding this comment.
Lemme try it real quick, see how it goes.
There was a problem hiding this comment.
"real quick" == 3 hours apparently
felliott
force-pushed
the
feature/fix-computing-names
branch
from
bbe7889 to
3d4eaf3
Compare
* Add hasRootFolder getter to detect if root folder selector should be shown. * Rename `selectedAccount` to `authorizedAccount` to match the invocation syntax.
* Responding to code review, improve disableSaveButton check
* Responding to code review, {{#if}} around the entire FileManager
felliott
force-pushed
the
feature/fix-computing-names
branch
from
3d4eaf3 to
6535fdf
Compare
|
Rebased onto the current commit, and added checks for configured computing addons. |
adlius
merged commit 3d910e0
into
CenterForOpenScience:feature/addon-services
Purpose
The new Boa addon is trying to
POSTa relationship calledcomputing-servicerather thanexternal-computing-service. After investigation, it looks like some of the naming conventions for computing have gone stale and need to be updated.Summary of Changes
AuthorizedComputingAccount=>AuthorizedComputingAccountModelcomputingServicetoexternalComputingServiceto match the gv API.Screenshot(s)
nope
Side Effects
still testing, will let you know!
QA Notes
Don't think so.
Ticket
ENG-6774