🐛 Refresh affect trackers after filing trackers #472

superbuggy · 2024-11-01T16:21:32Z

OSIDB-3483 Fix affect tracker refreshing after filing trackers

Checklist:

Commits consolidated
Changelog updated
Test cases added/updated
Jira ticket updated

Summary:

[Replace with a brief summary of the changes introduced by this PR.]

Changes:

[Replace with a detailed description of the changes made in this PR, including any new features, enhancements, bug fixes, or refactorings.]

Considerations:

[Replace with any additional considerations, notes, or instructions for reviewers.]

Closes OSIDB-3483

src/composables/useFlawModel.ts

MrMarble

I don't think we should enforce the as Ref<ZodFlawType>; everywhere.
The only place where initializeFlaw is called is on fetchFlaw, maybe instead of using the global flaw, use a local variable and only if the flaw is fetched correctly, update the global one, that way we can remove the optional null on useFlaw and leave it like ref<ZodFlawType>(blankFlaw()); so we can remove all the type casting.

src/components/FlawAffects/FlawAffects.vue

src/composables/useFlaw.ts

src/composables/useFetchFlaw.ts

C-Valen

I'm getting a type error when trying to file a bunch of trackers:

MrMarble · 2025-01-21T11:46:55Z

src/composables/__tests__/useFlawAffectsModel.spec.ts

