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

Feature/roles #1709

Merged
merged 11 commits into from
Aug 24, 2023
Merged

Feature/roles #1709

merged 11 commits into from
Aug 24, 2023

Conversation

szczygiel-m
Copy link
Contributor

No description provided.

@szczygiel-m szczygiel-m temporarily deployed to ci August 18, 2023 13:03 — with GitHub Actions Inactive
@szczygiel-m szczygiel-m temporarily deployed to ci August 18, 2023 14:09 — with GitHub Actions Inactive
@szczygiel-m szczygiel-m temporarily deployed to ci August 18, 2023 14:31 — with GitHub Actions Inactive
@szczygiel-m szczygiel-m temporarily deployed to ci August 21, 2023 11:21 — with GitHub Actions Inactive
@szczygiel-m szczygiel-m temporarily deployed to ci August 21, 2023 13:26 — with GitHub Actions Inactive
# Conflicts:
#	hermes-console-vue/src/api/hermes-client/index.ts
#	hermes-console-vue/src/mocks/handlers.ts
@szczygiel-m szczygiel-m marked this pull request as ready for review August 22, 2023 11:55
@@ -74,7 +77,7 @@
</v-col>
</v-row>

<v-row justify="center">
<v-row justify="center" v-if="roles?.roles.value?.includes(Roles.ADMIN)">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that top roles is not nullable?

Suggested change
<v-row justify="center" v-if="roles?.roles.value?.includes(Roles.ADMIN)">
<v-row justify="center" v-if="roles.roles.value?.includes(Roles.ADMIN)">

Copy link
Collaborator

Choose a reason for hiding this comment

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

also wondering whether fetching roles error should not result in banner with sth like:

could not fetch roles, some options may not be visible

right now we are silently ignoring roles errors which may be confusing for users (admins, topics owners and subscription owners)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I changed it, I am using function isAdmin from utils. I added TODO to send notification when fetching roles fails.

@@ -95,7 +97,9 @@
:subscription-metrics="subscriptionMetrics"
/>
<service-response-metrics />
<manage-messages-card />
<manage-messages-card
v-if="isSubscriptionOwnerOrAdmin(roles?.roles.value)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. is ? needed in all isSubscriptionOwnerOrAdmin(roles?.roles.value)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, otherwise some tests for this views are failing because some components cannot be rendered because of type error

@szczygiel-m szczygiel-m temporarily deployed to ci August 23, 2023 07:36 — with GitHub Actions Inactive
@@ -0,0 +1,6 @@
export const enum Roles {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Role*

@@ -10,6 +12,7 @@

const theme = useTheme();
const configStore = useAppConfigStore();
const roles = useRoles(null, null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

U can extract roles value

@GET
@Path("/console")
@Produces("application/javascript")
@ApiOperation(value = "Hermes console configuration", httpMethod = HttpMethod.GET)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this is open source project i would leave this and annotate it with @deprecated for some time I think

@szczygiel-m szczygiel-m merged commit 2a0e636 into hermes_console_vue Aug 24, 2023
@szczygiel-m szczygiel-m deleted the feature/roles branch August 24, 2023 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants