Skip to content

API contract for OOO feature #249

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

API contract for OOO feature #249

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions requests/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,12 @@ Returns a list of requests with pagination and filtering options.
"state": "string",
"updatedAt": "number"
}
// Additional request objects
/* Additional request objects
//NOTE : In case of dev=true the response would be send as above but in case of dev=false :
message --> reason,
state --> status,
reason --> comment
*/
Comment on lines +73 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

There appears to be a field naming inconsistency in the API contract. The comment at lines 73-77 indicates that when dev=false, fields are mapped as:

message --> reason
state --> status
reason --> comment

However, in the field documentation at lines 141-142, reason and status are already defined as the primary field names for request creation.

To maintain a clear API contract, please:

  1. Clarify which field names are canonical in the API (either message/state or reason/status)
  2. Update both the comment and field documentation to use consistent terminology
  3. Consider adding a note about backward compatibility if both naming conventions need to be supported

This will help prevent confusion for developers implementing against this API.

Suggested change
/* Additional request objects
//NOTE : In case of dev=true the response would be send as above but in case of dev=false :
message --> reason,
state --> status,
reason --> comment
*/
/* Additional request objects
//NOTE : For backward compatibility, the API supports two field naming conventions:
Canonical field names: reason, status, comment
Alternative field names (when dev=true): message, state, reason
When dev=false, the alternative field names are mapped to their canonical equivalents.
*/

Spotted by Diamond

Fix in Graphite

Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +73 to +78
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Remove invalid JSON comment and avoid schema changes behind a dev flag.

  • The block comment inside the JSON sample makes the payload invalid.
  • Changing field names based on dev toggles is a breaking contract; pick a canonical schema (prefer reason, comment, status) and document any legacy aliases separately.

Apply this diff to keep the sample valid; move any notes outside the JSON code block:

-        /* Additional request objects
-        //NOTE : In case of dev=true the response would be send as above but in case of dev=false :
-        message --> reason,
-        state --> status,
-        reason --> comment
-        */

Follow-up (outside this range): add a Markdown “Notes” subsection clarifying the single, canonical response schema and any deprecated aliases.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/* Additional request objects
//NOTE : In case of dev=true the response would be send as above but in case of dev=false :
message --> reason,
state --> status,
reason --> comment
*/
🤖 Prompt for AI Agents 
In requests/README.md around lines 73 to 78, the JSON sample contains an inline
block comment which makes the payload invalid and documents conditional field
renames behind a dev flag; remove the comment from inside the JSON code block
and update the sample to use the single canonical schema (reason, comment,
status) so the example is valid JSON, then move the explanatory note about
legacy aliases and the dev flag into a separate "Notes" Markdown subsection
below the code block that documents the canonical field names and any deprecated
aliases (e.g., message→reason, state→status, reason→comment) rather than
changing the schema conditionally.

],
"next": "string",
"prev": "string"
Expand Down Expand Up @@ -133,8 +138,8 @@ Creates a new request.
- `type`: Required string to specify the type of request (e.g., OOO,EXTENSION,TASK).
- `from`: Required number to specify the start timestamp of the request.
- `until`: Required number to specify the end timestamp of the request.
- `message`: Required string to specify the message for the request.
- `state`: Required string to specify the state of the request (e.g., PENDING).
- `reason`: Required string to specify the reason for the request.
- `status`: Required string to specify the status of the request (e.g., PENDING).

Comment on lines +141 to 143
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Do not require client-supplied status on create; align POST body with canonical fields.

Creation should not accept status; the server sets it (typically PENDING). Keep reason only.

-  - `reason`: Required string to specify the reason for the request.
-  - `status`: Required string to specify the status of the request (e.g., PENDING).
+  - `reason`: Required string to specify the reason for the request.

Follow-up: update ALL request examples to stop using message/state and consistently use reason/comment/status where applicable.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- `reason`: Required string to specify the reason for the request.
- `status`: Required string to specify the status of the request (e.g., PENDING).
- `reason`: Required string to specify the reason for the request.
🤖 Prompt for AI Agents 
In requests/README.md around lines 141 to 143, the docs currently list
client-supplied `status` as required for create; remove `status` from the
POST/create field list so only `reason` is required and note that the server
sets `status` (e.g., PENDING). Update all request creation examples in the
README to omit `status` (and stop using `message/state`) and instead use the
canonical fields `reason` (and `comment` where applicable) and show `status`
only in server responses or later PUT/PATCH examples.

- Example OOO Request:
```json
Expand Down