Skip to content

Feature: certificate implementation#1176

Merged
johnnyzhang82 merged 23 commits into
masterfrom
feature/cert
Mar 22, 2018
Merged

Feature: certificate implementation#1176
johnnyzhang82 merged 23 commits into
masterfrom
feature/cert

Conversation

@johnnyzhang82
Copy link
Copy Markdown
Contributor

@johnnyzhang82 johnnyzhang82 commented Mar 17, 2018

Fix #1165. npmImplement certificate interface for BatchLab including:

  1. List/Get certificate
  2. Create certificate
  3. Delete certificate
  4. Cancel deletion of a certificate

image

Comment thread app/app.routes.ts Outdated
canActivate: [NavigationGuard],
component: CertificateHomeComponent,
children: [
{ path: "", component: CertificateDefaultComponent }, // thumbprint/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

comment should be certificates/ no?

Comment thread package.json Outdated
"mousetrap": "^1.6.0",
"ms-rest-js": "~0.2.3",
"node-fetch": "~1.7.3",
"node-forge": "^0.7.4",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just registered for OSS approval

Comment thread app/services/certificate.service.ts Outdated
import { List } from "immutable";
import { AsyncSubject, Observable, Subject } from "rxjs";
// tslint:disable-next-line:no-var-requires
const forge = require("node-forge");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

es6 imports

Comment thread app/services/certificate.service.ts Outdated
* @param file
*/
public getCertificateExtension(file: File) {
return file.name.substr(file.name.length - 3, 3).toLowerCase();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use FileUrlUtils#getFileExtension()

return this._basicProperties;
}

public list(options?: any, forceNew?: boolean);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is there actually a listNext for certificates?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is nextLink in certificateListResult
image

@@ -0,0 +1,3 @@
<bl-simple-form [submit]="ok" [containerRef]="dialogRef" [multiUse]="false" size="small" actionName="Enable" title="Reactivate certificate">
<p>Do you want to reactivate the certificate'<b>{{certificateThumbprint}}</b>'?</p>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

need space or ': ' between here:

certificate'{{certificateThumbprint}}'

import { ContextMenu, ContextMenuItem } from "@batch-flask/ui/context-menu";
import { LoadingStatus } from "@batch-flask/ui/loading";
import { QuickListItemStatus } from "@batch-flask/ui/quick-list";
import { Certificate, CertificateState } from "app/models";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have been adding a line break between @batch-flask imports and the other app imports. I think it makes it cleaner and easier to read.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@batch-flask imports can be added to the external imports above, as its techincally supposed to act like an external library

case CertificateState.active:
return "";
default:
return `Certficate is ${certificate.state}`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Certificate

import { ActivatedRoute, Router } from "@angular/router";
import { autobind } from "@batch-flask/core";
import { ElectronRemote } from "@batch-flask/ui";
import { Observable, Subscription } from "rxjs";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

move flask imports out of this block

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think they belong here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok cool. if you are happy then i am also :)

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 19, 2018

Codecov Report

Merging #1176 into master will decrease coverage by 0.28%.
The diff coverage is 21.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1176      +/-   ##
==========================================
- Coverage   53.66%   53.38%   -0.29%     
==========================================
  Files         941      946       +5     
  Lines       21343    21538     +195     
  Branches     2364     2394      +30     
==========================================
+ Hits        11454    11497      +43     
- Misses       9889    10041     +152
Impacted Files Coverage Δ
src/common/constants/constants.ts 100% <ø> (ø) ⬆️
...inned-entity-dropdown/pinned-dropdown.component.ts 81.03% <0%> (-6.01%) ⬇️
app/services/batch-api/jobScheduleProxy.ts 10.52% <0%> (ø) ⬆️
app/models/index.ts 100% <100%> (ø) ⬆️
app/services/pinned-entity.service.ts 97.46% <100%> (+0.03%) ⬆️
app/models/decorators/index.ts 100% <100%> (ø) ⬆️
src/@batch-flask/core/record/navigable-record.ts 100% <100%> (ø) ⬆️
app/services/index.ts 100% <100%> (ø) ⬆️
...s/decorators/delete-certificate-error-decorator.ts 11.76% <11.76%> (ø)
app/services/certificate.service.ts 12.87% <12.87%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8a3fef...05e877e. Read the comment docs.

@johnnyzhang82 johnnyzhang82 merged commit 33bf74b into master Mar 22, 2018
@johnnyzhang82 johnnyzhang82 deleted the feature/cert branch March 22, 2018 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants