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

PIMS 1604/1597/1620/1601 - Bug Fixes #2344

Merged
merged 13 commits into from
Apr 24, 2024
Merged

Conversation

GrahamS-Quartech
Copy link
Contributor

🎯 Summary

PIMS-1604
PIMS-1597
PIMS-1620
PIMS-1601

Various bug fixes enhancements that were all kinda in similar places:

  • PID will now be properly zero padded in the table and detail view and when editing an existing entry. This is also reflected in table search, so searching with leading zeroes is now valid.
  • Validation on building area fields is swapped around.
  • Searching on classification types in the property table works correctly now.
  • Saving a null tenancy date field is now considered valid.

🔰 Checklist

  • I have read and agree with the following checklist and am following the guidelines in our Code of Conduct document.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation where required.
  • I have tested my changes to the best of my ability.
  • My changes generate no new warnings.

…or edit dialog. Net usable area validation fixed. Fixed searching on classification type. Fixed saving null tenancy date.
Copy link

🚀 Deployment Information

The Express API Image has been built with the tag: 2344. Please make sure to utilize this specific tag when promoting these changes to the TEST and PROD environments during the API deployment. For more updates please monitor Image Tags Page on Wiki.

Copy link

codeclimate bot commented Apr 23, 2024

Code Climate has analyzed commit 50fb3f4 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 93.7%.

View more on Code Climate.

Copy link

🚀 Deployment Information

The React APP Image has been built with the tag: 2344. Please make sure to utilize this specific tag when promoting these changes to the TEST and PROD environments during the APP deployment. For more updates please monitor Image Tags Page on Wiki.

Copy link
Collaborator

@LawrenceLau2020 LawrenceLau2020 left a comment

Choose a reason for hiding this comment

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

The validation seems to be missing on this, it's letting me add net usable area greater than the total area.

image

@GrahamS-Quartech
Copy link
Contributor Author

GrahamS-Quartech commented Apr 23, 2024

The validation seems to be missing on this, it's letting me add net usable area greater than the total area.

This is intentional. Aiden / the users he UAT'd with seemed to think we had it backwards 🤷

@GrahamS-Quartech
Copy link
Contributor Author

GrahamS-Quartech commented Apr 24, 2024

The validation seems to be missing on this, it's letting me add net usable area greater than the total area.

As per stand up discussion I've reverted this change, but I'm pretty confused as to why UAT suggested the opposite.

@dbarkowsky
Copy link
Collaborator

Some issue with PID saving if the 0s aren't there.
Also, should we separate the groups of 3 with hyphens? 000000000 vs 000-000-000
image

Comment on lines 194 to 196
(val.length <= 9 &&
(val.length > 0 || formVals['PIN'].length > 0 || propertyType === 'Building')) ||
'Must have set either PID or PIN',
'Must have set either PID or PIN not exceeding 9 digits.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's an issue here because val is a number, so .length seems to be undefined.
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tried doing this validation after converting val to a string. It will go through like that, but then the value saved in the database is NULL, so something is getting lost along the way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've corrected this to account for the new behavior where TextFormField will parseFloat onBlur. Something to note for the future though: doing that parseFloat means that entering enough digits will now lock up the text field since the parsed float will be in scientific notation.

Copy link
Collaborator

@dbarkowsky dbarkowsky left a comment

Choose a reason for hiding this comment

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

Last change seems to have fixed the issue for me.
Will note that the PID/PIN fields allow for decimals. Wondering if we shouldn't just make additional field types instead of trying to conform everything to the text field.

@GrahamS-Quartech GrahamS-Quartech merged commit 5a2ab0b into main Apr 24, 2024
9 checks passed
@GrahamS-Quartech GrahamS-Quartech deleted the PIMS-1604-1597-1620-1601 branch April 24, 2024 23:31
@GrahamS-Quartech
Copy link
Contributor Author

Wondering if we shouldn't just make additional field types instead of trying to conform everything to the text field.

I think we're at the point where that would make sense 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants