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

Fix: Overlapping timeframe keys #3640

Merged
merged 1 commit into from
Nov 6, 2024
Merged

Fix: Overlapping timeframe keys #3640

merged 1 commit into from
Nov 6, 2024

Conversation

mmioana
Copy link
Contributor

@mmioana mmioana commented Nov 5, 2024

Description

  • This PR addresses the bug related to overlapping timeframe keys.
  • Given for the moment we query data only for the last 4 months (which is less than the actual number of months in a year), the issue was not immediately visible.

Testing

TODO: Let's test the retrieved data contains the full date keys and the changes didn't impact the Total in and out chart

  • Step 1. First make sure you have started your dev env and have data in your colony
  • Step 2. Visit http://localhost:9091/planex and make sure the chart has data to display for the current month

Screenshot 2024-11-05 at 16 33 55

query MyQuery {
  getDomainBalance(
    input: {
      colonyAddress: "<COLONY_ADDRESS_HERE>", 
      timeframePeriod: 4, 
      timeframeType: WEEKLY
    }
  ) {
    timeframe {
      key
      value {
        total
        totalIn
        totalOut
      }
    }
  }
}

Screenshot 2024-11-05 at 16 35 36

  • Step 5. Now please use different timeframePeriod and timeframeType values (ideally timeframePeriod > 12 if you use Monthly timeframeType or timeframePeriod > 31 for Daily)

Important

With the latest changes to fetch data directly from the network for the Total timeframeType, the timeframe array should be empty

Diffs

New stuff

  • getFullPeriodFormat to retrieve the whole needed date for a timeframe entry
  • parseTimeframeKey to parse the timeframe key before using it for the chart data

Contributes to #3555

@mmioana mmioana self-assigned this Nov 5, 2024
@mmioana mmioana marked this pull request as ready for review November 5, 2024 15:41
@mmioana mmioana requested review from a team as code owners November 5, 2024 15:41
@mmioana mmioana linked an issue Nov 5, 2024 that may be closed by this pull request
rdig
rdig previously approved these changes Nov 5, 2024
Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Queries seem to be running correctly and time intervals seem to not be overlapping anymore

Screenshot from 2024-11-05 20-05-50
Screenshot from 2024-11-05 20-06-38
Screenshot from 2024-11-05 20-07-14

Base automatically changed from fix/3555-total-funds to master November 5, 2024 20:13
@mmioana mmioana dismissed rdig’s stale review November 5, 2024 20:13

The base branch was changed.

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Nice solution 💪
After starting the environment, we got some payments
image
Weekly data:
image
Daily:
image
Monthly:
image
Large timeframes also work (can't really screenshot the longboi 😅 ), so nice one 🚢

Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Nice job! This is working great, exactly as the testing steps describe:

Screenshot 2024-11-06 at 12 01 34 Screenshot 2024-11-06 at 12 03 39 Screenshot 2024-11-06 at 12 02 11 Screenshot 2024-11-06 at 12 03 12

Fix: Overlapping timeframe keys
@mmioana mmioana force-pushed the fix/3555-timeframe-keys branch from 8dbdd99 to 2e2f004 Compare November 6, 2024 12:47
@mmioana mmioana merged commit 57d5c97 into master Nov 6, 2024
4 of 6 checks passed
@mmioana mmioana deleted the fix/3555-timeframe-keys branch November 6, 2024 12:52
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.

[QA Dashboard] Total funds bug
4 participants