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

Add more attributes to output DTOs for negotiator resources #598

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

Conversation

asulis
Copy link
Contributor

@asulis asulis commented Jan 17, 2025

Negotiator pull request:

Description:

Not all attributes for Organizations, resources and Network are currently provided in the DTOs output objects (and so they are not available in the Frontend). This PR adds these attributes to the DTOs objects.

Checklist:

Make sure you tick all the boxes below if they are true or do not apply before you ask for review

  • [ x] I have performed a self-review of my code
  • [ x] I have made my code as simple as possible
  • [ x] I have added relevant tests for my changes and the code coverage has not dropped substantially
  • [ x] I have removed all commented code
  • [ x] I have updated the documentation in all relevant places (Javadoc, Swagger, MDs...)
  • [ x] I have described the PR and added a meaningful title in the Conventional Commits format

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.61%. Comparing base (628c269) to head (ddcaa08).

Files with missing lines Patch % Lines
...gotiator/governance/organization/Organization.java 50.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #598      +/-   ##
============================================
- Coverage     84.63%   84.61%   -0.02%     
  Complexity     1011     1011              
============================================
  Files           150      150              
  Lines          3795     3797       +2     
  Branches        237      237              
============================================
+ Hits           3212     3213       +1     
- Misses          414      415       +1     
  Partials        169      169              
Flag Coverage Δ
unit 84.61% <50.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@RadovanTomik RadovanTomik left a comment

Choose a reason for hiding this comment

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

please see comment, plus I would rather add unit tests for the mappers not heavy integration tests to test something as simple as mapping of attributes.

@@ -43,4 +43,7 @@ public class OrganizationDTO {

@Schema(description = "URI of the organization", example = "https://organization.org")
private String uri;

@Schema(description = "Indicates if the organization is withdrawn", example = "false")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does withdrawn mean? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to "revoked" in 4ec4c46

Copy link
Collaborator

Choose a reason for hiding this comment

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

what does revoked mean? :)

Comment on lines 38 to 43
@Nullable private String description;

@Nullable private String contactEmail;

@Nullable private String uri;

Copy link
Collaborator

Choose a reason for hiding this comment

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

again this DTO is deprecated. The ResourceResponseModel and ResourceWithStatusDTO are used

Copy link
Contributor Author

@asulis asulis Jan 21, 2025

Choose a reason for hiding this comment

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

The problem here is that it has been deprecated, but several tests, changed in this PR, still use this class. Should we change again the tests and definitely remove this class ? Same in the code, for example ResourceModelMapper is still using it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

do not remove the class for now, its fine that some tests use it

@asulis asulis force-pushed the feat/add_sync_attrs_to_dto branch from 3876d0f to ddcaa08 Compare January 29, 2025 09:05
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