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

Allow a name to be removed from nro name_instance table #512

Merged
merged 4 commits into from
Feb 7, 2019

Conversation

perkinss
Copy link
Contributor

Issue #, if available:
bcgov/entity#54

Description of changes:
Check whether new name is blank before adding a new row to the name_instance table in nro.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namex license (Apache 2.0).

@katiemcgoff
Copy link
Contributor

I believe there is an api change needed too - current code just blanks out name but doesn't remove the name record. The record needs to be removed from Namex db, otherwise it's considered a name to be actioned on - ie: if name 3 is "removed" (just blanked out), user can reject 1, reject 2, and now we're on name 3 trying to make a decision on a blank name. This is a bug.

@perkinss
Copy link
Contributor Author

perkinss commented Feb 1, 2019

Oh! thanks for catching that
I'll look into it

@perkinss perkinss force-pushed the 54-Allow-removal-of-name branch from 5c670f3 to 47225e4 Compare February 4, 2019 21:48
@katiemcgoff katiemcgoff self-requested a review February 5, 2019 18:36
Copy link
Contributor

@katiemcgoff katiemcgoff left a comment

Choose a reason for hiding this comment

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

Looks like this doesn't quite work - because the name record is deleted from the postgres db, it doesn't exist in the "nrd" object that is passed to the change_nr() function. So it doesn't get removed from NRO.

@katiemcgoff
Copy link
Contributor

katiemcgoff commented Feb 7, 2019

There is one remaining issue, in postgres now, due to the changed behaviour. Closing this PR.

KBM note - nevermind, it's all fine. Approving.

@katiemcgoff katiemcgoff closed this Feb 7, 2019
@katiemcgoff katiemcgoff reopened this Feb 7, 2019
@katiemcgoff katiemcgoff merged commit ab28550 into bcgov:master Feb 7, 2019
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.

2 participants