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

18532 - AM Button Implementation #748

Conversation

jamespaologarcia
Copy link
Contributor

Issue #: /bcgov/entity#18532
Added implementation for amalgamate now button.

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

@jamespaologarcia
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-748-w80ga369.web.app

@severinbeauvais severinbeauvais changed the title For PR - AM Button Implementation 18532 - AM Button Implementation Nov 23, 2023
Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

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

I think you need to update something in App.vue as well, somewhere around here: fixed now

const nr = JSON.parse(sessionStorage.getItem('NR_DATA'))

This is in case the user was initially not logged in and was redirected to log in, to resume the flow when they return to Namerequest UI.

Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

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

You also need to update existing-request-display.vue to handle your second "todo checkbox" in your ticket. fixed now

It should look like this, except "Amalgamate Now":

image

This ties in to my comment about updating getBusinessRequest().

If this doesn't fit in your current ticket's estimate, let me know and we'll create a new ticket for it.

@jamespaologarcia
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-748-w80ga369.web.app

src/App.vue Outdated Show resolved Hide resolved
@jamespaologarcia
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-748-w80ga369.web.app

@jamespaologarcia
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-748-w80ga369.web.app

@JazzarKarim
Copy link
Collaborator

James, I'm pressing on the amalgamate now button after an NR has been approved, but I don't see anything happening. Shouldn't we be getting an HTTP 400 error for now at least since the BE hasn't been setup yet?
amalgamate now button doing nothing

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-748-w80ga369.web.app

@jamespaologarcia
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-748-w80ga369.web.app

@jamespaologarcia
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-748-w80ga369.web.app

@severinbeauvais
Copy link
Collaborator

James, please indicate here (or better, in the ticket), what testing you've done, including:

  • entities supported by FF (create draft)
  • entities not supported by FF (go to COLIN)
  • while not logged in
  • while already logged in
  • numbered amalgamations (button on search page)
  • named amalgamations (button on existing request display page)

Thanks.

@jamespaologarcia
Copy link
Contributor Author

/gcbrun

header: { accountId, name },
incorporationApplication: {
nameRequest: { legalType, nrNumber }
if (nr.request_action_cd === 'AML') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks correct now - thanks.

If you're ever in here again -- perish the thought! -- use NrRequestActionCodes.AMALGAMATE instead of "AML". (This isn't worth a new commit on its own.)

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-748-w80ga369.web.app

@severinbeauvais
Copy link
Collaborator

James, are you ready to merge this, or do you want to do a bit more testing first?

@severinbeauvais
Copy link
Collaborator

James, are you ready to merge this, or do you want to do a bit more testing first?

@jamespaologarcia , I wrote this on Nov 29. What's your answer please?

@jamespaologarcia
Copy link
Contributor Author

James, are you ready to merge this, or do you want to do a bit more testing first?

@jamespaologarcia , I wrote this on Nov 29. What's your answer please?

Hi,

I sent the PR link to Omid. I'm waiting for confirmation before I merge it.

Copy link
Collaborator

@eve-git eve-git left a comment

Choose a reason for hiding this comment

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

LGTM

@severinbeauvais
Copy link
Collaborator

James, are you ready to merge this, or do you want to do a bit more testing first?

@jamespaologarcia , I wrote this on Nov 29. What's your answer please?

James replied: "All good from my side. I tested the cases you pointed out to me."

@severinbeauvais severinbeauvais merged commit b08fc6c into bcgov:main Dec 5, 2023
5 checks passed
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.

5 participants