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

25190 - Court Order Filings #714

Merged
merged 18 commits into from
Feb 7, 2025

Conversation

Rajandeep98
Copy link
Collaborator

@Rajandeep98 Rajandeep98 commented Jan 29, 2025

Issue #: bcgov/entity#25190

Description of changes:
Court Order Filings Added

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

@severinbeauvais

This comment was marked as outdated.

@Rajandeep98
Copy link
Collaborator Author

@severinbeauvais

This comment was marked as outdated.

@bcregistry-sre

This comment was marked as outdated.

@Rajandeep98

This comment was marked as outdated.

@bcregistry-sre

This comment was marked as outdated.

package.json Outdated Show resolved Hide resolved
@Rajandeep98

This comment was marked as outdated.

@bcregistry-sre

This comment was marked as outdated.

@Rajandeep98

This comment was marked as outdated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For some reason, your package process added several packages that we don't need.

In this file, I expect to see only the updated package version number and the updated enums package.

Please resolve this. We do not need and should not have those additional esbuild dependencies.

Eg,
image
image
image
image
Etc.

openFileDialog () {
// Access the underlying file input element and trigger click
const fileInput = this.$refs.fileUploadRef.$el.querySelector('input')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, so you're not using the new file uploader component?

How about moving this code to a public function in the file upload component, that you just call here?

Reason: this component should not know what's inside the file upload component.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You didn't fix this here so it will have to be updated later.

readonly PageSizes = PageSizes
// enum for template
readonly FilingCodes = FilingCodes
/** Prop to display the dialog. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are any of these props needed? If not, please replace remove them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You didn't fix this here so it will have to be updated later. (Ideally, when you write the unit tests.)

@severinbeauvais
Copy link
Collaborator

I see no unit tests in this PR. Please create another ticket to create a unit test suite for this filing.

@severinbeauvais
Copy link
Collaborator

/gcbrun

@bcregistry-sre
Copy link
Collaborator

bcregistry-sre commented Feb 6, 2025

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.

It looks great! Thank you.

I approve, but please update the app version again, and then I'm OK with merging this (with the understanding that unit tests will come in the next week or so).

Copy link

sonarqubecloud bot commented Feb 7, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
8.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link
Collaborator

@ketaki-deodhar ketaki-deodhar 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! Please create a ticket for unit tests and refer it in the original ticket

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.

In the package lock file, I see those new architecture packages. Are you using only "npm" in this repo to manage dependencies? Anyways, let's merge this and see if it's OK as-is.

PS Did you create a new ticket to write unit tests for this new filing?

Approving.

@severinbeauvais severinbeauvais merged commit 840026f into bcgov:main Feb 7, 2025
9 of 12 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.

4 participants