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

[feat] Add renewal tab on project level #36

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

VoigtS
Copy link
Member

@VoigtS VoigtS commented Mar 11, 2025

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:

  • Renew all commitments
  • Renew commitments from a specific category
  • Renew singular commitments

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:
image

(the commitments will now be sorted by their category name first...)
Todos:

  • React to a was_extended attribute from the API in order to not list the commitments despite them beeing renewed already.
  • Omit a table if no applicable commitments are found.
  • Add proper unit tests

During development I thought about how we get the renwable commitments. We can:

  • Get the commitments API and filter them out at UI level (currently implemented)
  • Implement a separate endpoint that only gets renwable commitments
  • Add another attribute to the existing 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.

Copy link

changeset-bot bot commented Mar 11, 2025

🦋 Changeset detected

Latest commit: 069709b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sapcc/limes-ui Minor

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

wagnerd3
wagnerd3 previously approved these changes Mar 14, 2025
Copy link

@wagnerd3 wagnerd3 left a 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>
)}

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.

Copy link
Member Author

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(() => {

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?

Copy link
Member Author

@VoigtS VoigtS Mar 14, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants