Skip to content

Conversation

@HenrikHL
Copy link
Contributor

@HenrikHL HenrikHL commented Aug 14, 2025

User description

SD-2384: Remove mandatory requirement for shipmentCutOffTimes


PR Type

Enhancement


Description

  • Remove mandatory requirement for shipmentCutOffTimes field

  • Update booking confirmation schema to make cut-off times optional


Diagram Walkthrough

flowchart LR
  A["Booking Schema"] --> B["shipmentCutOffTimes Field"]
  B --> C["Remove Mandatory Condition"]
  C --> D["Optional Field"]
Loading

File Walkthrough

Relevant files
Enhancement
BKG_v2.0.0.yaml
Remove mandatory shipmentCutOffTimes requirement                 

bkg/v2/BKG_v2.0.0.yaml

  • Remove mandatory condition text for shipmentCutOffTimes field
  • Keep field description and structure intact
  • Make cut-off times optional for confirmed bookings
+0/-2     

@github-actions
Copy link

Error: The following file(s) are not allowed to be changed: bkg/v2/BKG_v2.0.0.yaml

@qodo-code-review
Copy link

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: check_changes

Failed stage: Check for forbidden changes [❌]

Failure summary:

The action failed due to a repository policy check in actions/github-script@v7 that blocks changes
to forbidden file paths. The script read patterns from .github/forbidden_changes.txt, compared them
against the changed files using regex, and detected a violation:
- Forbidden file changed:
bkg/v2/BKG_v2.0.0.yaml
The script then called core.setFailed(...), causing the GitHub Action to fail
with the error: "You are not allowed to change the following file(s): bkg/v2/BKG_v2.0.0.yaml".

Relevant error logs:
1:  ##[group]Runner Image Provisioner
2:  Hosted Compute Agent
...

111:  git switch -c <new-branch-name>
112:  Or undo this operation with:
113:  git switch -
114:  Turn off this advice by setting config variable advice.detachedHead to false
115:  HEAD is now at a946ec5 Merge 55ac62b4663e005b6a3887f1db539cf967088ec5 into 2d2e21a38abab7f3c7aa7b58a3a7d713dd62111d
116:  ##[endgroup]
117:  [command]/usr/bin/git log -1 --format=%H
118:  a946ec549283c4470f897f7fcaa1421b3b554a62
119:  ##[group]Run actions/github-script@v7
120:  with:
121:  script: const fs = require('fs');
122:  try {
123:    const forbiddenPaths = fs.readFileSync('.github/forbidden_changes.txt', 'utf8').split('\n').filter(path => path.trim() !== ''); // Read and filter empty lines
124:    console.log("Found valid lines (files): " + forbiddenPaths.length);
125:    return forbiddenPaths;
126:  } catch (error) {
127:    core.setFailed('Could not read forbidden paths file: ' + error.message);
128:    return [];
...

167:  const changedFiles = changedFilesString.split(',');
168:  
169:  let violations = [];
170:  
171:  changedFiles.forEach(file => {
172:    forbiddenPaths.forEach(forbiddenPath => {
173:      // console.log("Processing file: " + file + " with forbidden path: " + forbiddenPath);
174:      const regex = new RegExp(forbiddenPath); // Use regex for matching
175:      if (regex.test(file)) {
176:        violations.push(file);
177:      }
178:    });
179:  });
180:  
181:  if (violations.length > 0) {
182:    core.setFailed(`You are not allowed to change the following file(s): ${violations.join(', ')}`);
183:    // Create a comment on the PR
184:    await github.rest.issues.createComment({
185:      issue_number: context.issue.number,
186:      owner: context.repo.owner,
187:      repo: context.repo.repo,
188:      body: `**Error**: The following file(s) are not allowed to be changed: ${violations.join(', ')}`
189:    });
190:  }
191:  debug: false
192:  user-agent: actions/github-script
193:  result-encoding: json
194:  retries: 0
195:  retry-exempt-status-codes: 400,401,403,404,422
196:  ##[endgroup]
197:  ##[error]You are not allowed to change the following file(s): bkg/v2/BKG_v2.0.0.yaml
198:  Post job cleanup.

@qodo-code-review
Copy link

qodo-code-review bot commented Aug 14, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

SD-2384 - Partially compliant

Compliant requirements:

  • Remove mandatory requirement text for shipmentCutOffTimes in booking confirmation schema.
  • Preserve description and structure for shipmentCutOffTimes while making it optional.

Non-compliant requirements:

  • Align all stated bookingStatus values (CONFIRMED/PENDING_AMENDMENT/COMPLETED/DECLINED) rule enforcement across the API (confirmedEquipments and transportPlan must be present) – not verifiable/implemented in the shown diff.

Requires further human verification:

  • Confirm that validation logic beyond the schema (e.g., request/response validators, service-level checks) enforces presence of confirmedEquipments and transportPlan for the specified bookingStatus values.
  • Verify that other API documentation and examples were updated to reflect shipmentCutOffTimes being optional.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incomplete Rule Alignment

The ticket broadens the condition across multiple booking statuses and fields. The diff only removes the mandatory note for shipmentCutOffTimes and adds a condition for Transport being mandatory for CONFIRMED. Verify that validations for PENDING_AMENDMENT/COMPLETED/DECLINED and confirmedEquipments/transportPlan are updated elsewhere.

    **Condition:** Mandatory and non-empty for a `CONFIRMED` Booking
  items:
    $ref: '#/components/schemas/Transport'
shipmentCutOffTimes:
  type: array
  description: |
    A list of cut-off times provided by the carrier in the booking confirmation. A cut-off time indicates the latest deadline within which a task must be completed. The confirmed schedule cannot be guaranteed if a cut-off time is missed. Customs brokers may set additional cut-off times to receive the export customs documentation, which is not included in the shipment cut-off times of a carrier booking.
Documentation Consistency

Ensure all references to shipmentCutOffTimes mandatory status are removed or updated across the spec (schemas, examples, descriptions) to avoid conflicting guidance.

shipmentCutOffTimes:
  type: array
  description: |
    A list of cut-off times provided by the carrier in the booking confirmation. A cut-off time indicates the latest deadline within which a task must be completed. The confirmed schedule cannot be guaranteed if a cut-off time is missed. Customs brokers may set additional cut-off times to receive the export customs documentation, which is not included in the shipment cut-off times of a carrier booking.
  items:
    $ref: '#/components/schemas/ShipmentCutOffTime'
advanceManifestFilings:

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Clarify optionality and validation

With the mandatory condition removed, add explicit minItems and nullable
semantics to avoid ambiguous validation. Clarify in the description that the
array may be absent or empty to align clients and servers.

bkg/v2/BKG_v2.0.0.yaml [3855-3858]

 description: |
   A list of cut-off times provided by the carrier in the booking confirmation. A cut-off time indicates the latest deadline within which a task must be completed. The confirmed schedule cannot be guaranteed if a cut-off time is missed. Customs brokers may set additional cut-off times to receive the export customs documentation, which is not included in the shipment cut-off times of a carrier booking.
+  
+  This field is optional for `CONFIRMED` bookings and may be omitted or an empty array if no cut-off times apply.
+minItems: 0
+nullable: true
 items:
   $ref: '#/components/schemas/ShipmentCutOffTime'

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly proposes adding explicit, machine-readable validation (minItems: 0, nullable: true) to reflect the removal of the human-readable mandatory condition, improving schema clarity and robustness.

Medium
  • More

@HenrikHL
Copy link
Contributor Author

Modifying wrong version

@HenrikHL HenrikHL closed this Aug 14, 2025
@HenrikHL HenrikHL deleted the SD-2384_Remove-cutOffTime-requirements branch August 19, 2025 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants