Skip to content

Conversation

@odeimaiz
Copy link
Member

@odeimaiz odeimaiz commented Jul 2, 2025

What do these changes do?

The frontend wasn't caching Service's Pricing Plans correctly. This PR fixes it.

Buggy:
Buggy

Fixed:
Fixed

Related issue/s

How to test

Dev-ops

@odeimaiz odeimaiz self-assigned this Jul 2, 2025
@odeimaiz odeimaiz requested a review from Copilot July 2, 2025 12:29
@odeimaiz odeimaiz added this to the Engage milestone Jul 2, 2025
@odeimaiz odeimaiz added bug buggy, it does not work as expected a:frontend issue affecting the front-end (area group) labels Jul 2, 2025
Copy link
Contributor

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

Fixes the caching mechanism for service pricing plans by switching from a single-string key to a nested cache keyed by service identifier and version.

  • Extract service identifier and version for cache lookup instead of using the full URL string.
  • Update cache retrieval to check nested keys, and cache storage to initialize and populate per-service-version entries.
  • Adjust logic in getPricingPlan to align with new nested cache structure.
Comments suppressed due to low confidence (3)

services/static-webserver/client/source/class/osparc/store/Services.js:354

  • [nitpick] Using generic names key and version can cause confusion. Consider renaming to serviceKey and serviceVersion (or cacheKey/cacheVersion) to clarify their purpose.
      const version = serviceUrl["version"];

services/static-webserver/client/source/class/osparc/store/Services.js:372

  • There are no existing tests covering the new nested cache behavior. Please add unit tests to verify cache lookups and storage for various service keys and versions.
          this.__pricingPlansCached[key][version] = pricingPlansData;

services/static-webserver/client/source/class/osparc/store/Services.js:353

  • The serviceUrl returned by getServiceUrl is typically a string, not an object with key and version properties. Instead, use the original serviceKey and serviceVersion parameters to index the cache.
      const key = serviceUrl["key"];

Copy link
Member

@mguidon mguidon left a comment

Choose a reason for hiding this comment

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

Merci. Could that be hot-fixed at some point?

Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

Thanks!

@odeimaiz
Copy link
Member Author

odeimaiz commented Jul 2, 2025

Merci. Could that be hot-fixed at some point?

Let's ask @pcrespov what label we should use for hotfix candidates 👍

@odeimaiz odeimaiz mentioned this pull request Jul 2, 2025
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

“There are only two hard things in Computer Science: cache invalidation and naming things.”
Phil Karlton

@pcrespov
Copy link
Member

pcrespov commented Jul 2, 2025

Let's ask @pcrespov what label we should use for hotfix candidates 👍

@matusdrobuliak66 @mguidon list them to the corresponding hotfix issue. In this case I found a one-for-all hotfix issue #8003

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

thanks

@odeimaiz
Copy link
Member Author

odeimaiz commented Jul 3, 2025

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jul 3, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • label=🤖-automerge
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 3, 2025

@odeimaiz odeimaiz enabled auto-merge (squash) July 3, 2025 12:32
@odeimaiz odeimaiz merged commit 7b1a8b9 into ITISFoundation:master Jul 3, 2025
59 checks passed
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:frontend issue affecting the front-end (area group) bug buggy, it does not work as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants