Skip to content

Conversation

@severinbeauvais
Copy link
Collaborator

@severinbeauvais severinbeauvais commented Dec 3, 2025

Issue: bcgov/entity#31526 + bcgov/entity#31543

Description of changes:

  • added subtitle for pending filings
  • added body for pending filings
  • misc cleanup
  • added header actions for Court Order when not staff
  • added header actions for pending filings
  • added component for pending subtitle
  • added general filing strings for pending filings
  • fixed string key typo for dissolution sub types
  • ensured todos and filings lists are clear before dashboard is mounted
  • now push instead of assign new filings into filings list
  • added method to insert a filing into filings list
  • if task is a pending-payment-completed maintenance filing then insert it into filings list instead of todos list
  • added todo utils to check task type and convert task to filing
  • fixed businessCached check
  • changed "screen" -> "page" (per Jacqueline)
  • fixed const in composable
  • fixed e2e tests

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namex license (Apache 2.0).

- added body for pending filings
- misc cleanup
- added header actions for Court Order when not staff
- added header actions for pending filings
- added component for pending subtitle
- added general filing strings for pending filings
- fixed string key typo for dissolution sub types
- ensure todos and filings lists are clear before dashboard is mounted
- now push instead of assign new filings into filings list
- add method to insert a filing into filings list
- if task is a pending-payment-completed maintenance filing then insert it into filings list instead of todos list
- add todo utils to check task type and convert task to filing
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request implements functionality to display pending maintenance filings with "Payment Completed" status in the Filing History section instead of the To Do section. The changes enhance the user experience by properly categorizing filings and providing appropriate UI feedback for pending filings.

  • Modified the todo store to identify and redirect pending maintenance filings to the filings list
  • Added utility functions to distinguish maintenance filings from bootstrap tasks and check payment completion status
  • Enhanced UI components to display pending filings with appropriate subtitles, body content, and header actions

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/utils/todo/task-filing/index.ts Added blank line for code formatting
src/utils/todo/index.ts Added utility functions to identify maintenance filings, check payment status, and convert tasks to filing items
src/stores/todo.ts Modified task loading logic to redirect pending maintenance filings to filings list instead of todos
src/stores/filings.ts Changed loadFilings to append filings using push instead of assignment; added insertFiling method to add filings at the beginning of the list
src/pages/dashboard.vue Added clear calls for todos and filings in onBeforeMount to ensure fresh data; updated comments for clarity
src/lang/en.json Added new i18n strings for pending filing messages; fixed typo in "dissolutionSubTypes" key
src/interfaces/filing-i.ts Reordered interface properties for consistency (displayLedger before displayName)
src/components/bcros/filing/common/filedAnd/Pending.vue Created new component to display pending filing subtitle with status indicators
src/components/bcros/filing/common/HeaderActions.vue Added conditional logic to show View/Hide buttons for Court Orders (non-staff) and pending filings
src/components/bcros/filing/CommonTemplate.vue Added conditional rendering for pending filing body content with appropriate messaging

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if (!businessCached || force) {
filings.value = await getFilings(identifier)
const newFilings = await getFilings(identifier)
filings.value.push(...newFilings)
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The _filingsForIdentifier variable is never updated after loading filings. This means the cache check on line 78 will always fail after the first load, causing loadFilings to repeatedly append new filings instead of replacing them.

After line 81, you should add: _filingsForIdentifier.value = identifier

Suggested change
filings.value.push(...newFilings)
filings.value.push(...newFilings)
_filingsForIdentifier.value = identifier

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Dec 3, 2025

Choose a reason for hiding this comment

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

CP is correct. This looks like unfinished code so I simply cleaned it up.

I also updated the check for "no filings" because filings.value is always True, even when it's first initialized as [] (on line 12).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Re: line 77

This method (loadFilings) is only called for actual businesses (not bootstrap filings), so the history list should always have 1 or more filings. And if, for some reason, it doesn't, then the expression on line 78 will be true and it won't hurt to try to get the new filings again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Re: line 80

Whether loadFilings is called with an empty list or one with a pending task, the new filings will be added to the end (bottom) of the list, keeping a potential pending task at the top.

// if this is a "Pending | Payment Completed" maintenance filing task
// then insert it into the filings list instead of the todos list
if (isPendingPaymentCompletedTask(task) && isMaintenanceFilingTask(task)) {
useBcrosFilings().insertFiling(taskAsFiling(task))
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Potential null pointer exception: taskAsFiling(task) can return null (see line 84 in the function), but insertFiling expects a non-null ApiResponseFilingI parameter. This could cause a runtime error when trying to insert a null filing.

Consider adding a null check:

const filing = taskAsFiling(task)
if (filing) {
  useBcrosFilings().insertFiling(filing)
}
Suggested change
useBcrosFilings().insertFiling(taskAsFiling(task))
const filing = taskAsFiling(task)
if (filing) {
useBcrosFilings().insertFiling(filing)
}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Dec 3, 2025

Choose a reason for hiding this comment

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

I'm not concerned about this because:

  • task is an iterable, so won't be null
  • the checks on line 91 will be false(y) if this isn't the right kind of task, so line 92 won't be executed


const insertFiling = (filing: ApiResponseFilingI) => {
filings.value.unshift(filing)
}
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Dec 3, 2025

Choose a reason for hiding this comment

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

Note: whether insertFiling is called before the filings are fetched (list is empty) or after the filings are fetched, it will insert the filing at the start (top) of the list, keeping a potential pending task at the top.

"isCorrectionPending": false,
"name": "correction",
"priority": false,
"paymentStatusCode": "COMPLETED",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A "pending - payment completed" task now displays in the History list, so I simply changed this to "pending - payment incomplete".

Ditto below.

{{ $t('text.filing.general.paidButNotCompletedByRegistry').replace('FILING', title) }}
</p>

<p> {{ $t('text.filing.general.paidButNotCompletedByRegistry', { title }) }} </p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that I used the built-in replacement feature to replace {title} in the string with the title property.

<strong v-if="!isExpanded">{{ $t('button.filing.actions.view') }}</strong>
<strong v-else>{{ $t('button.filing.actions.hide') }}</strong>
</template>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Below, I thought it was clearer to have separate conditions, even though they all display View/Hide, instead of trying to combine the conditions. Thoughts?

defineProps({
filing: { type: Object as PropType<ApiResponseFilingI>, required: true }
})
</script>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like this:

image

"filingPending": "Filing Pending",
"paidButNotCompletedByRegistry": "This FILING is paid, but the filing has not been completed by the Business Registry yet. Some filings may take longer than expected.",
"paidButStillBeingProcessed": "This {title} is paid, but is still being processed. Refresh this page or come back later to check on the progress.",
"ifThisIssueContinues": "If this issue continues, please contact us.",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like this:

image

"correction": "Correction",
"courtOrder": "Court Order",
"dissolution": "Dissolution",
"dissolutionSybTypes": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Typo!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before (note "This Dissolution is..."):
image

After (note "This Voluntary Dissolution is..."):
image

Ref: https://dev.business-dashboard.bcregistry.gov.bc.ca/BC0888391?accountid=3040

onBeforeMount(async () => {
useBcrosTodos().clearTodos()
useBcrosFilings().clearFilings()
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Dec 3, 2025

Choose a reason for hiding this comment

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

This is probably not needed since the the lists are already initialized to [] but I'm adding this here to be explicit and in case this page/component is ever re-mounted.

@severinbeauvais
Copy link
Collaborator Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

bcregistry-sre commented Dec 3, 2025

Temporary Url for review: https://business-dashboard-dev--pr-240-t8m3ord0.web.app

SB says, you can look at the screenshots in the tickets or you can try these:

@severinbeauvais severinbeauvais changed the title 31526 + 31543 Display pending maintenance filings in History instead of To Do 31526 + 31543: Display pending maintenance filings in History instead of To Do + Update Court Order buttons Dec 4, 2025
@severinbeauvais severinbeauvais changed the title 31526 + 31543: Display pending maintenance filings in History instead of To Do + Update Court Order buttons 31526 + 31543: Display pending maintenance filings in History instead of To Do + Update Court Order action buttons Dec 4, 2025
Copy link
Collaborator

@loneil loneil left a comment

Choose a reason for hiding this comment

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

Code changes make sense, poked at deployed links a bit and look good I think

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.

5 participants