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

Feat/1382 changes to inactive workplaces report updated #6090

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

ssrome
Copy link
Collaborator

@ssrome ssrome commented Feb 14, 2024

Work done

  • Ticket worked on
  • Add parent workplace id and cqc column to inactive workplaces report
  • Add migrations for additional columns in establishmentLastActivity view
  • Added migration command to new-start (runs migrations on completion of deploy pipeline)
  • Added condition to prevent multiple refreshes of establishmentLastActivityView

Tests

Does this PR include tests for the changes introduced?

  • Yes
  • No, I found it difficult to test
  • No, they are not required for this change

Sorry, something went wrong.

@ssrome ssrome marked this pull request as ready for review February 14, 2024 14:17
@ssrome ssrome marked this pull request as draft February 18, 2025 16:08
Comment on lines 13 to 17
const stopViewRefresh = _req.query?.stopViewRefresh;
try {
if (stopViewRefresh !== 'true' || !stopViewRefresh) {
await inactiveWorkplacesUtils.refreshEstablishmentLastActivityView();
}
Copy link
Collaborator

@kapppa-joe kapppa-joe Feb 20, 2025

Choose a reason for hiding this comment

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

Seems that we are doing this check in multiple places. Would it be good if we extract them to a function, so that the logic are kept in one place?

for example:

const refreshViewIfNecessary = async (req) => {
  const stopViewRefresh = req.query?.stopViewRefresh;
  if (stopViewRefresh !== 'true' || !stopViewRefresh) {
    await inactiveWorkplacesUtils.refreshEstablishmentLastActivityView();
  }
};

and then, in the try block we can replace the if branch in line 15 ~ 17, 36 ~ 38 and 51 ~ 53 by calling await refreshViewIfNecessary(_req)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have now put the duplicated code in a util

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.

None yet

4 participants