vuestorefront / vsf-capybara

Capybara is a Storefront UI based theme for Vue Storefront. Always Open Source, MIT license. Made with :green_heart: by Vue Storefront.
https://capybara.storefrontcloud.io/
MIT License
154 stars 84 forks source link

[Bug] AProductQuantity calculated error for max value (and validation) #566

Open infinityredux opened 3 years ago

infinityredux commented 3 years ago

Current behaviour

Found via code inspection in editor, because the isBelowMax variable is never actually used. Looking at the code currently it's checking isBelowZero a second time (which is redundant because if isBelowZero is true, the second if will never be reached.)

Expected behavior

The value of isBelowMax should be used, presumably instead for the second isBelowZero,

Steps to reproduce the issue

Current relevant code:

    error () {
      const isBelowZero = !this.$v.value.numeric || !this.$v.value.minValue || !this.$v.value.required
      if (isBelowZero) return this.$t(`Quantity must be positive integer`)
      const isBelowMax = this.maxQuantity && this.value && !this.$v.value.maxValue
      if (isBelowZero) {
        return this.$t('Quantity must be below {quantity}', {
          quantity: this.maxQuantity
        })
      }
      return ''
    }

Can you handle fixing this bug by yourself?

Environment details

N/A

Additional information

Looking closer at this, I'm also not certain the actual maxValue validation is correct either:

  validations () {
    return {
      value: {
        minValue: minValue(1),
        maxValue: maxValue(this.maxQuantity) && this.unlimitQuantity,
        numeric,
        required
      }
    };
  },

Based on the above, the Vuelidate maxValue function should return true (valid) as long as value is less than the given max value. However, the overall maxValue validation cannot evaluate to true unless value is less than this.maxQuantity and the quantity is also unlimited (i.e. this.unlimitQuantity is true.)

So, unless I've misunderstood the purpose of unlimitQuantity (specifying that this product actually has an unlimited quantity) if this is true, the maxValue validation should always be valid (there cannot be a value above unlimited / infinite.) In other words, I believe this should be a logical or instead of and?

Finally, the name isBelowMax bothers me... it will only be true if the maxValue validation fails (i.e. the value is above max.) So it should probably be isAboveMax instead?

ymaheshwari1 commented 3 years ago

@infinityredux Have you found a solution for this? If not, then can I pick this issue?

infinityredux commented 3 years ago

@infinityredux Have you found a solution for this? If not, then can I pick this issue?

Sorry, thought I would have had time to fix by now, but other things wound up taking priority. Feel free to pick up the issue.