-
Notifications
You must be signed in to change notification settings - Fork 0
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
[feat] Add renewal tab on project level #36
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 069709b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Just 2 ideas, I didn't spot any bugs
})} | ||
</DataGrid> | ||
</div> | ||
)} |
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.
Improvement idea (no bug): Would some alternative text be advisable in case you have commitments, but no inconsistencies? I think it would be good to show the customer, that he could check for inconsistencies here. Else, a customer might visit the UI, have no commitments and never know about the option to conveniently check inconsistencies. When he comes back for some renewal, he might be suprised to see that he has to fix some things first.
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.
Agreed. A info text is now available.
let inconsistencyHeadCells = [...headCells]; | ||
inconsistencyHeadCells.push({ key: "reason", label: "Reason" }); | ||
|
||
const renewablePerService = React.useMemo(() => { |
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.
Are commitments sorted by default when they come from the backend? If not, a default sorting by expiresAt
might be a good idea. even better would be to make the table sortable, but I guess there is no table component available which would offer this out of the box?
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.
Commitments are not specifically sorted from the get request to the commitments API.
They will just be sorted by their database ID.
For now the RenewalMangager takes care that commitments come sorted into this component.
There is no default implementation for sorting table columns (not even icons that indicate an asc or desc order). It has to be custom made. I add this to the requirements for upcoming cleanup and fixes, because it would be good to have a separate sorting Hook that I can use for the commitment table in the EditPanel as well.
For now the sort is updated to sort after serviceType first, then after expiration date.
This a PR to add the commitment renewal functionality.
Limes Ref: sapcc/limes#674
Goal is to display commitments that are about to expire. The user has the option to:
Below the Renewal table, a inconistency table will be displayed. It shows commitments in trasfer, pending commitmetns or commitments with another state that should be fixed before they can be renewed.
I opt to have this view, because before a renewal takes place, a clean state should be a prerequisite.
Image:

(the commitments will now be sorted by their category name first...)
Todos:
was_extended
attribute from the API in order to not list the commitments despite them beeing renewed already.During development I thought about how we get the renwable commitments. We can:
commitments
API and filter them out at UI level (currently implemented)commitments
endpoint.A separate endpoint should probably be implemented. However, it should also include the
inconsistencies
and that part was implemeted by speculation. I would like to hear your opinion on them first (correctness etc.)If they don't get implemented there (and are omitted instead), the UI still needs to filter them out from the existing
commitment
endpoint.Adding another attribute to the existing
commitment
API bears the same problem above with the inconistencies. Then the attribute should support different states.