NRPT-117: Adds Permit and Amendment import with flavours #467

KitArmstrong · 2020-07-02T21:09:38Z

Ticket: https://bcmines.atlassian.net/browse/NRPT-117

This updates the Core importer to create Permits and PermitAmendments for all Mine records. Mines are only created as a BCMI flavour (previous change, not this ticket), but the Permits and PermitAmendments have both master and BCMI flavours. This is because it might be required to share this data in the future, especially the permit as it does exist for LNG.

If running the importer against the Core Prod API (recommended) there will be about 15 records that it errors on. This is expected and should be logged to the console.

NickPhura · 2020-07-03T17:01:45Z

api/src/models/master/permitAmendment.js

+
+    dateAdded: { type: Date, default: Date.now() },
+    dateUpdated: { type: Date, default: null },
+    datePublished: { type: Date, default: null },


Can remove the datePublished and publishedBy from the master record, as they should never themselves get published.

Removed from master.

NickPhura · 2020-07-03T17:03:28Z

api/src/models/master/permitAmendment.js

+
+    addedBy: { type: String, default: '' },
+    updatedBy: { type: String, default: '' },
+    publishedBy: { type: String, default: '' },


Could add sourceDateAdded and sourceDateUpdated to be consistent with the other types. Same goes for the bcmi flavour version of the models, though I'm not sure if we use them ever, so maybe there is an argument for leaving them out of the flavour models.

Also, probably need to add isBcmiPublished ? This would be in alignment with the other types, though I'm not sure how collections influences this, if it does.

Added isBcmiPublished to permit and permit amendments. I don't capture the source date information as none of that is returned by the Core API.

I don't capture the source date information as none of that is returned by the Core API.

Yeah, fair enough then

If you add it here, isBcmiPublished needs to be done for all other master records in order to facilitate search on the admin page.

NickPhura · 2020-07-03T17:04:09Z

api/src/models/master/permit.js

+    permitNumber: { type: String, default: '' },
+    status: { type: String, default: '' },
+    permitAmendments: [{ type: 'ObjectId', default: [], index: true }],
+


For line 48, though apparently I cant add a comment there, not sure why: Similar to my below comment, could add isBcmiPublished to align with the other models, though not sure how collections impacts this.

NickPhura · 2020-07-03T17:10:27Z

api/src/models/bcmi/permit-bcmi.js

+    read: [{ type: String, trim: true, default: 'sysadmin' }],
+    write: [{ type: String, trim: true, default: 'sysadmin' }],
+
+    mineGuid: { type: String, default: '' },


Open question: do we need to include all of the other Permit fields here as well? Our other records always copy all of the master fields, regardless of whether or not they get shown on the frontends. But maybe that isn't necessary? Not sure

I was talking to @marklise about this and opted to follow what the others have done. However, I would think they should just live on the flavour.

