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

fix: Replace public implementation to display delegatee info with platform #3172

Merged
merged 13 commits into from
Aug 20, 2024

Conversation

harshal015
Copy link
Collaborator

Clickup

Please add link here
app.clickup.com

Code Coverage

Please add code coverage here

UI Preview

Please add screenshots for UI changes

@github-actions github-actions bot added size/M Medium PR and removed size/S Small PR labels Aug 16, 2024
@harshal015 harshal015 requested a review from mvaishnavi August 16, 2024 05:29
<div *ngIf="acc?.ou.org_id === currentOrg.id" class="delegated--org-element-us-details">
{{currentOrg.name}}
</div>
<div class="delegated--org-element-us-details">{{currentOrg.name}}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

why have we removed the condition here? @harshal015

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it'll be anyway visible for all delegators as the API will respond with delegators from same org.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can entirely remove the org name as well

this.isDelegateePresent$ = from(this.authService.getEou()).pipe(map((eou) => eou.ou.delegatee_id !== null));
this.isDelegateePresent$ = from(this.authService.getEou()).pipe(
switchMap((res) => this.employeesService.getByParams({ user_id: `eq.${res.us.id}` })),
map((employee) => employee.data.delegatees.length > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

If delegatees is null this would break right?
We have mentioned delegatees as optional in model and also as per docs.fylehq.com it could be null.
Use optional chaining.

@@ -95,6 +103,7 @@ describe('NotificationsPage', () => {
});

orgUserSettingsService.get.and.returnValue(of(orgUserSettingsData));
employeesService.getByParams.and.returnValue(of());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we not returning anything?
Either it should return employees or throw an error

<div *ngIf="acc?.ou.org_id === currentOrg.id" class="delegated--org-element-us-details">
{{currentOrg.name}}
</div>
<div class="delegated--org-element-name">{{acc?.full_name}}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if acc is undefined or null?
Are we handling the null cases as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we are displaying switch accounts page only if its non empty, so it'll always have a value

@github-actions github-actions bot added size/L Large PR and removed size/M Medium PR labels Aug 20, 2024
@harshal015 harshal015 requested a review from mvaishnavi August 20, 2024 14:17
Copy link

Unit Test Coverage % values
Statements 95.85% ( 19052 / 19875 )
Branches 91.1% ( 10432 / 11450 )
Functions 94.24% ( 5700 / 6048 )
Lines 95.89% ( 18185 / 18964 )

@harshal015 harshal015 merged commit 6731a9e into master Aug 20, 2024
6 of 7 checks passed
harshal015 added a commit that referenced this pull request Aug 20, 2024
* replace delegatee info from eou

* minor

* switch to delegator changes

* fix unit tests

* fix employee service unit tests

* more fixes

* switch to delegator changes

* eslint fixes

* more fixes

* minor

* add safety
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Large PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants