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

[MDS-6014] Spatial File FE #3181

Merged
merged 18 commits into from
Jul 12, 2024
Merged

[MDS-6014] Spatial File FE #3181

merged 18 commits into from
Jul 12, 2024

Conversation

taraepp
Copy link
Collaborator

@taraepp taraepp commented Jul 10, 2024

Objective

  • show spatial files in a table
  • upload spatial files in a modal
    • validate on FE
  • show the geomark on the map in a view modal

Misc. changes:

  • change to map component to show geojson features
    • there were some issues with the timing of the call to fitBounds and with no minimum height set in the component, issues rendering without a container (see in action: look at the legal land owner page on the form on test, and delete the other elements in the same row. With no sibling elements, it will disappear)
  • modal wrapper/add party component form: upgraded to tsx and props tidied up (there were unused props)
    • extracted the main modal logic to a common component, used to wrap around separate CORE/MS implementations
  • RenderFileUpload: dealt with an extension having the potential for multiple MIME-types. Cleared up variable ambiguity (lots of eslint warnings about reusing names). Destructured props, deleted unused, and attached the prop type to the component. Fixed bug where uppercase extensions weren't interpreted as valid. Disallowed empty files (everywhere that the common component is used). But a major takeaway here is that RenderFileUpload will now yell at you if you give it bad props
  • I initially considered using the DocumentTable. That didn't go very well, but I did take out the bit where DocumentTable took DocumentTable as a prop.

MDS-6014

Why are you making this change? Provide a short explanation and/or screenshots

@@ -17,4 +17,6 @@ EXPOSE 5001

RUN mkdir -p /tmp/celery

RUN mkdir -p /tmp/spatial
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that this doesn't match exactly the folder the assets are downloaded to. We should probably add the missing / to the docman paths

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was working with this in my PR too, but from what I can tell the folder doesn't get persisted to the resulting container after this command (/tmp/celery also doesn't exist if you exec into the container after creation)

Copy link
Collaborator

Choose a reason for hiding this comment

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

In my PR I added the creation in the init file for docman.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just took this stuff out from both Dockerfiles. 👍

simensma-fresh
simensma-fresh previously approved these changes Jul 12, 2024
Copy link
Collaborator

@simensma-fresh simensma-fresh left a comment

Choose a reason for hiding this comment

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

Loving all the cleanup! 🧹

Two small comments, but looks great!
(and a merge conflict)

<FormWrapper
isModal
name={modalFormName}
onSubmit={async (values) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a huge deal, but I this function is getting a bit large to live inline. Might be nice as it's own function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. Extracted a function!

addFileStart?: () => void;
}

const defaultProps = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙌🏻

);

export default withFeatureFlag(connect(null, mapDispatchToProps)(FileUpload));
export default FileUpload;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice solution on this file as a whole! Happy to see some of the clean up too!

matbusby-fw
matbusby-fw previously approved these changes Jul 12, 2024
Copy link
Collaborator

@matbusby-fw matbusby-fw left a comment

Choose a reason for hiding this comment

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

Looks great. A couple thoughts/suggestions, but it's looking nice and clean!

@taraepp taraepp dismissed stale reviews from matbusby-fw and simensma-fresh via 41831a8 July 12, 2024 20:08
Copy link

Quality Gate Failed Quality Gate failed for 'bcgov-sonarcloud_mds_common'

Failed conditions
55.0% Coverage on New Code (required ≥ 80%)
5.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link

Quality Gate Passed Quality Gate passed for 'bcgov-sonarcloud_mds_minespace-web'

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
85.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

Quality Gate Failed Quality Gate failed for 'bcgov-sonarcloud_mds_core-web'

Failed conditions
77.6% Coverage on New Code (required ≥ 80%)
33.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@@ -13,18 +11,23 @@ import DocumentCompressionWarningModal from "./DocumentCompressionWarningModal";
import DocumentCompressedDownloadModal from "./DocumentCompressedDownloadModal";

interface DocumentCompressionProps {
documentType: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for cleaning up this file.

@taraepp taraepp merged commit be32ca1 into develop Jul 12, 2024
13 of 15 checks passed
@taraepp taraepp deleted the mds-6014-spatial-file-fe branch July 12, 2024 22:54
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