SAP / spartacus

Spartacus is a lean, Angular-based JavaScript storefront for SAP Commerce Cloud that communicates exclusively through the Commerce REST API.
Apache License 2.0
744 stars 389 forks source link

Changes to forms now that title code no longer mandatory #872

Closed Xymmer closed 5 years ago

Xymmer commented 5 years ago

Title code no longer required (dependency on RAY-74), we'll remove it on some pages and make sure it is optional for others

Remove from:

Display but make sure it is optional from:

Check if elsewhere.

Check related tickets #440 and #287 and see if they are fixed or we can close them. If not, pull them into the sprint and fix them.

Xymmer commented 5 years ago

update: RAY-74 was put on hold while waiting for a milestone, i've asked for it to get back on track

Xymmer commented 5 years ago

update: RAY-74 now merged and integrated with M3 check with gil to ensure the server your using has it or else it will fail still if trying to submit an address without it.

Pucek9 commented 5 years ago

Backend doesn't allow for register user without titleCode ('' or null):

message: "This field is required."
reason: "missing"
subject: "titleCode"
subjectType: "parameter"
type: "ValidationError"

Similarly in adding address / shipping address:

message: "This field is required and must to be between 1 and 255 characters long."
reason: "invalid"
subject: "titleCode"
subjectType: "parameter"
type: "ValidationError"
Xymmer commented 5 years ago

check with gil if we have latest milestones / cs's from them

Xymmer commented 5 years ago

also, i realized we'll be causing a problem with this change for people who don't have 1905. is there a way to test if this field is mandatory, and hide it or set it to 'none' (and we'd have to add 'none' to the backend)? the way people test for features in JS so that people using 1811 or 1808 aren't prevented from registering a user.

Pucek9 commented 5 years ago

also, i realized we'll be causing a problem with this change for people who don't have 1905. is there a way to test if this field is mandatory, and hide it or set it to 'none' (and we'd have to add 'none' to the backend)? the way people test for features in JS so that people using 1811 or 1808 aren't prevented from registering a user.

I have tried use 'none' as value. Validation is more complex. Registration:

      "message" : "TitleModel with code 'none' not found!",
      "type" : "UnknownIdentifierError"

Adding address:

      "message" : "No result for the given example [TitleModel (<unsaved>)] was found. Searched with these attributes: {code=none}",
      "type" : "UnknownIdentifierError"
Pucek9 commented 5 years ago

Branch with changes is already done, but need wait for testing possibility https://github.com/SAP/cloud-commerce-spartacus-storefront/tree/feature/GH-872

Xymmer commented 5 years ago

we'd have to add 'none' as a valid title value in the backend

Xymmer commented 5 years ago

Hey all, after talking with Patrick I realized that my request and the solution i proposed would result in emails being sent in the form of "None Bill Marcotte".

At the same time we need to be backwards compatible with 1811 because otherwise people won't be able to register.

If you submit an OCC request without title code, this is returned in the body:

       {
            "message": "This field is required.",
            "reason": "missing",
            "subject": "titleCode",
            "subjectType": "parameter",
            "type": "ValidationError"
        },

So:

Let me know if this makes sense!

Also make sure Matt knows about the spartacus vs backend version issue for documentation.

Xymmer commented 5 years ago

to add to this, i've asked tobias what he thinks of just adding a configuration titlecodemandatory=false and then we behave based on that. so we don't have to add too much logic to spartacus.

Xymmer commented 5 years ago

so still blocked :)

Pucek9 commented 5 years ago

Waiting for VPN credentials

Xymmer commented 5 years ago

if Mr. Pucek9 doesn't get vpn access soon, let's assign to someone else.

Pucek9 commented 5 years ago

Tested manually on: https://dev-com-17.accdemo.b2c.ydev.hybris.com:9002 - looks good

I've added workaround to convert empty string to null and vice versa in sending/fetching data in address book form. Please tell me what do you think about this change, maybe let decide to update backend?

Pucek9 commented 5 years ago

17: allow register and add address without selecting title (workaround converts empty string to null and null to empty string) 20: not allow for register without title (workaround changes server's global message from "'This field is required.'" to "Title is required" ). Not allow to add/update address without title, replaced message: "Invalid Address" / "Title is required"