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-6232] IRT/Application statuses #3317

Merged
merged 9 commits into from
Nov 29, 2024
Merged

Conversation

taraepp
Copy link
Collaborator

@taraepp taraepp commented Nov 25, 2024

Objective

  • add those status options
  • make them work the way they're supposed to
  • hunt down bugs that were making it so I couldn't test if it was working

Not done:

  • I actually didn't get to the status emails and notifications 🥹 (In a new ticket now)

MDS-6232

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

@@ -188,8 +188,6 @@ export const INFORMATION_REQUIREMENTS_TABLES = (projectGuid) =>
`/projects/${projectGuid}/information-requirements-table`;
export const INFORMATION_REQUIREMENTS_TABLE = (projectGuid, irtGuid) =>
`/projects/${projectGuid}/information-requirements-table/${irtGuid}`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Long story short:

  • separate endpoint for status has been nuked.
  • status now combined with document endpoint (and also action creator, etc)
  • caveat here is that additional parameters have to be added in form data (pretty obvious when you look at the action creator)

@@ -155,6 +155,15 @@ export enum PROJECT_STATUS_CODES {
CHR = "CHR",
}

export enum MAJOR_MINE_APPLICATION_AND_IRT_STATUS_CODE_CODES {
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 is a really terrible name, I know! And so long! I just couldn't think of anything else and MAJOR_MINE_APPLICATION_AND_IRT_STATUS_CODES was already taken. It needed making but I'd be more than happy to see it renamed. 😂

except BadRequest as err:
current_app.logger.error("Error occurred while retrieving file | document_guid")
current_app.logger.info(err)
raise BadRequest('Missing file information')
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 was just extracting the file update into its own function.

@@ -42,25 +52,22 @@ def get(self, project_guid, irt_guid):
@requires_any_of([MINE_ADMIN, MINESPACE_PROPONENT, EDIT_INFORMATION_REQUIREMENTS_TABLE])
@api.marshal_with(IRT_MODEL, code=200)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in hindsight, it really doesn't make much sense to keep the @api.marshal_with(IRT_MODEL) and also the params. 🤦‍♀️

@@ -142,8 +142,6 @@ def update(self,
self.save(commit=False)

if len(documents) > 0:
mine_document_guid = documents[0].mine_document_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.

this is/was a bug in project decision package.
It's a new document, so it doesn't have a mine_document_guid. Also, project is passed in as a parameter so it doesn't need to be queried below.

There was a spot to show project decision docs on the MMA and I wanted to see it populated, and this was a surprisingly quick fix. In the end, with discussion with Rebecca, it's hidden behind a feature flag (on the FE it was a document table with documents=[], so not at all useful)


const headerHeight = 121;
const tabNavHeight = 60;
const topOffset = headerHeight + tabNavHeight;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

at this point, should probably pull this out to the parent component, and/or find a better way. Buuuut it looks and works better now at least.

component: MajorMineApplicationTab,
helpKey: "Major-Mine-Application",
};

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 came up when I did a quick search for "final-app" and it turns out it's not referenced anywhere. So I deleted it.

};

const onRemoveFile = (err, fileItem) => {
remove(documents, { document_manager_guid: fileItem.serverId });
return dispatch(change(FORM.INFORMATION_REQUIREMENTS_TABLE, "final_irt", 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.

the manually dispatched changes aren't necessary any more- gave the input the name of "final_irt" so now it handles it on its own

);
})}
</Tabs>
items={tabItems}
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 a bit baffled by the buggy UI on this page and did some paring down in the process of figuring out what was wrong. (the flashy IRT page on the bugs channel! Locally, you'll get a ResizeObserver Loop error, possibly depending on the height of your browser but it's the natural height of my browser so it just always happened)

Turned out to be an antd/CSS issue. For the tabs, when there's overflow, antd wants to put a little ellipses button to show the overflow items. But when it's just on the threshold, it's like it doesn't know whether to show it or not, and so the result is constant layout changes. I didn't see a prop in the antd docs I could control this with, so instead I hid the button and made it scroll to show overflow in the scss file. Note that this is the only instance of "vertical-tabs" on MS, so the rule is quite specific to this area. My first idea was to make the menu like the other menus but the page structure was too different for that to be a simple change (updating the UI is going to be a major overhaul FYI)

@@ -138,6 +134,7 @@ export const InformationRequirementsTablePage = () => {
}, [uploadedSuccessfully]);

useEffect(() => {
handleFetchData();
return () => {
dispatch(clearInformationRequirementsTable({}));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fun fact- clearing the IRT means that if you click the back button from the IRT to return to the project overview, your IRT now (on test) shows "Not Started".
So, a couple changes here-

  • combine useEffects with same dep arrays
  • it doesn't need to fetch the data on load, because it's a child of the project page
  • project page looks at the project.irt instead of the irt in the state because I didn't like all the refreshing I had to do.
  • but I didn't totally refactor the data stuff here, it was just too much

@@ -217,8 +214,7 @@ export const InformationRequirementsTablePage = () => {

await handleFetchData();
setUploadedSuccessfully(true);
setEditMode(!isEditMode);
return cleanFilePondFile();
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'm not 100% sure what this function is supposed to do, but it just errored out every time. 🤷‍♀️

@@ -103,7 +114,7 @@ const StepForms = ({
state: { current: 2 },
});
}}
disabled={!uploadedSuccessfully && !project?.information_requirements_table?.irt_guid}
disabled={!uploadedSuccessfully}
>
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 second part of the condition made it so that I could click the button and it would take me in circles (if I didn't upload a document).

@matbusby-fw matbusby-fw force-pushed the mds-6232-irt-app-statuses branch from ef2f7dc to f239f40 Compare November 28, 2024 22:58
Copy link

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

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

See analysis details on SonarQube Cloud

Copy link

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

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

See analysis details on SonarQube Cloud

Copy link

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

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

See analysis details on SonarQube Cloud

@matbusby-fw matbusby-fw added the 👍 Ready for review Pull request has been double checked by the author and is ready for comments and feedback. label Nov 29, 2024
Copy link

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

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

See analysis details on SonarQube Cloud

@matbusby-fw matbusby-fw merged commit 2aac6e5 into develop Nov 29, 2024
16 of 20 checks passed
@matbusby-fw matbusby-fw deleted the mds-6232-irt-app-statuses branch November 29, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👍 Ready for review Pull request has been double checked by the author and is ready for comments and feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants