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

chore(add-item): permit insert flags when is the same product #187

Merged
merged 5 commits into from
Aug 4, 2022

Conversation

matheusgnreis
Copy link
Member

No description provided.

Copy link
Member

@leomp12 leomp12 left a comment

Choose a reason for hiding this comment

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

Não entendi, você quer adicionar flags no item que já existe?
É aquele caso do brinde? Se for na real não era pra você adicionar a flag no item que já existe, é pra ter a flag só no novo item mesmo (o brinde):

  • item 0: 5 granolas, sem flag
  • item 1: 1 granola, freebie

Do jeito que você tá fazendo aí mudaria o item 0, aí teria só um item com 6 granolas e todas freebie, não é isso? Aí não passaria no checkout...

@leomp12
Copy link
Member

leomp12 commented Aug 4, 2022

Se for pro caso do mesmo produto sendo pago e como brinde, eu acho que você tem que editar é essa condição aqui

if (
!item.kit_product &&
item.product_id === newItem.product_id &&
item.variation_id === newItem.variation_id &&
(!item.customizations || !item.customizations.length)
) {
passar a checar as flags aí também (se as flags não baterem não mantém o mesmo item, segue adicionando um novo)

@matheusgnreis
Copy link
Member Author

Não entendi, você quer adicionar flags no item que já existe? É aquele caso do brinde? Se for na real não era pra você adicionar a flag no item que já existe, é pra ter a flag só no novo item mesmo (o brinde):

* item 0: 5 granolas, sem flag

* item 1: 1 granola, freebie

É examente isso que queria fazer desde o início, achava que por ser mesmo id, ele atualizava e não inseria outro.

const newItemFlags = newItem.flags ? newItem.flags : false
if (!itemFlags && !newItemFlags) {
return true
} else if (!itemFlags && newItemFlags || itemFlags && !newItemFlags) {
Copy link
Member

Choose a reason for hiding this comment

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

esses AND e OR aqui estão ambíguos, teria que ter um parênteses na segunda parte

Copy link
Member

Choose a reason for hiding this comment

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

E o StandardJS te mostraria isso antes de mim inclusive..

Copy link
Member Author

Choose a reason for hiding this comment

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

Ue, eu estou com isso instalado no meu vs code

Copy link
Member

Choose a reason for hiding this comment

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

Tem que instalar as dependências (sempre) do repositório também
Fez isso?

src/lib/check-flags.js Outdated Show resolved Hide resolved
Copy link
Member

@leomp12 leomp12 left a comment

Choose a reason for hiding this comment

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

👍🏾

} else if (!itemFlags && newItemFlags || (itemFlags && !newItemFlags)) {
return false
} else {
return itemFlags.every(flag => newItemFlags.includes(flag))
Copy link
Member

Choose a reason for hiding this comment

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

Dá pra passar, mas (FYI) isso não vai funcionar se itemFlags for um array vazio ou se tiver menos flags que o do novo item

Copy link
Member

Choose a reason for hiding this comment

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

E cagou indentação aqui, o que me leva a crer que não tá usando o linter 😞 , nem testando...

@leomp12 leomp12 merged commit a9addd9 into ecomplus:master Aug 4, 2022
Comment on lines +42 to +43
const check

Copy link
Member

Choose a reason for hiding this comment

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

😐

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

isso é um erro de sintaxe, não compila 🙃

Copy link
Member Author

Choose a reason for hiding this comment

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

estava com outro instalado

Copy link
Member Author

Choose a reason for hiding this comment

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

É, sei de onde brotou isso nao, devo ter copiado errado. Testei no meu local, mas nao conseguia subir por nao ter permissão, então copiei e coloquei direto no github

Copy link
Member Author

Choose a reason for hiding this comment

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

Foi mal

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.

2 participants