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

Insufficent validation for city field #2653

Closed
2 tasks done
ArturDivante opened this issue Mar 25, 2019 · 7 comments
Closed
2 tasks done

Insufficent validation for city field #2653

ArturDivante opened this issue Mar 25, 2019 · 7 comments
Assignees
Labels
bug Bug reports P4: Nice to have Priority mark - it's nice to have
Milestone

Comments

@ArturDivante
Copy link
Collaborator

ArturDivante commented Mar 25, 2019

Related issue

#2317

Steps to reproduce the issue

  1. Place some items in cart and go to checkout
  2. Fill required fields
  3. Input ONLY numbers (i.e. '001') in City* field
  4. Try to place and an order

Current behavior

  • City field accept that input
  • 500 error appears in console
  • Server error message appears on the front.

Expected behavior

Validation on the front should match backend reqirements
OR
Front error message should point to the problem

OrderTransfered

500 response:
{"code":500,"result":[{"keyword":"pattern","dataPath":".addressInformation.shippingAddress.city","schemaPath":"#/properties/addressInformation/properties/shippingAddress/properties/city/pattern","params":{"pattern":"[a-zA-Z]+"},"message":"should match pattern "[a-zA-Z]+""}]}

Repository

demo.stroefrontcloud.io

Can you handle fixing this bug by yourself?

  • NO

Which Release Cycle state this refers to? Info for developer.

Pick one option.

  • This is a bug report for current Stable version on https://demo.storefrontcloud.io and should be placed in next stable version hotfix - In this case Developer should create branch from hotfix or master branch and create Pull Request 4. Hotfix back to hotfix.

Additional information

For many reasons, city field should accept digits. But in case ONLY digits were placed into that field, a problem occurs. This might look a bit like edge case, but it's quite possible for customer to accidentally input street number or zip code into city field. In that case, error message wont show him, what is wrong

@ArturDivante ArturDivante added bug Bug reports P4: Nice to have Priority mark - it's nice to have labels Mar 25, 2019
@pkarw
Copy link
Collaborator

pkarw commented Mar 25, 2019

Related to #2578

@pkarw
Copy link
Collaborator

pkarw commented Mar 30, 2019

Hey @lukeromanowicz - You've just recently modified these schemas, would You take care of this one?

@pkarw pkarw added the vs-hackathon Tasks for the Hackathon label Mar 30, 2019
@lukeromanowicz lukeromanowicz self-assigned this Apr 1, 2019
@lukeromanowicz
Copy link
Contributor

@pkarw
all of the checkout fields are mostly are not validated except minimum length and being required. I see two options here. To minimize code duplication we could either:

  • write more meaningful errors in vsf-api and display these errors in frontend app. In this approach, we have to keep in mind that orders can be submitted in offline mode so handling and fixing checkout address errors should be possible both in offline and online mode.
  • move whole validation from middleware to frontend - as backend is validating incoming requests, there is no need to duplicate validation in the middleware

Of course, we can also simply add the same validation on the frontend as it is in middleware ending up with 3 places of order validation (front, middleware and backend).

Which way do we choose?

@pkarw
Copy link
Collaborator

pkarw commented Apr 4, 2019

The most of the validation should take place in the front end plus we should display better validation msgs from the server side I belive - mix of 1+2 from Your proposal

I mean - we’re offline first platform so we can’t allow the user to relying only on server side validation. However when we’re online and server returns error we should properly display them

Unfortunately magento api methods return only single error strings so it would be very hardness to achieve this behavior without writing native magento2 module that we like to avoid.

So I belive we can just add the same validation in the middleware like we have in the front end and then if magento returns error we can and should just display it in the notification (this is already taking place)

We probably just need to align the validation rules at this point plus maybe display the AJV errors better way in the front end (currently there is just a shot msg on internal validation error without the details)

@pkarw
Copy link
Collaborator

pkarw commented Apr 10, 2019

@lukeromanowicz would be awesome to have this done till 1.9

@pkarw pkarw added this to the 1.10.0-rc.1 milestone Apr 16, 2019
@pkarw pkarw mentioned this issue Apr 23, 2019
5 tasks
@lukeromanowicz
Copy link
Contributor

lukeromanowicz commented Apr 24, 2019

@pkarw it's not possible to implement UTF-8 complainant solution the right way until babel/babel#9892 is resolved.
Currently, it's possible to either choosing node 8 as target or '>0.5%','last 4 versions',. Setting both results with the same issue as not setting node 8 at all.

All we can do for now is to add polyfills for that on our own.

@lukeromanowicz lukeromanowicz removed the vs-hackathon Tasks for the Hackathon label Apr 24, 2019
@lukeromanowicz lukeromanowicz modified the milestones: 1.10.0-rc.1, 1.9.0 Apr 24, 2019
@patzick
Copy link
Collaborator

patzick commented Apr 24, 2019

Closing with related #2771

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports P4: Nice to have Priority mark - it's nice to have
Projects
None yet
Development

No branches or pull requests

4 participants