feat: implement setShipFavouriteLoadout.php #662

Sainan · 2024-12-29T15:48:38-08:00

Sainan commented

2024-12-29 15:48:38 -08:00

Closes #649

coderabbitai[bot] commented

2024-12-29 15:48:44 -08:00

(Migrated from github.com)

Walkthrough

A new controller setShipFavouriteLoadoutController has been implemented in the API to handle setting a favorite ship loadout. The controller is designed to process requests with a specific payload including BootLocation, FavouriteLoadoutId, and TagName. When a request is received, the controller retrieves the user's account ID, validates the boot location is set to "SHOP", and then updates or creates a favorite loadout entry in the user's personal rooms. The implementation includes input validation and ensures that the loadout is associated with the correct tag name. A corresponding route has been added to the API router to enable this new functionality.

Assessment against linked issues

Objective	Addressed	Explanation
Resolve unknown endpoint for POST /api/setShipFavouriteLoadout.php [#649]	✅
Support setting favorite loadout with specific payload structure	✅
Handle requests with BootLocation="SHOP"	✅

The changes directly address the issue by:

Adding the missing endpoint for /setShipFavouriteLoadout.php
Implementing a controller that can process the specific payload structure
Validating the BootLocation is set to "SHOP"
Providing a mechanism to set and update favorite loadouts

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

## Walkthrough A new controller `setShipFavouriteLoadoutController` has been implemented in the API to handle setting a favorite ship loadout. The controller is designed to process requests with a specific payload including `BootLocation`, `FavouriteLoadoutId`, and `TagName`. When a request is received, the controller retrieves the user's account ID, validates the boot location is set to "SHOP", and then updates or creates a favorite loadout entry in the user's personal rooms. The implementation includes input validation and ensures that the loadout is associated with the correct tag name. A corresponding route has been added to the API router to enable this new functionality. ## Assessment against linked issues | Objective | Addressed | Explanation | |-----------|-----------|-------------| | Resolve unknown endpoint for POST /api/setShipFavouriteLoadout.php [#649] | ✅ | | | Support setting favorite loadout with specific payload structure | ✅ | | | Handle requests with BootLocation="SHOP" | ✅ | | The changes directly address the issue by: 1. Adding the missing endpoint for `/setShipFavouriteLoadout.php` 2. Implementing a controller that can process the specific payload structure 3. Validating the `BootLocation` is set to "SHOP" 4. Providing a mechanism to set and update favorite loadouts      --- Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? <details> <summary>❤️ Share</summary> - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code) </details> <details> <summary>🪧 Tips</summary> ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit <commit_id>, please review it.` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` - `@coderabbitai help me debug CodeRabbit configuration file.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (Invoked using PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to do a full review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai generate docstrings` to [generate docstrings](https://docs.coderabbit.ai/finishing-touches/docstrings) for this PR. ([Beta](https://docs.coderabbit.ai/early-access#beta)) - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository. - `@coderabbitai help` to get help. ### Other keywords and placeholders - Add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. - Add `@coderabbitai summary` to generate the high-level summary at a specific location in the PR description. - Add `@coderabbitai` anywhere in the PR title to generate the title automatically. ### Documentation and Community - Visit our [Documentation](https://docs.coderabbit.ai) for detailed information on how to use CodeRabbit. - Join our [Discord Community](http://discord.gg/coderabbit) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements. </details>

coderabbitai[bot] (Migrated from github.com) reviewed 2024-12-29 15:50:40 -08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 1

🧹 Nitpick comments (5)

src/controllers/api/setShipFavouriteLoadoutController.ts (4)
7-7: Ensure route-level or higher-level authentication & rate limiting.

While this controller retrieves account information internally, it remains crucial to confirm that there's an authentication or authorization layer. Consider adding (or verifying) any rate-limiting or additional checks needed to avoid potential DDoS or brute-force requests for frequent loadout updates.

10-10: Suggest safer parsing or fallback approach.

Relying on JSON.parse(String(req.body)) is functional for typical usage, but it lacks robust error handling for malformed JSON requests. Consider adding a try/catch or using a body-parser middleware with error handling to ensure the request parsing doesn’t throw an unhandled exception.
- const body = JSON.parse(String(req.body)) as ISetShipFavouriteLoadoutRequest;
+ let parsedBody: unknown;
+ try {
+   parsedBody = JSON.parse(String(req.body));
+ } catch (err) {
+   return res.status(400).json({ error: "Invalid JSON payload" });
+ }
+ const body = parsedBody as ISetShipFavouriteLoadoutRequest;
23-25: Consider adding more robust success feedback.

Returning an empty object is acceptable, but you might want to add an "ok": true property or similar to the response. This can help the client confidently confirm that the update succeeded rather than receiving an empty JSON object.
- res.json({});
+ res.json({ ok: true });
27-31: Validate the naming convention and tighten type definitions.

ISetShipFavouriteLoadoutRequest is descriptive. Verify if the uppercase property names (“BootLocation”, “TagName”...) are consistent across the codebase. Also consider adding an enum for BootLocation if only “SHOP” is valid to ensure type safety.
src/routes/api.ts (1)

57-57: Check for consistent import location and grouping.

All routes are imported at the top. The addition of setShipFavouriteLoadoutController is consistent. If you wish to group controllers thematically, consider reorganizing this file’s imports by domain (like “ship-related controllers”).

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e41022f176 and bddfdb9dc7.

📒 Files selected for processing (2)

src/controllers/api/setShipFavouriteLoadoutController.ts (1 hunks)
src/routes/api.ts (2 hunks)

🔇 Additional comments (4)

src/controllers/api/setShipFavouriteLoadoutController.ts (3)

1-6: Use named imports properly and consider relevant error handling.

Currently, the imports appropriately bring in the needed functionalities, but you may want to verify whether exceptions thrown by getAccountIdForRequest or getPersonalRooms are properly handled at a higher level in your middleware. If these functions can reject or throw, ensure they are caught and appropriately responded to.

14-22: Confirm that only one loadout per tag is needed.

Currently, the code searches the FavouriteLoadouts array for a single match and either updates it or pushes a new entry. If multiple items share the same tag, only the first one is updated. Verify that this is consistent with business rules (unique tags) or handle duplicates gracefully.

8-9: Validate that personalRooms is properly initialized.

Before accessing nested properties like personalRooms.TailorShop.FavouriteLoadouts, ensure personalRooms, TailorShop, and FavouriteLoadouts exist. If the user has no TailorShop or related property from the DB, this may cause runtime errors.

✅ Verification successful

personalRooms is safely initialized with proper error handling

Based on the verification results, the getPersonalRooms service properly handles initialization and error cases:

In personalRoomsService.ts, if no personal rooms are found for the account, it throws an error with a clear message
The TailorShop property is guaranteed to exist as it's defined in the schema with default values:
- tailorShopDefault in personalRoomsModel.ts initializes with empty FavouriteLoadouts array and other required properties
- The schema enforces the structure through tailorShopSchema

The code is safe as it will either:

Return a fully initialized personal rooms object with default values
Throw an error if the account's personal rooms don't exist

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching places where personalRooms.TailorShop might be null or undefined. 
# We'll also check how getPersonalRooms is defined or used.
rg -A 5 "getPersonalRooms"
rg -A 5 "TailorShop"

Length of output: 12718

src/routes/api.ts (1)

138-138: Route naming and consistency.

The route name /setShipFavouriteLoadout.php is consistent with existing patterns. Ensure that the .php extension is intentional and that all relevant documentation references this route accurately.

**Actionable comments posted: 1** <details> <summary>🧹 Nitpick comments (5)</summary><blockquote> <details> <summary>src/controllers/api/setShipFavouriteLoadoutController.ts (4)</summary><blockquote> `7-7`: **Ensure route-level or higher-level authentication & rate limiting.** While this controller retrieves account information internally, it remains crucial to confirm that there's an authentication or authorization layer. Consider adding (or verifying) any rate-limiting or additional checks needed to avoid potential DDoS or brute-force requests for frequent loadout updates. --- `10-10`: **Suggest safer parsing or fallback approach.** Relying on `JSON.parse(String(req.body))` is functional for typical usage, but it lacks robust error handling for malformed JSON requests. Consider adding a try/catch or using a body-parser middleware with error handling to ensure the request parsing doesn’t throw an unhandled exception. ```diff - const body = JSON.parse(String(req.body)) as ISetShipFavouriteLoadoutRequest; + let parsedBody: unknown; + try { + parsedBody = JSON.parse(String(req.body)); + } catch (err) { + return res.status(400).json({ error: "Invalid JSON payload" }); + } + const body = parsedBody as ISetShipFavouriteLoadoutRequest; ``` --- `23-25`: **Consider adding more robust success feedback.** Returning an empty object is acceptable, but you might want to add an `"ok": true` property or similar to the response. This can help the client confidently confirm that the update succeeded rather than receiving an empty JSON object. ```diff - res.json({}); + res.json({ ok: true }); ``` --- `27-31`: **Validate the naming convention and tighten type definitions.** `ISetShipFavouriteLoadoutRequest` is descriptive. Verify if the uppercase property names (“BootLocation”, “TagName”...) are consistent across the codebase. Also consider adding an enum for `BootLocation` if only “SHOP” is valid to ensure type safety. </blockquote></details> <details> <summary>src/routes/api.ts (1)</summary><blockquote> `57-57`: **Check for consistent import location and grouping.** All routes are imported at the top. The addition of `setShipFavouriteLoadoutController` is consistent. If you wish to group controllers thematically, consider reorganizing this file’s imports by domain (like “ship-related controllers”). </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e41022f176a42b508df742eccfede187a44918e9 and bddfdb9dc70f7a14e588fe93c62a16c77b20c75d. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `src/controllers/api/setShipFavouriteLoadoutController.ts` (1 hunks) * `src/routes/api.ts` (2 hunks) </details> <details> <summary>🔇 Additional comments (4)</summary> <details> <summary>src/controllers/api/setShipFavouriteLoadoutController.ts (3)</summary> `1-6`: **Use named imports properly and consider relevant error handling.** Currently, the imports appropriately bring in the needed functionalities, but you may want to verify whether exceptions thrown by `getAccountIdForRequest` or `getPersonalRooms` are properly handled at a higher level in your middleware. If these functions can reject or throw, ensure they are caught and appropriately responded to. --- `14-22`: **Confirm that only one loadout per tag is needed.** Currently, the code searches the `FavouriteLoadouts` array for a single match and either updates it or pushes a new entry. If multiple items share the same tag, only the first one is updated. Verify that this is consistent with business rules (unique tags) or handle duplicates gracefully. --- `8-9`: **Validate that `personalRooms` is properly initialized.** Before accessing nested properties like `personalRooms.TailorShop.FavouriteLoadouts`, ensure `personalRooms`, `TailorShop`, and `FavouriteLoadouts` exist. If the user has no TailorShop or related property from the DB, this may cause runtime errors. <details> <summary>✅ Verification successful</summary> **`personalRooms` is safely initialized with proper error handling** Based on the verification results, the `getPersonalRooms` service properly handles initialization and error cases: - In `personalRoomsService.ts`, if no personal rooms are found for the account, it throws an error with a clear message - The `TailorShop` property is guaranteed to exist as it's defined in the schema with default values: - `tailorShopDefault` in `personalRoomsModel.ts` initializes with empty `FavouriteLoadouts` array and other required properties - The schema enforces the structure through `tailorShopSchema` The code is safe as it will either: - Return a fully initialized personal rooms object with default values - Throw an error if the account's personal rooms don't exist </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Searching places where personalRooms.TailorShop might be null or undefined. # We'll also check how getPersonalRooms is defined or used. rg -A 5 "getPersonalRooms" rg -A 5 "TailorShop" ``` Length of output: 12718 </details> </details> <details> <summary>src/routes/api.ts (1)</summary> `138-138`: **Route naming and consistency.** The route name `/setShipFavouriteLoadout.php` is consistent with existing patterns. Ensure that the `.php` extension is intentional and that all relevant documentation references this route accurately. </details> </details> </details>

src/controllers/api/setShipFavouriteLoadoutController.ts

						
				@ -0,0 +10,4 @@

				    const body = JSON.parse(String(req.body)) as ISetShipFavouriteLoadoutRequest;

				    if (body.BootLocation != "SHOP") {

				        throw new Error(`unexpected BootLocation: ${body.BootLocation}`);

				    }

coderabbitai[bot] (Migrated from github.com) commented

2024-12-29 15:50:39 -08:00

🛠️ Refactor suggestion

Return a proper response instead of throwing an error.

Throwing an error can break the request pipeline and produce a 500 internal server error. Instead, return a suitable status code (e.g., 400) and problem detail JSON, clarifying why the request was rejected.

- if (body.BootLocation != "SHOP") {
-   throw new Error(`unexpected BootLocation: ${body.BootLocation}`);
- }
+ if (body.BootLocation !== "SHOP") {
+   return res.status(400).json({ error: `unexpected BootLocation: ${body.BootLocation}` });
+ }

📝 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.

    if (body.BootLocation !== "SHOP") {
        return res.status(400).json({ error: `unexpected BootLocation: ${body.BootLocation}` });
    }

_:hammer_and_wrench: Refactor suggestion_ **Return a proper response instead of throwing an error.** Throwing an error can break the request pipeline and produce a 500 internal server error. Instead, return a suitable status code (e.g., 400) and problem detail JSON, clarifying why the request was rejected. ```diff - if (body.BootLocation != "SHOP") { - throw new Error(`unexpected BootLocation: ${body.BootLocation}`); - } + if (body.BootLocation !== "SHOP") { + return res.status(400).json({ error: `unexpected BootLocation: ${body.BootLocation}` }); + } ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. `````suggestion if (body.BootLocation !== "SHOP") { return res.status(400).json({ error: `unexpected BootLocation: ${body.BootLocation}` }); } ````` </details>

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: OpenWF/SpaceNinjaServer#662