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: Integrate with Butter CMS #66

Merged
merged 8 commits into from
Nov 8, 2024

Conversation

soniaklimas
Copy link
Contributor

I want to merge this change because it integrates with ButterCMS.

Pull Request Checklist

  • Test the changes locally to ensure they work as expected.
  • Document the testing process and results in the pull request description. (Screen recording, screenshot etc)
  • Include new tests for any new functionality or significant changes.
  • Ensure that tests cover edge cases and potential failure points.
  • Document the impact of the changes on the system, including potential risks and benefits.
  • Provide a rollback plan in case the changes introduce critical issues.
  • Update documentation to reflect any changes in functionality.

@soniaklimas soniaklimas added the WIP label Nov 4, 2024
@soniaklimas soniaklimas self-assigned this Nov 4, 2024
Copy link

vercel bot commented Nov 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nimara-ecommerce ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 8, 2024 10:28am

Copy link
Collaborator

@karolkarolka karolkarolka left a comment

Choose a reason for hiding this comment

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

I've could seen some of the functions being moved from one dir to another like serializers, but I think that you've removed some code related to Saleor CMS and we should keep it

@@ -1,23 +1,3 @@
import type { LanguageCodeEnum } from "@nimara/codegen/schema";
Copy link
Collaborator

Choose a reason for hiding this comment

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

why Saleor CMS types are being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved types related to use case to folder use-case (same as search case).

@@ -1,12 +0,0 @@
import { cmsPageGetUseCase } from "#root/use-cases/cms-page/cms-page-get-use-case";
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you delete the Saleor CMS infra? I've thought about keeping ButterCMS and Saleor infra and using one or another depending of the needs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved it to another file.

} from "../graphql/queries/generated";
import type { CMSMenuGetInfra, SaleorCMSMenuServiceConfig } from "../types";

const serializeMenuItem = ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't it still needed for Saleor CMS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is but I created separated file for serializers and move it there.

import type { MenuItem } from "@nimara/domain/objects/Menu";
import { loggingService } from "@nimara/infrastructure/logging/service";

export const getAttributes = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

why it is being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not use it anymore (there is new serializer which returns fields of cms data).

? item.category[0].split("category[_id=")[1]?.split("]")[0] || ""
: "";

const category: Category | null = categoryId
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const category: Category | null = categoryId
const category = categoryId

Copy link
Collaborator

@karolkarolka karolkarolka left a comment

Choose a reason for hiding this comment

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

one last very small change and you can merge

typeof item.page === "string"
? item.page.split("/").filter(Boolean).pop() || ""
: "";
const page: Page | null = item.page
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const page: Page | null = item.page
const page = item.page

@soniaklimas soniaklimas merged commit cf0fe2d into develop Nov 8, 2024
3 checks passed
@soniaklimas soniaklimas deleted the MS-801-butter-cms-integration branch November 8, 2024 10:29
items: MenuGet_menu_Menu_items_MenuItem[] | ButterCMSMenuItem[],
source: "saleor" | "butterCms",
): Menu => {
if (source === "saleor") {

Choose a reason for hiding this comment

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

To jest problem trochę. Powinniśmy używać kompozycji zamiast budować takie warunki.
Co myslisz o tym by stworzyć dwie funkcje: jedną dla saleor, drugą dla butter.
Wtedy jedna z nich moze byc uzyta w infrze buttera, a druga w infrze saleora.
Od poziomu infry powinnismy byc provider specifc, ale z wyraznym oddzieleniem kodu różnych providerów, by uniknąc tego ze zmiana dla jednego providera spowoduje bug dla drugiego.

fields: SelectionAttributeFragment[] | ButterCMSPageFields,
source: "saleor" | "butterCms",
): PageField[] => {
if (source === "saleor") {

Choose a reason for hiding this comment

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

tak samo. Nigdzie nie powinnismy miec conditional z nazwą providera.

);

return {
menu: serializeMenu(

Choose a reason for hiding this comment

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

Tutaj i tak jesteśmy w kontekscie providera.

karolkarolka pushed a commit that referenced this pull request Dec 4, 2024
karolkarolka pushed a commit that referenced this pull request Dec 5, 2024
karolkarolka pushed a commit that referenced this pull request Dec 5, 2024
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.

3 participants