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

Remove not required SharedService class #24

Merged
merged 2 commits into from
Aug 24, 2017
Merged

Remove not required SharedService class #24

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b80c51
Remove not required SharedService class
Aug 24, 2017
f2167b6
Support updating layout settings after page navigation is completed
Aug 24, 2017
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
1 change: 0 additions & 1 deletion saas/axops/src/ui/src/app/services/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,3 @@ export { SlackService } from './slack.service';
export * from './playground-info.service';
export * from './system-request.service';
export * from './retention-policy.service';
export * from './shared.service';
2 changes: 0 additions & 2 deletions saas/axops/src/ui/src/app/services/services.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ import { PlaygroundInfoService } from './playground-info.service';
import { TrackingService } from './tracking.service';
import { SlackService } from './slack.service';
import { SystemRequestService } from './system-request.service';
import { SharedService } from './shared.service';

@NgModule({
providers: [
Expand Down Expand Up @@ -98,7 +97,6 @@ import { SharedService } from './shared.service';
GlobalSearchService,
SecretService,
TrackingService,
SharedService,
PlaygroundInfoService,
{
provide: ContentService,
Expand Down
11 changes: 0 additions & 11 deletions saas/axops/src/ui/src/app/services/shared.service.ts
View file
Open in desktop

This file was deleted.

9 changes: 2 additions & 7 deletions saas/axops/src/ui/src/app/views/+user-management/user-profile/user-profile.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Router, ActivatedRoute } from '@angular/router';
import { Subscription } from 'rxjs';

import { User } from '../../../model';
import { UsersService, AuthenticationService, SharedService } from '../../../services';
import { UsersService, AuthenticationService } from '../../../services';
import { LayoutSettings, HasLayoutSettings } from '../../layout';
import { UserUtils } from '../user-utils';

Expand All @@ -27,8 +27,7 @@ export class UserProfileComponent implements OnInit, OnDestroy, LayoutSettings,
private usersService: UsersService,
private authenticationService: AuthenticationService,
private activatedRoute: ActivatedRoute,
private utils: UserUtils,
private sharedService: SharedService) {
private utils: UserUtils) {
}

public get layoutSettings(): LayoutSettings {
Copy link
Contributor

@wokeGit wokeGit Aug 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

LayoutSettings is not working correct in this scenario.
To reproduce a bug I found:
Please go to Users -> select yourself account, you will be redirected to your profile screen -> click the action button. Here you can see two options at the dropdown:
screen shot 2017-08-24 at 09 48 51
It's a bug, you shouldn't be able to "Grant Admin Access".
What's more, you can click "user profile" then cancel it and now the "Grant Admin Access" action is not available.

If we can use Layout setting like this:

    get layoutSettings(): LayoutSettings {
        return this;
    }

    get globalAddActionMenu() {
        return this.utils.getActionMenu(this.user, item => this.editUserProfile(item.username));
    }

    get pageTitle(): string {
        return 'Profile';
    };

    get breadcrumb(): { title: string, routerLink: any[] }[] {
        return [{
            title: 'All Users',
            routerLink: [`/app/user-management/overview`]
        }, {
            title: this.username,
            routerLink: null,
        }];
    }

I know we were discussing to not use LayoutSettings this way but looks like in this particular scenario it works.
@alexmt Please let me know if you have better idea for this issue.

Don't know if described issue is good place to mention it here. But SharedService was created to resolve ie. this issue.
@alexmt Let me know if I should create separate issue for that.
Everything other looks good to me, thank you for removing SharedService.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for explanation @wokeGit ! Now I understand why SharedService was created. I've updated LayoutComponent so layout setting might be changed after navigation end event: f2167b6

Please take a look

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!
LGTM

Expand Down Expand Up @@ -84,15 +83,11 @@ export class UserProfileComponent implements OnInit, OnDestroy, LayoutSettings,
}

private async loadUser() {
let emitToLayout = !this.user.id;
await this.usersService.getUser(this.username).subscribe(result => {
this.user = result;
if (this.username === this.authenticationService.getUsername()) {
this.isCurrentLoggedInUser = true;
}
if (emitToLayout) {
this.sharedService.updateSource.next(this.layoutSettings);
}
});
}
}
8 changes: 1 addition & 7 deletions saas/axops/src/ui/src/app/views/layout/layout.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
SecretService,
ViewPreferencesService,
AuthenticationService,
SharedService
} from '../../services';
import { Task, Application } from '../../model';

Expand Down Expand Up @@ -101,8 +100,7 @@ export class LayoutComponent implements OnInit, OnDestroy {
private playgroundInfoService: PlaygroundInfoService,
private notificationService: NotificationService,
private viewPreferencesService: ViewPreferencesService,
private authenticationService: AuthenticationService,
private sharedService: SharedService) {
private authenticationService: AuthenticationService) {

this.encryptForm = new FormGroup({
repo: new FormControl('', Validators.required),
Expand Down Expand Up @@ -144,10 +142,6 @@ export class LayoutComponent implements OnInit, OnDestroy {
this.playgroundTask = info;
}));

this.subscriptions.push(this.sharedService.updateSource.subscribe((layout) => {
this.layoutSettings = layout;
}));

this.authenticationService.getCurrentUser().then(user => {
this.subscriptions.push(Observable.merge(
this.notificationService.getEventsStream(user.username),
Expand Down