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

NRPT-214: New Record Types #521

Merged
merged 6 commits into from
Jul 30, 2020
Merged

NRPT-214: New Record Types #521

merged 6 commits into from
Jul 30, 2020

Conversation

dhlevi
Copy link
Contributor

@dhlevi dhlevi commented Jul 29, 2020

Adding new record types for NRPTI:

  • Annual Report
  • Dam Safety Inspection
  • Report
  • Certificate Amendment
  • Correspondence

includes API, common, and UI components.

All record types contain the mineGuid ref, but currently there's no way to set this via the screens and it is not part of the existing ticket spec. We'll have to include a method of associating the guid for manual creation of these and other record types for use in BCMI.

There's a lot of code to look over and I'm sure I've missed a few spots or made mistakes, so I'm pinging everyone to have a look!

See tickets NRPT-214 (the UI) and NRPT-284 (The API)

this.lastEditedSubText = `Added on ${this.utils.convertJSDateToString(new Date(this.currentRecord.dateAdded))}`;
}
for (const flavour of this.currentRecord.flavours) {
switch (flavour._schemaName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe just an if in this case?

}

navigateToDetails() {
this.router.navigate(['records', 'annual-reports', this.currentRecord._id, 'detail']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Without the / in front of records this will navigate relative the route it is called from. Is that intended? Depending on where this screen is it might work as expected without the / too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the routers here I just copied it exactly from the other details/add-edit screens. Happy to make a change, but if so I should make the change to all of them, not just the five new ones... they all seem to work as expected for now though!


if (!this.isEditing) {
this.factoryService.createAnnualReport(annualReport).subscribe(async res => {
console.log(res);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log.

this.factoryService
);

console.log(docResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log.

.pipe(
takeUntil(this.ngUnsubscribe),
catchError(error => {
console.log('Publish error:', error);
Copy link
Contributor

@KitArmstrong KitArmstrong Jul 30, 2020

Choose a reason for hiding this comment

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

Should we log the error?


if (!this.isEditing) {
this.factoryService.createCertificateAmendment(certificateAmendment).subscribe(async res => {
console.log(res);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log.

this.factoryService
);

console.log(docResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log.

this.factoryService
);

console.log(docResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log.


if (!this.isEditing) {
this.factoryService.createCorrespondence(correspondence).subscribe(async res => {
console.log(res);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log

Copy link
Contributor

@KitArmstrong KitArmstrong left a comment

Choose a reason for hiding this comment

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

Tested well locally. Nice to see everything following the same approach. I left a couple comments about logs and errors. I did not note them in every component, but might need to check them all.

@dhlevi
Copy link
Contributor Author

dhlevi commented Jul 30, 2020

Tested well locally. Nice to see everything following the same approach. I left a couple comments about logs and errors. I did not note them in every component, but might need to check them all.

Fixed the log statements and other notes above. Checked all of the new pages and updated where appropriate across the board.

Early on in development, I thought it would be a much nicer idea to convert a lot of the duplicate flavour stuff, record screens and functions scattered in various other components (the huge case statements, ex.) into more generic common components. It's starting to get fairly unruly in some spots and much of it would be easy enough to convert to single functions with a type. Not the time to do it with this ticket, but it might be worth a technical debt ticket to see about updating a lot of this stuff into shared/common functions and components.

Copy link
Contributor

@KitArmstrong KitArmstrong left a comment

Choose a reason for hiding this comment

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

Looks good. Working as expected locally.

Copy link
Contributor

@mtCarto mtCarto left a comment

Choose a reason for hiding this comment

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

Only a few console logs to remove, otherwise looks great

this.factoryService
);

console.log(docResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove or use a logging util?

.pipe(
takeUntil(this.ngUnsubscribe),
catchError(error => {
console.log('Unpublish error:', error);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove console log

.pipe(
takeUntil(this.ngUnsubscribe),
catchError(error => {
console.log('Publish error:', error);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

.pipe(
takeUntil(this.ngUnsubscribe),
catchError(error => {
console.log('Unpublish error:', error);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

this.factoryService
);

console.log(docResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

.pipe(
takeUntil(this.ngUnsubscribe),
catchError(error => {
console.log('Publish error:', error);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

.pipe(
takeUntil(this.ngUnsubscribe),
catchError(error => {
console.log('Unpublish error:', error);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

const outboundObject = {
correspondences: [correspondence]
};
console.log(outboundObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

const outboundObject = {
annualReports: [report]
};
console.log(outboundObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

const outboundObject = {
damSafetyInspections: [damSafetyInspection]
};
console.log(outboundObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

@dhlevi
Copy link
Contributor Author

dhlevi commented Jul 30, 2020

Extra console logging removed

@dhlevi dhlevi merged commit 86de2e0 into bcgov:master Jul 30, 2020
@dhlevi dhlevi deleted the NRPT-214-api-ui branch August 12, 2020 15:25
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