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

[MHUB-626] feat: add createdAt and updatedAt to PageModel #811

Closed
wants to merge 1 commit into from

Conversation

samuelfullerthomas
Copy link
Contributor

@samuelfullerthomas samuelfullerthomas commented Mar 28, 2024

Acceptance Criteria

https://internalproxy-us-east-1.dev.cloud.coveo.com/docs/internal?urls.primaryName=Commerce%20Reporting%20Service#/Reporting%20Metrics/getQueryExpressionProductMetrics

Screenshot 2024-03-28 at 15 58 00 Screenshot 2024-03-28 at 15 57 44

The swagger is wrong, sadly. @kgonzalesCoveo

  • My changes are publicly available, documented, and deployed in production. (i.e. on Swagger)
  • JSDoc annotates each property added in the exported interfaces
  • The proposed changes are covered by unit tests
  • Commits containing breaking changes a properly identified as such
  • README.md is adjusted to reflect the proposed changes (if relevant)
  • My merge commit message will be conventional (See Conventional Commit)

Copy link
Contributor

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

https://github.com/coveo/api-standards/blob/main/accepted/0002-offset-pagination.md.
Modification to the pagination interface must follow this standard.
Modifying the pagination standard is possible but must be done according to the procedure.
It is strongly recommended that uncompliant services comply with the existing standard.

As a last resort, I'd like to invite you to add this distortion of the standard at your resource level.

@kgonzalesCoveo
Copy link

https://github.com/coveo/api-standards/blob/main/accepted/0002-offset-pagination.md. Modification to the pagination interface must follow this standard. Modifying the pagination standard is possible but must be done according to the procedure. It is strongly recommended that uncompliant services comply with the existing standard.

As a last resort, I'd like to invite you to add this distortion of the standard at your resource level.

It does not look like the PageModel provided by the base service complies with the guideline.

https://github.com/coveo-platform/coveo-cloud-base-service/blob/917e3da63270ee25c19567d10126c5d53468246c/coveo-cloud-base-api/src/main/java/com/coveo/cloud/model/paging/PageModel.java#L14

The page extends functionality from PageModel
@louis-bompart

@louis-bompart
Copy link
Contributor

louis-bompart commented Mar 28, 2024

https://github.com/coveo/api-standards/blob/main/accepted/0002-offset-pagination.md. Modification to the pagination interface must follow this standard. Modifying the pagination standard is possible but must be done according to the procedure. It is strongly recommended that uncompliant services comply with the existing standard.
As a last resort, I'd like to invite you to add this distortion of the standard at your resource level.

It does not look like the PageModel provided by the base service complies with the guideline.

https://github.com/coveo-platform/coveo-cloud-base-service/blob/917e3da63270ee25c19567d10126c5d53468246c/coveo-cloud-base-api/src/main/java/com/coveo/cloud/model/paging/PageModel.java#L14

The page extends functionality from PageModel @louis-bompart

Indeed: Making the Java base service PageModel would've been a breaking change.
However, the rationale of my comment is that non-compliant elements of our API should be kept from leaking to the public.
There is a variety of ways to do so, be it by fixing the problem at the root (see https://github.com/coveo/api-standards/blob/main/accepted/0002-offset-pagination.md#aligning-existing-apis)
Moreover, there are other base services in the company, and I'm unsure if the PageModel of those exports to the public.
What I would advise is to check how other resources expose those parameters, and try to mimic that 🤔

@alanclarke alanclarke closed this Apr 3, 2024
@alanclarke alanclarke deleted the MHUB-626 branch April 3, 2024 15:46
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