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.

Sign up for GitHub

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

Jump to bottom

[Oauth] Developers page #15031

Merged
merged 87 commits into from
Sep 3, 2019
Merged

[Oauth] Developers page #15031

merged 87 commits into from
Sep 3, 2019

Conversation

csubira
Copy link
Contributor

@csubira csubira commented Jul 25, 2019 •
edited
Loading

houndci-bot
houndci-bot reviewed Jul 25, 2019
View reviewed changes
lib/assets/javascripts/new-dashboard/store/modules/apps/index.js Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Jul 25, 2019
View reviewed changes
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/OauthApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/SettingsSidebar.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/SettingsSidebar.vue Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Jul 29, 2019
View reviewed changes
lib/assets/javascripts/new-dashboard/router/index.js Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/ConnectedApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/ConnectedApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/ConnectedApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/ConnectedApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Jul 29, 2019
View reviewed changes
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppCard.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppCard.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppCard.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppCard.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppCard.vue Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Jul 30, 2019
View reviewed changes
lib/assets/javascripts/new-dashboard/pages/Apps/ConnectedApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/ConnectedApps.vue
</ul>
</div>
</div>
</div>
Copy link

Choose a reason for hiding this comment

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

Parsing error: x-invalid-end-tag vue/no-parsing-error

lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Jul 31, 2019
View reviewed changes
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/components/Apps/AppForm.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/store/modules/apps/connected/index.js Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/ConnectedApps.vue Outdated Show resolved Hide resolved
lib/assets/javascripts/new-dashboard/pages/Apps/ConnectedApps.vue Outdated Show resolved Hide resolved
jesusbotella
jesusbotella reviewed Sep 2, 2019
View reviewed changes
Copy link
Contributor

@jesusbotella jesusbotella left a comment

Choose a reason for hiding this comment

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

Just minor things that we need to check out! :)

lib/assets/javascripts/dashboard/data/api-key-model.js
@@ -149,7 +149,7 @@ module.exports = Backbone.Model.extend({
_parseDatabaseSchemas: function (grants) {
const schemas = _.find(grants, grant => grant.type === GRANT_TYPES.DATABASE).schemas;
const publicSchema = _.find(schemas, schema => schema.name === 'public');
return publicSchema && publicSchema.permissions && publicSchema.permissions.indexOf('create') > -1;
return !!(publicSchema && publicSchema.permissions && publicSchema.permissions.indexOf('create') > -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to do that? 🤔
The comparison returns a boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I thought, but when I implemented the tests I saw that it returned undefined when there is no publicSchema (considering_.find returns undefined when not found)

lib/assets/test/spec/dashboard/views/api-keys/api-keys-form-view.spec.js
view._onFormChanged();

expect(view._validationTooltip.clean).toHaveBeenCalled();
expect(view._validationTooltip).toBe(null);
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 we should check that the clean action is performed as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

lib/assets/javascripts/dashboard/views/api-keys/api-keys-form-view.js
this.$('.js-error').hide();
},

_onClickCreateCheckbox: function () {
const createCheckbox = this._formView.getValue().datasets.create;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really get what you're trying to do here. Isn't this change performed by backbone forms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced it for an event on change this checkboxes (as we discussed) and it has to do with forcing SQL checkbox to be checked when CREATE is checked and uncheck CREATE if SQL is unchecked.

@csubira csubira merged commit abc9ee2 into master Sep 3, 2019
@csubira csubira deleted the 418prod-oauth-dev branch September 3, 2019 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@jesusbotella jesusbotella jesusbotella approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@csubira @jesusbotella @houndci-bot