-
Notifications
You must be signed in to change notification settings - Fork 2
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
2422/eligib v2 improve #2426
2422/eligib v2 improve #2426
Conversation
Visit the preview URL for this PR (updated for commit 8e57fe0): https://jac-admin-develop--pr2426-2422-eligib-v2-impro-cdxiwph3.web.app (expires Thu, 01 Aug 2024 08:44:37 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 4e92cf51659207b0ae3509dc5c40edde50edfec0 |
@@ -262,6 +288,9 @@ export default { | |||
showNotMet: function () { | |||
this.$refs['issuesTable'].reload(); | |||
}, | |||
filterStatus: function () { | |||
this.$refs['issuesTable'].reload(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we should be doing this inside a computed property.
We should do it inside a method, lifecycle hook or (maybe best in this case) a watcher.
According to ChatGPT: "Computed properties in Vue.js are meant to be pure functions that derive their values from the component's reactive state. They should not have side effects such as making method calls that emit events or cause changes to the state."
@@ -251,6 +276,7 @@ export default { | |||
showNotMet: false, | |||
statutoryTypes: ['pq', 'pqe'], | |||
nonStatutoryTypes: ['pje', 'rls'], | |||
applicationStatusOptions: APPLICATION_STATUS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we need to add all statuses to the dropdown or just available statutes within the exercise using the helper function availableStatuses
. Could you double-check it, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomlovesgithub This meets the request laid out on the ticket, however, I've recently been speaking with Ryan about the character report, which is being used quite early on, before the point at which candidates have any status. I wonder if it might be prudent to include the blank status option in this list in case a similar need arises for users running the eligibility report?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomlovesgithub I can see the dropdown now, but when I download the annex, it includes all candidates rather than those filtered by the dropdown. The associated ticket referred to has apparently been released, hence can you look into this further pls?
What's included?
Who should test?
✅ Product owner
✅ Developers
✅ UTG
How to test?
[Here]
Observe the new filter - ensure that it correctly displays only candidates with the selected status.
Candidates status shows under their name - if they have no status, no status should appear.
note downloads will only work as set out in the ticket when this digital platform ticket is deployed or merged and released.
Risk - how likely is this to impact other areas?
🟢 No risk - this is a self-contained piece of work
🟠 Medium risk - this does change code that is shared with other areas
🔴 High risk - this includes a lot of changes to shared code
Additional context
Screen.Recording.2024-06-21.at.17.07.38.mov
Related permissions
Have permissions been considered for this functionality?
PREVIEW:DEVELOP
can be OFF, DEVELOP or STAGING