-
Notifications
You must be signed in to change notification settings - Fork 393
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
Bug/CXINT-2438: Update Sort Keys in Spartacus PDF Invoices Component #18488
Conversation
byCreatedAtAsc: 'createdAt:asc', | ||
byCreatedAtDesc: 'createdAt:desc', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me understand the backward compatibility consequences of this change?
I can understand that users of the newest backend API and newest Spartacus will get desired behavior.
And users of the old backend API and old Spartacus will also get desired correct behavior.
But does this change mean that some customers that use the old version of the backend API, but the newest Spartacus will encounter problems?
Analogical question: for users of new API but old Spartacus - will they encounter problems?
Should we wrap this change with an opt-in feature flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it should be wrapped in opt-in feature flag, i have made the necessary changes
3 flaky tests on run #42637 ↗︎
Details:
cypress/e2e/ssr/pages.core-e2e.cy.ts • 3 flaky tests • SSR
Review all test suite changes for PR #18488 ↗︎ |
@@ -45,6 +45,9 @@ import { InvoicesListComponent } from './invoices-list/invoices-list.component'; | |||
guards: [AuthGuard], | |||
}, | |||
}, | |||
features: { | |||
useUpdatedSortMappingForInvoices: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend starting with the name of the library, e.g. pdfInvoices
and if possible describe semantically the change: e.g. sortByInvoiceDate
.
What do you think about: pdfInvoicesSortByInvoiceDate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is better. have changed the name
https://jira.tools.sap/browse/CXINT-2438