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.

Sign up for GitHub

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

Jump to bottom

[BREAKING CHANGE: do not merge yet] cc-addon-elasticsearch-options: support multi-currency and disabled billing #1295

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[BREAKING CHANGE: do not merge yet] cc-addon-elasticsearch-options: support multi-currency and disabled billing #1295

wants to merge 5 commits into from

Conversation

florian-sanders-cc
Copy link
Contributor

@florian-sanders-cc florian-sanders-cc commented Jan 10, 2025 •
edited
Loading

Fixes #1172
Fixes #1230

What does this PR do?

  • Adapts the cc-addon-elasticsearch-options component to support other currencies,
  • Adapts the cc-addon-elasticsearch-options component to support cases where billing is disabled. In this case, sentences mentionning additional costs should be hidden, including in loading state when the cost is in skeleton mode while we fetch info.
  • Both these changes led to some refactoring and breaking changes:
    • other cc-addon-*-options components do not fetch any data, they update data when you enable / disable options but there is no loading vs loaded logic.
    • on the contrary the cc-addon-elasticsearch-options component does fetch data and has a loaded vs loading logic:
      • we need to fetch what is the default instance of a Kibana and an APM instance,
      • if billing is enabled, we need to fetch the cost of the default instance of a Kibana and an APM instance.
    • this is why a state property was added. The types may look too complex for such a tiny component, feel free to suggest improvements but bear in mind that the subject is not as simple as it looks (there are comments that should help within the component & its types).

Note: the skeleton display is fairly broken right now, just like in cc-invoice-list but there's already an issue about this (see #1293)

How to review?

  • Check the code,
  • Check the stories (including the 2 new stories: loading & no monthly cost),
  • 2 or 3 reviewers should be enough for this one.

@florian-sanders-cc florian-sanders-cc self-assigned this Jan 10, 2025
@florian-sanders-cc florian-sanders-cc force-pushed the cc-addon-elasticsearch-options/support-multi-currency branch 2 times, most recently from 4d77e28 to b4ef1ba Compare January 10, 2025 09:46
@github-actions GitHub Actions
Copy link
Contributor

🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-addon-elasticsearch-options/support-multi-currency/index.html.

This preview will be deleted once this PR is closed.

pdesoyres-cc
pdesoyres-cc approved these changes Jan 13, 2025
View reviewed changes
Copy link
Contributor

@pdesoyres-cc pdesoyres-cc left a comment

Choose a reason for hiding this comment

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

Well done Florian, I have some minor questions and nitpicks.

src/components/cc-addon-elasticsearch-options/cc-addon-elasticsearch-options.js Outdated Show resolved Hide resolved
src/components/cc-addon-elasticsearch-options/cc-addon-elasticsearch-options.types.d.ts Show resolved Hide resolved
HeleneAmouzou
HeleneAmouzou approved these changes Jan 13, 2025
View reviewed changes
Copy link
Contributor

@HeleneAmouzou HeleneAmouzou left a comment

Choose a reason for hiding this comment

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

GG ! 👏
Indeed, it is not as simple as it looks !
I just have a quesiton; as this component fetch data, what happen if the fetch did not work ? How does it handle error cases ?

@florian-sanders-cc
Copy link
Contributor Author

I just have a quesiton; as this component fetch data, what happen if the fetch did not work ? How does it handle error cases ?

@HeleneAmouzou this is a great question !!
I thought about adding some error state but I felt the info was not "important" enough to do so and that thanks to skeletons, we already had the important info.
If it fails to load, then you still know that kibana / apm will start a new app and that it may cost you something (if billing is enabled).

To be honest, if it fails to load, there's a very high chance that you won't be able to create your add-on anyway (because this data comes from the instances route + the billing route. Both these routes are essential to the creation tunnel).

This is subjective so don't hesitate to say if you think we should handle the error case more specifically (for now error = stuck in loading).

Galimede
Galimede approved these changes Jan 15, 2025
View reviewed changes
Copy link
Member

@Galimede Galimede left a comment

Choose a reason for hiding this comment

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

Nice work Florian, apart from a few nitpicks LGTM, GG! 💪

src/translations/translations.fr.js Show resolved Hide resolved
src/translations/translations.en.js Show resolved Hide resolved
src/components/cc-addon-option-form/cc-addon-option-form.js Outdated Show resolved Hide resolved
src/components/cc-addon-option-form/cc-addon-option-form.js Outdated Show resolved Hide resolved
@florian-sanders-cc florian-sanders-cc changed the title cc-addon-elasticsearch-options: support multi-currency and disabled billing [BREAKING CHANGE: do not merge yet] cc-addon-elasticsearch-options: support multi-currency and disabled billing Jan 16, 2025
@florian-sanders-cc
feat(cc-addon-elasticsearch-options)!: support multi-currency & disab…
0fd7144 
…led billing
@florian-sanders-cc
refactor(cc-tile-scalability): adapt to Flavor type changes
d03fc7e 
The `Flavor` type has been changed to remove `monthlyCost`.
The `monthlyCost` is only necessary for the `cc-addon-elasticsearch-options`.
There was no need to include it in the standard `Flavor` type used by other components.
@florian-sanders-cc
refactor(cc-tile-scalability.stories): fix type imports
b48ec89
@florian-sanders-cc
refactor(cc-addon-elasticsearch-options): feedback Pierre (fixup later)
f6aa21b
@florian-sanders-cc
refactor(cc-addon-elasticsearch-options): feedback Mathieu (fixup later)
e6240f0
@florian-sanders-cc florian-sanders-cc force-pushed the cc-addon-elasticsearch-options/support-multi-currency branch from 8ca655c to e6240f0 Compare January 16, 2025 10:14
@florian-sanders-cc florian-sanders-cc added the breaking-change Something that will required a major semver release label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeleneAmouzou HeleneAmouzou HeleneAmouzou approved these changes

@pdesoyres-cc pdesoyres-cc pdesoyres-cc approved these changes

@roberttran-cc roberttran-cc roberttran-cc approved these changes

@Galimede Galimede Galimede approved these changes

@hsablonniere hsablonniere Awaiting requested review from hsablonniere

@clara-layus-cc clara-layus-cc Awaiting requested review from clara-layus-cc

Assignees

@florian-sanders-cc florian-sanders-cc

Labels
breaking-change Something that will required a major semver release
Projects
None yet
Milestone
No milestone
5 participants
@florian-sanders-cc @Galimede @roberttran-cc @pdesoyres-cc @HeleneAmouzou