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-5927] Bring Projects Form to CORE #3145

Merged
merged 37 commits into from
Jun 12, 2024
Merged

Conversation

taraepp
Copy link
Collaborator

@taraepp taraepp commented Jun 7, 2024

Objective

Bring Projects form to CORE to make it match MS

Bonus:

  • streamlined a lot of props so components handled things themselves
  • the ProjectSummaryForm is the only instance using the common SteppedForm, so made it handle the form itself, check for errors, etc.
  • some bugs in authorizations documents (going away when you navigate to a different page, showing up in both new/amendment)

Tech Debt:

  • there's no "cancel confirmation" when you try to leave the page on CORE
  • CORE had the "edit/view mode" toggle, and I did keep that, but some of the components don't have a view mode yet. What's a checkbox supposed to look like, for example? The Add button still shows up on the Related Projects page
  • previously CORE users could remove and archive documents, if they had either admin role or edit project summaries. Did not bring this in yet
  • the status doesn't display on the header for every page yet, and if you want to update it you have to do that under "Ministry Contact"
    • I'm also not really sure if either of those fields should be required, so they're not
  • if we decide to fix the view mode, should probably also stop the tabs disabling while it's in View mode

MDS-5927

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

taraepp added 7 commits June 7, 2024 20:08
…TES, update references, remove restriction on CORE viewing/saving a draft, fix a style issue
…d up as props, make the forms have proper references. Use label props instead of hardcoding label text
…ct summary form on the CORE project summary page, make adjustments both there and on MS
const secondLine = abbrevLabel
? `<div>We accept most common ${fileTypeDisplayString} files</div>`
? `<div>We accept most common ${fileTypeDisplayString} files${fileSize}.</div>`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just for Denise- making file-size show up by default.

@@ -0,0 +1,12 @@
import React, { FC } from "react";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One of many, many components to get moved or copied to common

Copy link
Collaborator

Choose a reason for hiding this comment

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

More more more!

import React, { FC, ReactElement, useEffect, useState } from "react";
import { useSelector, useDispatch } from "react-redux";
import { getFormSyncErrors, getFormValues, submit } from "redux-form";
import { Button, Col, Menu, Popconfirm, Row, StepProps } from "antd";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically, SteppedForm in this state:

  • makes its own FormWrapper with the settings that are ideal for it
  • checks for errors
  • passes down isEditMode to FormWrapper to enable a view mode
  • transforms the values according to a function prop

@@ -540,14 +459,15 @@ const RenderAuthCodeFormSection = ({
);
};

export const AuthorizationsInvolved = (props) => {
export const AuthorizationsInvolved = () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

initialValues does not need to be passed as a prop when we're using reduxForm. Redux has all that in the state already (both initial & current values, and data about errors, etc) so we can use a selector to get that context right where we need it instead of prop drilling.
Most of the edits here are just taking out props that duplicate redux state.

required
validate={[requiredList]}
normalize={normalizeGroupCheckBox}
onDrop={(event) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This onDrop though was for a bug. I noticed that when I had "amendment" checked, and dropped a file, that it unchecked the box on me! Can be replicated in test, currently.

arrayPush(
FORM.ADD_EDIT_PROJECT_SUMMARY,
`authorizations.[${code}].AMENDMENT.[${index}].amendment_documents`,
`authorizations.[${code}].${type}[${index}].amendment_documents`,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this here was for the documents duplicating. The change from AMENDMENT to ${type} is to stop them duplicating across new/amendment, and the index now being a prop set by the parent is what stops them from being duplicated across multiple amendments.

};

const tableDocuments =
initialValues?.documents?.map(
documents?.map(
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 think that having initialValues here was also making bugs (not showing files that weren't in initialValues)

@@ -23,7 +31,7 @@ jest.mock("@mds/common/providers/featureFlags/useFeatureFlag", () => ({
isFeatureEnabled: () => true,
}),
}));

window.scrollTo = jest.fn();
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 was getting so many console errors from window.scrollTo and useLayoutEffect that I ran out of terminal lines and couldn't see the tests that were actually failing! Mocking made that better.

return cls.query.filter_by(
project_summary_guid=project_summary_guid, deleted_ind=False).one_or_none()
return cls.query.filter(ProjectSummary.status_code.is_distinct_from("DFT")).filter_by(
def find_by_project_summary_guid(cls, project_summary_guid):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no longer filtering out DRAFT on CORE.

@@ -195,21 +195,20 @@ export const MINE_PRE_APPLICATIONS = {
};

export const ADD_PROJECT_SUMMARY = {
route: "/mines/:mineGuid/project-description/new",
dynamicRoute: (mineGuid) => `/mines/${mineGuid}/project-description/new`,
route: "/mines/:mineGuid/project-description/new/:tab",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed both these routes to match the name on MS so I could use GLOBAL_ROUTES

@@ -8,7 +8,8 @@ import {
REGIONS,
} from "@mds/common/tests/mocks/dataMocks";
import { PROJECTS, STATIC_CONTENT } from "@mds/common/constants/reducerTypes";
import { ReduxWrapper } from "@/tests/utils/ReduxWrapper";
import { ReduxWrapper as CommonWrapper } from "@mds/common/tests/utils/ReduxWrapper";
import { ReduxWrapper as MSWrapper } from "@/tests/utils/ReduxWrapper";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🧀

newDocuments.forEach((newDoc) => {
updateAmendmentDocuments(newDoc, code);
});
updateAmendmentDocuments(newDocuments);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the changes to updateAmendmentDocuments is a fix for the bug Alex posted in the bugs channel today. Before it was updating a single document, not really having the power of removing documents. Now it takes as its parameter the desired value of the field, allowing us to remove documents here.

...spatial_documents,
...support_documents,
])
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Making sure that documents persist between user navigation events and are added to the table when uploaded. spatial & support variables are populated right in formatProjectSummary

const canModify = is_latest_version && !this.is_archived;
if (!this.mine_document_guid) return [];
const canModify = is_latest_version && !this.is_archived && this.mine_document_guid;
const canView = this.file_type === ".pdf" && this.document_manager_guid;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For cypress tests. So that the actions column can be there before we save the file (download/view actions require document_manager_guid, not mine_document_guid, but I kept the other actions from being allowed)

<FormWrapper name={formName} onSubmit={() => {}}>
<LegalLandOwnerInformation />
</FormWrapper>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if the ReduxWrapper can be used here for consistency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the .getActions used in the test, can't use ReduxWrapper. This one was only updated to make it stop console logging errors about missing form context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is correct. getActions() is a method of the store object which is encapsulated in ReduxWrapper.

Copy link

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

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Copy link

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

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link

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

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Copy link

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

Failed conditions
E Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint


const RenderEMAPermitCommonSections = ({ props }) => {
const { code } = props;
const RenderEMAPermitCommonSections = ({ code, isAmendment, index }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love this change. Much clearer

"Successfully updated project.",
false
)
);
})
.then(() => {
.then(async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this async here?


const handleUpdate = (message) => {
const { project_guid, mrc_review_required, contacts, project_lead_party_guid } = formValues;
const handleUpdateProjectSummary = async (payload, message) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The combo of .then and async await here feels a bit clunky. Is it possible to make it all async/await?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's probably a good thing to do. I don't want to expand the scope too much though, and want to unblock AMS work so I'll leave it as something to keep in mind for the future.

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.

Looking great. Left one comment on maybe making one of the functions fully async/await rather than a mix of .then and async.

Some of the refactors are a bit tricky to track in a PR, but it all looks really solid!

@taraepp taraepp merged commit c8431da into develop Jun 12, 2024
20 of 24 checks passed
@taraepp taraepp deleted the mds-5927-core-projects branch June 12, 2024 18:31
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