If I understand what you said correctly (possibly I don't), then we DO want to add all of the other permit fields here? recordName, dateIssued, legislation, etc.

Oh sorry, I thought this was on the master. Umm, to me it seems unnecessary on the flavour, but maybe there was a reason it was done before. Do you think they should be? Would they be used for anything?

Do you think they should be? Would they be used for anything?

Not sure. Trying to think if there is a good reason to include them, other than just because that is what was done in past.

The only thing that comes to mind, is if we wanted to use those other values for filtering/searching.
On LNG, we don't show that many fields, but some of them get used for searching/filtering. BCMI in theory might be similar, though with collections, its harder to say, since now we aren't directly querying for bcmi records from the public site.

So not sure :/

I guess given that we have no current reason to change it, we can just leave it as is, and always migrate it over in future if it becomes necessary for w/e reason

Okay, that sounds good.

NickPhura · 2020-07-03T17:16:23Z

api/src/integrations/core/permit-amendment-utils.js

+      receivedDate: (amendment.received_date && new Date(amendment.received_date)) || null,
+      issueDate: (amendment.issue_date && new Date(amendment.issue_date)) || null,
+      authorizedEndDate: (amendment.authorized_end_date && new Date(amendment.authorized_end_date)) || null,


Probably a nit: Assuming the dateString is in a valid form, we could probably just set it directly without calling new Date()?

NickPhura · 2020-07-03T17:38:42Z

api/src/integrations/core/datasource.js

+    // Update the amendments and permit.
+    const transformedAmendments = await permitAmendmentUtils.transformRecords(permit);
+    // To trigger flavour for this import.
+    const preparedAmendments = transformedAmendments.map(amendment => ({ ...amendment, PermitAmendmentBCMI: {} }))
+    const updatedAmendments = await permitAmendmentUtils.updateRecord(preparedAmendments);


Is it possible to be performing an update to an existing Permit, but have new amendments that need to get created?
If it is, then would probably need to do the findExisting check here first, and then call either permitAmendmentUtils update/create.

And then related, are we missing a put controller for permit-amendments?

Good call. Updated to handle an update and create.

NickPhura · 2020-07-03T17:43:33Z

api/src/controllers/put/mine-permit.js

@@ -0,0 +1,74 @@
+const mongoose = require('mongoose');


Do we still have the concept of a mine-permit, or is this replaced by permit and permit-amendment?

Do not. Removed.

KitArmstrong · 2020-07-03T20:27:56Z

@NickPhura Addressed your comments.

NickPhura · 2020-07-03T21:01:00Z

@NickPhura Addressed your comments.

Sweet, will give it another look over in a bit here

dhlevi

👍 That was a lot of work! Tests are passing, looks like enums are updated with the new flavour info, consistent naming with the *BCMI postfix. Looks good!

NickPhura · 2020-07-03T21:36:57Z

@KitArmstrong I tried running the core importer, but got a 503 error. It looks like I did successfully got an api token (says it expires in 16 hours), but then the subsequent calls to get verified mines, etc, returned 503.

I've got all of the env vars set up for Core Test. Maybe it is down? Or is there possibly some other config I am missing?

Ill mess with it a bit more though and see what I come up with

NickPhura · 2020-07-03T21:50:06Z

I know this is a known side effect of Core not supplying any of the 'normal' Permit values, but we'll probably want to put some kind of canned data into some of these fields, so it doesn't render empty like this (see screenshot).

Probably this is something that UX/PO folks need to think about, so maybe can be its own ticket. Assuming they feel the same way of course.

marklise

Looks good. We got a few tickets to make for other things that weren't caught during refinement.

NickPhura

Ran the importer a couple of times, appears to work as expected.
Tests all pass.

Nice one!
👍

Editing my Edit: never mind what I had previously. It is correct!

KitArmstrong added 2 commits July 2, 2020 14:01

NRPT-117: Adds Permit and Amendment import with flavours

e12b06a

Fix comment

76df8f9

KitArmstrong requested review from marklise, NickPhura and dhlevi July 2, 2020 21:09

Fixes unit tests

30df145

NickPhura reviewed Jul 3, 2020

View reviewed changes

KitArmstrong added 2 commits July 3, 2020 11:52

PR changes

3a25743

Remove unused import

92d2941

dhlevi approved these changes Jul 3, 2020

View reviewed changes

marklise approved these changes Jul 3, 2020

View reviewed changes

NickPhura approved these changes Jul 3, 2020

View reviewed changes

Merge branch 'master' into permitImport

55e9580

KitArmstrong merged commit 3ea4972 into bcgov:master Jul 3, 2020

KitArmstrong deleted the permitImport branch July 3, 2020 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NRPT-117: Adds Permit and Amendment import with flavours #467

NRPT-117: Adds Permit and Amendment import with flavours #467

KitArmstrong commented Jul 2, 2020

NickPhura Jul 3, 2020

KitArmstrong Jul 3, 2020

NickPhura Jul 3, 2020 •

edited

Loading

KitArmstrong Jul 3, 2020

NickPhura Jul 3, 2020

marklise Jul 3, 2020

NickPhura Jul 3, 2020 •

edited

Loading

KitArmstrong Jul 3, 2020

NickPhura Jul 3, 2020

KitArmstrong Jul 3, 2020

NickPhura Jul 3, 2020

KitArmstrong Jul 3, 2020

NickPhura Jul 3, 2020

KitArmstrong Jul 3, 2020

NickPhura Jul 3, 2020

KitArmstrong Jul 3, 2020

NickPhura Jul 3, 2020 •

edited

Loading

KitArmstrong Jul 3, 2020

NickPhura Jul 3, 2020 •

edited

Loading

KitArmstrong Jul 3, 2020

KitArmstrong commented Jul 3, 2020 •

edited by NickPhura

Loading

NickPhura commented Jul 3, 2020

dhlevi left a comment

NickPhura commented Jul 3, 2020

NickPhura commented Jul 3, 2020 •

edited

Loading

marklise left a comment

NickPhura left a comment •

edited

Loading

NRPT-117: Adds Permit and Amendment import with flavours #467

NRPT-117: Adds Permit and Amendment import with flavours #467

Conversation

KitArmstrong commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickPhura Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickPhura Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickPhura Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickPhura Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KitArmstrong commented Jul 3, 2020 • edited by NickPhura Loading

NickPhura commented Jul 3, 2020

dhlevi left a comment

Choose a reason for hiding this comment

NickPhura commented Jul 3, 2020

NickPhura commented Jul 3, 2020 • edited Loading

marklise left a comment

Choose a reason for hiding this comment

NickPhura left a comment • edited Loading

Choose a reason for hiding this comment

NickPhura Jul 3, 2020 •

edited

Loading

NickPhura Jul 3, 2020 •

edited

Loading

NickPhura Jul 3, 2020 •

edited

Loading

NickPhura Jul 3, 2020 •

edited

Loading

KitArmstrong commented Jul 3, 2020 •

edited by NickPhura

Loading

NickPhura commented Jul 3, 2020 •

edited

Loading

NickPhura left a comment •

edited

Loading