Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENG-5145] converge with gravyvalet backend #2234

Conversation

aaxelb
Copy link
Contributor

@aaxelb aaxelb commented Jun 7, 2024

Purpose

Summary of Changes

  • addons_service adapter: include auth cookies with requests
  • consistent displayName attr on external services, authorized accounts, and configured addons
  • add thruAccount relation to addon-operation-invocations (to allow the account owner to e.g. choose an authorized root folder without having configured an addon yet)
  • some renames for consistency
  • skip unused credentials members (allow/default to {})

Screenshot(s)

Side Effects

QA Notes

@aaxelb aaxelb changed the title [wip] converge with gravyvalet backend [ENG-5145] converge with gravyvalet backend Jun 11, 2024
@aaxelb aaxelb marked this pull request as ready for review June 11, 2024 18:19
@aaxelb aaxelb requested a review from futa-ikeda June 11, 2024 18:19
Comment on lines +161 to +164
const resourceRefs = await this.store.query('resource-reference', {
filter: {resource_uri: this.node.links.iri?.toString()},
});
this.serviceNode = resourceRefs.firstObject;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the peekRecord not work properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

peekRecord uses id, but gravyvalet mints its own ids and getting the gv resource-ref for an osf resource requires looking it up with filter[resource_uri] (...which is less than ideal but where we've landed for now, it seems -- might be cleaner if it were fetched/cached on the node model?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would still be worth doing a peekRecord here and for the userReference, but now with the correct GV ids. We instantiate this class quite a bit and taking advantage of the caching would be nice. Unless query checks for any cached records first?

Comment on lines +143 to +146
if (providerCapabilities?.includes(capability)) {
textTranslationKey = textTranslationChoices.true;
localClass = 'success-bg';
} else if (providerCapabilities.includes((capability + '_partial' as TermsOfServiceCapabilities))) {
} else if (providerCapabilities?.includes((capability + '_partial' as TermsOfServiceCapabilities))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to get the providerCapabilities in so we don't have those ?s

Copy link
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huzzah! Thank you for helping with the integration!

Comment on lines +147 to +148
assert.dom('[data-test-files-provider-link="bitbucket"]').exists('Bitbucket shown');
assert.dom('[data-test-files-provider-link="bitbucket"]').hasAttribute('href',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did this pass without the closing brackets? 🤨

@brianjgeiger brianjgeiger merged commit 9fea8b2 into CenterForOpenScience:feature/addon-services Jun 12, 2024
9 checks passed
@@ -216,9 +214,8 @@ export default class Provider {
initiateOauth: initiateOauth || false,
apiBaseUrl: (this.provider as ExternalStorageServiceModel).configurableApiRoot ? credentials.url : '',
externalUserId: this.currentUser.user?.id,
scopes: [],
storageProvider: this.provider,
configuringUser: this.userReference,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brianjgeiger it looks like the removal of this line is what's messing up the user settings page. should add accountOwner: this.userReference here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants