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

feat: fetch provider list on portal based on selected application #373

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions apps/portal/src/app/graphql/graphql.queries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,33 @@ export const GetApplications = gql`
}
`;

export const GetProviders = gql`
query GetProviders($limit: Int!, $offset: Int!, $filters: [UniversalFilter!]) {
providers(
options: {
limit: $limit
offset: $offset
sortBy: "createdOn"
sortOrder: ASC
filters: $filters
}
) {
providers {
applicationId
channelType
createdOn
name
providerId
status
updatedOn
}
total
offset
limit
}
}
`;

export const LoginUser = gql`
mutation LoginUser($username: String!, $password: String!) {
login(loginUserInput: { username: $username, password: $password }) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ <h2 class="p-col-12">Notifications</h2>
<div class="p-grid">
<div class="p-col-12 p-md-3 filter-row">
<p-dropdown
[options]="channelTypes"
[(ngModel)]="selectedChannelType"
placeholder="Select a Channel Type"
[options]="providers"
[(ngModel)]="selectedProvider"
placeholder="Select a Provider"
class="grid-dropdown"
[showClear]="true"
(onChange)="loadNotificationsLazy({ first: 0, rows: this.pageSize })"
Expand Down Expand Up @@ -52,7 +52,10 @@ <h2 class="p-col-12">Notifications</h2>
placeholder="Select an Application"
class="grid-dropdown"
[showClear]="false"
(onChange)="loadNotificationsLazy({ first: 0, rows: this.pageSize })"
(onChange)="
loadNotificationsLazy({ first: 0, rows: this.pageSize });
getProvidersForSelectedApplication()
"
*ngIf="applications.length !== 0"
></p-dropdown>
</div>
Expand Down
129 changes: 100 additions & 29 deletions apps/portal/src/app/views/notifications/notifications.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { NotificationsService } from './notifications.service';
import { Notification, NotificationResponse } from './notification.model';
import { ApplicationsService } from '../applications/applications.service';
import { ApplicationResponse } from '../applications/application.model';
import { ProvidersService } from '../providers/providers.service';
import { ProviderResponse } from '../providers/provider.model';

@Component({
selector: 'app-notifications',
Expand Down Expand Up @@ -36,24 +38,21 @@ export class NotificationsComponent implements OnInit {
};

// for component
channelTypes = Object.entries(ChannelType).map(([, value]) => ({
label: `${this.channelTypeMap[value].altText} - ${this.channelTypeMap[value].providerName}`,
value,
}));

deliveryStatuses = Object.entries(DeliveryStatus).map(([, value]) => ({
label: this.deliveryStatusMap[value].value,
value,
}));

applications = [];

selectedChannelType = null;
providers = [];

selectedDeliveryStatus = null;

selectedApplication = null;

selectedProvider = null;

selectedFromDate = null;

selectedToDate = null;
Expand Down Expand Up @@ -85,6 +84,7 @@ export class NotificationsComponent implements OnInit {
constructor(
private notificationService: NotificationsService,
private applicationService: ApplicationsService,
private providerService: ProvidersService,
private messageService: MessageService,
private authService: AuthService,
) {}
Expand Down Expand Up @@ -177,6 +177,9 @@ export class NotificationsComponent implements OnInit {

// Now that applications are loaded, load notifications
this.loadNotificationsLazy({ first: 0, rows: this.pageSize });

// Load providers for selected application
this.getProvidersForSelectedApplication();
});
}

Expand Down Expand Up @@ -225,6 +228,89 @@ export class NotificationsComponent implements OnInit {
}
}

getProvidersForSelectedApplication() {
// Set the query variables
const variables = {
filters: [],
offset: 0,
limit: 15,
};

if (!this.selectedApplication) {
// Handle missing selected application
return;
}

// Set query filters
variables.filters.push({
field: 'applicationId',
operator: 'eq',
value: this.selectedApplication.toString(),
});

// Fetch the login token
const loginToken = this.getJWTLoginToken();

if (!loginToken) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there is no login token we should be logging out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Assuming its handled in getJWTLoginToken()

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 not being handled in getJWTLoginToken()
However we do have a logout function, maybe we can create new PR for it

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kshitij-k-osmosys Okay can do in New PR.

// Handle missing token
return;
}
Comment on lines +271 to +274
Copy link

@coderabbitai coderabbitai bot Jan 17, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling for missing login token.

The error handling for missing login token should follow the same pattern as other error cases in the component.

     if (!loginToken) {
-      // Handle missing token
+      this.messageService.add({
+        key: 'tst',
+        severity: 'error',
+        summary: 'Error',
+        detail: 'Login token is missing. Please log in again.',
+      });
+      this.loading = false;
       return;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!loginToken) {
// Handle missing token
return;
}
if (!loginToken) {
this.messageService.add({
key: 'tst',
severity: 'error',
summary: 'Error',
detail: 'Login token is missing. Please log in again.',
});
this.loading = false;
return;
}

Copy link
Contributor Author

@kshitij-k-osmosys kshitij-k-osmosys Jan 17, 2025

Choose a reason for hiding this comment

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

Handled in different PR
We log out in this scenario
https://github.com/OsmosysSoftware/osmo-x/pull/373/files#r1887016153

Copy link

Choose a reason for hiding this comment

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

@kshitij-k-osmosys Understood. Thanks for clarifying that the missing login token scenario is handled by logging out the user.

                🐰
               ╱|、
              (˚ˎ 。7  
               |、˜〵          
               じしˍ,)ノ
               *thinking*

✏️ Learnings added
Learnt from: kshitij-k-osmosys
PR: OsmosysSoftware/osmo-x#373
File: apps/portal/src/app/views/notifications/notifications.component.ts:271-274
Timestamp: 2025-01-17T08:16:12.033Z
Learning: In the OsmoX portal application, when a login token is missing, the user should be logged out instead of showing an error message.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


// Fetch providers and handle errors
this.providerService
.getProviders(variables, loginToken)
.pipe(
// catchError operator to handle errors
catchError((error) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't have common code for this code block? Can add a TODO task for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we create an issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kshitij-k-osmosys No need, directly create a task in pinestem for now in backlog.

this.messageService.add({
key: 'tst',
severity: 'error',
summary: 'Error',
detail: `There was an error while loading providers. Reason: ${error.message}`,
});
return of(null); // Return null to indicate error
}),
)
.subscribe((providerResponse: ProviderResponse | null) => {
if (providerResponse?.errors?.length) {
const unauthorizedError = providerResponse.errors.find(
(error) => error.message === 'Unauthorized',
);

this.providers = [];

if (unauthorizedError) {
this.messageService.add({
key: 'tst',
severity: 'error',
summary: 'Error',
detail: 'Unauthorized access. Please log in again.',
});
this.authService.logoutUser();
} else {
providerResponse.errors.forEach((error) => {
this.messageService.add({
key: 'tst',
severity: 'error',
summary: 'Error',
detail: `GraphQL Error - Get Providers: ${error.message}`,
});
});
}
} else if (providerResponse?.providers?.length) {
// Fetch list of providers for dropdown
this.providers = providerResponse.providers.map((obj) => ({
// Name to display and ID to return upon selection
label: `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}`,
Copy link

@coderabbitai coderabbitai bot Dec 18, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add null check for channelType mapping

The code assumes that the provider's channelType will always exist in channelTypeMap. Add a null check to handle potential missing mappings.

- label: `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}`,
+ label: `${obj.name}${obj.channelType && this.channelTypeMap[obj.channelType]?.altText ? ` - ${this.channelTypeMap[obj.channelType].altText}` : ''}`,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
label: `${obj.name} - ${this.channelTypeMap[obj.channelType].altText}`,
label: `${obj.name}${obj.channelType && this.channelTypeMap[obj.channelType]?.altText ? ` - ${this.channelTypeMap[obj.channelType].altText}` : ''}`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 4fd8b8f

Copy link

Choose a reason for hiding this comment

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

@kshitij-k-osmosys Thank you for implementing the fix! Your solution with the fallback to ChannelType.UNKNOWN is even better than my suggestion as it ensures there's always a meaningful label displayed. Well done!

🔗 Analysis chain

@kshitij-k-osmosys Thank you for implementing the fix! The code is now more robust in handling missing channel type mappings.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Fetch the specific commit details
git show 4fd8b8ff412c14a6cf8bb85b304dbab3a6ffbf94

Length of output: 1212


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

value: obj.providerId,
}));
} else {
this.providers = [];
this.selectedProvider = null;
}
});
}

loadNotificationsLazy(event: LazyLoadEvent) {
this.loading = true;

Expand Down Expand Up @@ -257,29 +343,6 @@ export class NotificationsComponent implements OnInit {
value: this.selectedApplication.toString(),
});

if (this.selectedChannelType) {
if (this.selectedChannelType === this.allPortalChannelTypes.UNKNOWN) {
// Condition to filter all notifications with unknown channel type
const existingChannelTypes = Object.keys(this.channelTypeMap).filter(
(value) => value !== this.allPortalChannelTypes.UNKNOWN.toString(),
);
existingChannelTypes.forEach((key) => {
variables.filters.push({
field: 'channelType',
operator: 'ne',
value: key.toString(),
});
});
} else {
// Default behavior when we are sorting on known channelType
variables.filters.push({
field: 'channelType',
operator: 'eq',
value: this.selectedChannelType.toString(),
});
}
}

if (this.selectedDeliveryStatus) {
variables.filters.push({
field: 'deliveryStatus',
Expand All @@ -288,6 +351,14 @@ export class NotificationsComponent implements OnInit {
});
}

if (this.selectedProvider) {
variables.filters.push({
field: 'providerId',
operator: 'eq',
value: this.selectedProvider.toString(),
});
}

if (this.selectedFromDate) {
variables.filters.push({
field: 'createdOn',
Expand Down
18 changes: 18 additions & 0 deletions apps/portal/src/app/views/providers/provider.model.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { GraphQLFormattedError } from 'graphql/error/GraphQLError';

export interface Provider {
providerId: number;
name: string;
channelType: number;
createdOn: Date;
updatedOn: Date;
status: number;
}

export interface ProviderResponse {
providers: Provider[];
total: number;
offset: number;
limit: number;
errors?: ReadonlyArray<GraphQLFormattedError>;
}
42 changes: 42 additions & 0 deletions apps/portal/src/app/views/providers/providers.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { Injectable } from '@angular/core';
import { Observable, catchError, map } from 'rxjs';
import { GraphqlService } from 'src/app/graphql/graphql.service';
import { GetProviders } from 'src/app/graphql/graphql.queries';
import { ApolloQueryResult } from '@apollo/client/core';
import { Provider, ProviderResponse } from './provider.model';

interface GetProvidersResponse {
providers: {
providers?: Provider[];
total?: number;
offset?: number;
limit?: number;
};
}
@Injectable({
providedIn: 'root',
})
export class ProvidersService {
constructor(private graphqlService: GraphqlService) {}

getProviders(variables, inputToken): Observable<ProviderResponse> {
return this.graphqlService.query(GetProviders, variables, inputToken).pipe(
map((response: ApolloQueryResult<GetProvidersResponse>) => {
const providerArray = response.data?.providers.providers;

const providerResponseObject: ProviderResponse = {
providers: providerArray,
total: response.data?.providers.total,
offset: response.data?.providers.offset,
limit: response.data?.providers.limit,
errors: response.errors || null,
};
return providerResponseObject;
}),
catchError((error) => {
const errorMessage: string = error.message;
throw new Error(errorMessage);
}),
);
}
}
Loading