+async function useMockedModel(flaw: ZodFlawType) {
+  const _useFlaw = await vi.importActual<typeof import('@/composables/useFlaw')>('@/composables/useFlaw');
+  vi.mocked(useFlaw).mockReturnValue({ ..._useFlaw.useFlaw(), flaw: ref(flaw) });
+  const { flaw: flawRef } = useFlaw();


Since this component only uses the flaw and not the other properties, I think we could skip importing the original implementation, just adding the partial option to vi.mocked will get rid of the type error.

Suggested change

async function useMockedModel(flaw: ZodFlawType) {

const _useFlaw = await vi.importActual<typeof import('@/composables/useFlaw')>('@/composables/useFlaw');

vi.mocked(useFlaw).mockReturnValue({ ..._useFlaw.useFlaw(), flaw: ref(flaw) });

const { flaw: flawRef } = useFlaw();

function useMockedModel(flaw: ZodFlawType) {

vi.mocked(useFlaw, { partial: true }).mockReturnValue({ flaw: ref(flaw) });

const { flaw: flawRef } = useFlaw();

I've tried this locally and all the tests pass, this way we can remove the async functions

MrMarble · 2025-01-21T11:47:17Z

src/composables/__tests__/useFlawAffectsModel.spec.ts

@@ -14,6 +14,7 @@ import type { ZodFlawType } from '@/types';
 import { postAffects, deleteAffects } from '@/services/AffectService';
 import { getFlaw } from '@/services/FlawService';

+// const { blankFlaw } = useFlaw();


could you remove this?

MrMarble · 2025-01-21T11:47:28Z

src/composables/__tests__/useFlawAffectsModel.spec.ts

+// vi.mock('@/composables/useFlawAffectsModel', async importOriginal => ({
+//   ...await importOriginal(),
+//   refreshAffects: vi.fn(),
+// }));


also this can be removed

MrMarble · 2025-01-21T11:54:33Z

src/composables/useRelatedFlawTrackers.ts

+  watch(relatedFlaws, () => {
+    selectedRelatedFlaws.value.forEach((selectedFlaw) => {
+      const index = relatedFlaws.value.findIndex(({ uuid }) => uuid === selectedFlaw.uuid);
+      if (index !== -1) {


This is not a suggestion, just a comment.
This expression can also be written as !!~index, the ~ bitwise operator will flip the numbers and only -1 will return 0, so we can coerce that to boolean with the !!.
It is a bit obscure but I like it for some reason 😆

That's cool!

MrMarble · 2025-01-21T12:03:47Z

src/composables/useFlaw.ts


 export function useFlaw() {
-  return flaw;
+  return { flaw, relatedFlaws, blankFlaw, resetFlaw };


I think we could leave blankFlaw outside of the useFlaw composable as it is returning a constant value that does not modifies flaw in any way

I had this same thought

src/composables/useRelatedFlawTrackers.ts

MrMarble · 2025-01-22T08:29:25Z

src/composables/useRelatedFlawTrackers.ts

+  watch(relatedFlaws, () => {
+    selectedRelatedFlaws.value.forEach((selectedFlaw, index) => {
+      const relatedIndex = relatedFlaws.value.findIndex(({ uuid }) => uuid === selectedFlaw.uuid);
+      if (index !== -1) {


I think this was left behind after the renaming, index comes from the loop so it will never be -1

Suggested change

if (index !== -1) {

if (relatedIndex !== -1) {

Once this is fixed I'll leave my approval, rest LGTM

♻️ Unify flaw source of truth ♻️ Don't do circular dependencies ♻️ Never use null for default flaw value

MrMarble · 2025-01-28T09:08:54Z

src/composables/useRelatedFlawTrackers.ts

+  async function refreshRelatedFlaws() {
+    isLoadingTrackers.value = true;
+    // After filing new trackers, there is a delay before the new trackers are available on the flaws
+    await new Promise(resolve => setTimeout(resolve, 2000));


I don't think a random timeout is a solution, tracker creation is asynchronous, it depends not only on Jira API response time, but also if workers are idle and ready to take the creation task or busy with periodic work / requests from other users, this value might work locally, but it will surely fail in production.

Maybe we could implement a polling system, make a request every second or half a second to the /affects/<affect_uuid> endpoint to see if the tracker is already there, if it takes more than 10 requests (5 to 10 seconds depending on the delay) we show an alert with Tracker creation took too long so we don't keep the user waiting.

What do you think?

Another thing I saw while testing this is that when we make the POST to trackers for creation, the response already has everything we need to show the tracker in the UI, can't we use this to update the trackers table? (status is empty but since we are creating it, I think we can assume it is NEW)

I noted this as well, but it does not have the relational associations. Additionally, matching based on ps_update_stream will not yield correct associations in some situations.

I don't think a random timeout is a solution, tracker creation is asynchronous, it depends not only on Jira API response time, but also if workers are idle and ready to take the creation task or busy with periodic work / requests from other users, this value might work locally, but it will surely fail in production.

Maybe we could implement a polling system, make a request every second or half a second to the /affects/<affect_uuid> endpoint to see if the tracker is already there, if it takes more than 10 requests (5 to 10 seconds depending on the delay) we show an alert with Tracker creation took too long so we don't keep the user waiting.

What do you think?

I agree and had considered the same, but have concerns about implementation complexity and resource allocation for this particular PR.

While not ideal, I think this is candidate solution for a temporary fix, though I think providing a warning that a hard refresh may be required. I agree with the polling approach, and while not a massive extra lift, does expand the scope of the solution. I considered this strategy, but we are already experiencing scope creep here. I agree with you, but from an operational perspective, I am somewhat hesitant to implement a more robust solution at this time.

It is worth noting that the JIRA sync is non-blocking for the relational associations to take place and that the 2 seconds seems sufficient for the relational associations to propagate in my testing.

Alright, so pending the discussion between @MrMarble and I during our code review, I realized we can in fact use the data from the request-response cycle for trackers since each tracker is created one by one. This however does expand the scope of the PR, and we have fix planned documented in OSIDB-3961.

📝 Update changelog 🐛 Fix typo ✨ Atomically refresh trackers for affects after filing 🔥 Remove unused reference 🔥 Remove unused reference 🐛 Separate refresh logic to avoid weird bundled code in vitest 🚨 Handle linter warning 🔥 Remove unused code 🐛 Fix trackers refresh behavior ✅ Align test mocks with streamlined data flow ✅ Update tests for never-null flaws 🐛 Fix index mismatch ♻️ Simplify mock 🔥 Remove dead code 🐛 Exclude affect history from comparison 🐛 Correct index reference 🚨 Remove unused references 🚨 Remove unused references 🚨 Remove unused references ♻️ Remove blankFlaw from composable scope 🔧 Make related flaws query faster 🐛 Fix refreshing of related flaw trackers Used incorrect source of truth for related flaws

📸 Update snapshots 📝 Document limitations in comments 📸 Update snapshots

🚨 Remove blank line

superbuggy requested review from C-Valen and a team November 1, 2024 16:21

superbuggy self-assigned this Nov 4, 2024

MrMarble reviewed Nov 6, 2024

View reviewed changes

src/composables/useFlawModel.ts Outdated Show resolved Hide resolved

superbuggy marked this pull request as draft December 10, 2024 15:03

superbuggy force-pushed the feature/OSIDB-3483-fix-tracker-refresh-after-filing branch from e8c74c2 to 357dad7 Compare December 11, 2024 14:43

superbuggy force-pushed the feature/OSIDB-3483-fix-tracker-refresh-after-filing branch from 357dad7 to 8cca421 Compare January 13, 2025 19:10

superbuggy marked this pull request as ready for review January 14, 2025 15:27

superbuggy marked this pull request as draft January 15, 2025 15:47

superbuggy force-pushed the feature/OSIDB-3483-fix-tracker-refresh-after-filing branch from f7c0752 to d5e4165 Compare January 16, 2025 17:40

superbuggy marked this pull request as ready for review January 16, 2025 17:47

superbuggy force-pushed the feature/OSIDB-3483-fix-tracker-refresh-after-filing branch from d5e4165 to 5bc3463 Compare January 16, 2025 17:55

MrMarble requested changes Jan 20, 2025

View reviewed changes

src/components/FlawAffects/FlawAffects.vue Outdated Show resolved Hide resolved

src/composables/useFlaw.ts Show resolved Hide resolved

src/composables/useFetchFlaw.ts Outdated Show resolved Hide resolved

C-Valen reviewed Jan 21, 2025

View reviewed changes

MrMarble reviewed Jan 21, 2025

View reviewed changes

src/composables/useRelatedFlawTrackers.ts Outdated Show resolved Hide resolved

MrMarble reviewed Jan 22, 2025

View reviewed changes

superbuggy force-pushed the feature/OSIDB-3483-fix-tracker-refresh-after-filing branch from 2af8bdd to 9d02fe7 Compare January 22, 2025 15:34

C-Valen previously approved these changes Jan 22, 2025

View reviewed changes

superbuggy dismissed C-Valen’s stale review via b408e9b January 22, 2025 15:40

superbuggy force-pushed the feature/OSIDB-3483-fix-tracker-refresh-after-filing branch from 9d02fe7 to b408e9b Compare January 22, 2025 15:40

MrMarble previously approved these changes Jan 22, 2025

View reviewed changes

superbuggy dismissed MrMarble’s stale review via 92555c7 January 22, 2025 17:28

♻️ Unify flaw source of truth

af49e86

♻️ Unify flaw source of truth ♻️ Don't do circular dependencies ♻️ Never use null for default flaw value

superbuggy force-pushed the feature/OSIDB-3483-fix-tracker-refresh-after-filing branch from 81070ee to 87c1783 Compare January 24, 2025 14:58

MrMarble reviewed Jan 28, 2025

View reviewed changes

MrMarble previously approved these changes Jan 28, 2025

View reviewed changes

superbuggy dismissed MrMarble’s stale review via fa35dbb January 28, 2025 14:57

superbuggy added 2 commits January 28, 2025 10:02

👔 Attempt to accomodate delay

52aff6a

superbuggy added 2 commits January 28, 2025 10:05

✨ Indicate tracker refreshing

309f971

📸 Update snapshots 📝 Document limitations in comments 📸 Update snapshots

✨ Allow for cache busting on flaw requests

204da29

🚨 Remove blank line

superbuggy force-pushed the feature/OSIDB-3483-fix-tracker-refresh-after-filing branch from d86b8ec to 204da29 Compare January 28, 2025 15:05

MrMarble approved these changes Jan 28, 2025

View reviewed changes

superbuggy merged commit d61ea68 into main Jan 28, 2025
5 checks passed

superbuggy deleted the feature/OSIDB-3483-fix-tracker-refresh-after-filing branch January 28, 2025 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Refresh affect trackers after filing trackers #472

🐛 Refresh affect trackers after filing trackers #472

superbuggy commented Nov 1, 2024 •

edited

Loading

MrMarble left a comment •

edited

Loading

C-Valen left a comment

MrMarble Jan 21, 2025

MrMarble Jan 21, 2025

MrMarble Jan 21, 2025

MrMarble Jan 21, 2025

superbuggy Jan 21, 2025

MrMarble Jan 21, 2025

superbuggy Jan 21, 2025

MrMarble Jan 22, 2025

MrMarble Jan 22, 2025

MrMarble Jan 28, 2025

MrMarble Jan 28, 2025

superbuggy Jan 28, 2025

superbuggy Jan 28, 2025

superbuggy Jan 28, 2025

🐛 Refresh affect trackers after filing trackers #472

🐛 Refresh affect trackers after filing trackers #472

Conversation

superbuggy commented Nov 1, 2024 • edited Loading

OSIDB-3483 Fix affect tracker refreshing after filing trackers

Checklist:

Summary:

Changes:

Considerations:

MrMarble left a comment • edited Loading

Choose a reason for hiding this comment

C-Valen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superbuggy commented Nov 1, 2024 •

edited

Loading

MrMarble left a comment •

edited

Loading