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

Update QuotationSorting #725

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alessiocol
Copy link

@alessiocol alessiocol commented Feb 3, 2025

Selected sorting fields are added to QuotationSorting.

Checklist:

  • Test coverage
  • Version bump
  • Changelog update

@alessiocol alessiocol force-pushed the PT-5960-populate-quotationsorting-container branch from 97871f5 to 135fdea Compare February 3, 2025 15:45
@alessiocol alessiocol marked this pull request as ready for review February 3, 2025 15:53
@alessiocol alessiocol requested review from a team as code owners February 3, 2025 15:53
@@ -207,7 +207,10 @@ def choose_feasibility(self, feasibility_id: str, accepted_option_id: str) -> di


class QuotationSorting:
Copy link
Contributor

@andher1802 andher1802 Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question, non-binding, should we add unit tests for the new sorting criteria in here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I just expanded the test, please let me know your thoughts

CHANGELOG.md Outdated
@@ -29,6 +29,10 @@ You can check your current version with the following command:
```

For more information, see [UP42 Python package description](https://pypi.org/project/up42-py/).
## 2.2.0a23
**Feb 3, 2025**
- Updated `QuotationSorting`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Maybe "Add sorting fields to QuotationSorting"?

@@ -207,7 +207,10 @@ def choose_feasibility(self, feasibility_id: str, accepted_option_id: str) -> di


class QuotationSorting:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

andher1802
andher1802 previously approved these changes Feb 14, 2025
Copy link
Contributor

@andher1802 andher1802 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approved since @matheuspinheirosantos did and I think the merge rights is still not updated.

@andher1802 andher1802 dismissed their stale review February 14, 2025 14:46

did not have merge rights either

@alessiocol alessiocol force-pushed the PT-5960-populate-quotationsorting-container branch from 33f7115 to 527cde7 Compare March 19, 2025 16:12
Copy link

@jonathanBareket jonathanBareket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (From a Python beginner 🐍 )

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.

4 participants