feat: Add allowed config keys feature

[kofimokome]

Fixes #116

[sourceant]

Code Review Summary

✨ This pull request successfully refactors the application's configuration management by removing a custom ConfigurationController and its associated trait, and instead fully leveraging the whilesmart/eloquent-model-configuration package. This change streamlines the codebase, centralizes configuration logic, and aligns with best practices for using third-party packages. New feature tests validate the updated configuration API endpoints.

🚀 Key Improvements

• Reduced custom code and improved maintainability by adopting the whilesmart/eloquent-model-configuration package.
• Centralized and explicit allowed_keys configuration in config/model-configuration.php for better security and control.
• Comprehensive feature tests for configuration CRUD operations, ensuring the new setup works as expected.

💡 Minor Suggestions

• Add detailed inline comments to config/model-configuration.php to clarify the purpose of allowed_keys and the expectation for the custom App\Models\Configuration model.
• Review the necessity of client_id handling for configurations and ensure its functionality (validation and storage) is either replicated by the whilesmart package or no longer required.

🚨 Critical Issues

• Verify that App\Models\Configuration::class, as specified in config/model-configuration.php, correctly exists and extends the appropriate base model from the whilesmart/eloquent-model-configuration package. Incorrect setup here could lead to runtime errors or unexpected behavior.

[sourceant]

Review complete. See the overview comment for a summary.

[sourceant]

🔍 Code Review

💡 1. **app/Http/Controllers/API/v1/ConfigurationController.php** (Lines 26-37) - STYLE

The current implementation defines the $rules array and then conditionally modifies the 'key' rule. While functional, it's slightly more idiomatic and potentially clearer to build the 'key' rule string upfront before initializing the $rules array. This avoids modifying an array element after its initial definition, which can sometimes be less readable for complex rule constructions.

Suggested Code:

        $allowedKeys = $this->getAllowedConfigKeys();
        $keyRule = 'required';
        if (! empty($allowedKeys)) {
            $keyRule .= '|in:'.implode(',', $allowedKeys);
        }

        $rules = [
            'key' => $keyRule,
            'value' => 'required',
            'type' => 'required|in:string,int,float,bool,array,json,date',
            'client_id' => ['nullable', 'string', new ValidateClientId],
        ];
        $validator = Validator::make($request->all(), $rules);

Current Code:

        $rules = [
            'key' => 'required',
            'value' => 'required',
            'type' => 'required|in:string,int,float,bool,array,json,date',
            'client_id' => ['nullable', 'string', new ValidateClientId],
        ];

        $allowedKeys = $this->getAllowedConfigKeys();
        if (! empty($allowedKeys)) {
            $rules['key'] = 'required|in:'.implode(',', $allowedKeys);
        }
        $validator = Validator::make($request->all(), $rules);

💡 2. **app/Http/Controllers/API/v1/ConfigurationController.php** (Lines 93-95) - CLARITY

The current logic for client_id in the update method implicitly prevents changing an already-set client_id by only assigning it if ! $config->client_id. It's better practice to explicitly inform the user that a client_id cannot be modified once set, rather than silently ignoring the provided value. This improves API clarity and user experience by providing actionable feedback.

Suggested Code:

        if (isset($data['client_id'])) {
            if ($config->client_id && $config->client_id !== $data['client_id']) {
                return $this->failure('Client ID cannot be changed once set.', 400);
            }
            if (! $config->client_id) {
                $config->setClientGeneratedId($data['client_id'], $user);
            }
        }

Current Code:

        if (isset($data['client_id']) && ! $config->client_id) {
            $config->setClientGeneratedId($data['client_id'], $user);
        }

💡 3. **tests/Feature/ConfigurationTest.php** (Lines 70-77) - IMPROVEMENT

Add a test case to ensure that attempting to update a non-existent configuration key returns a 404 status. This covers an important negative scenario handled by the update method's if (! $configuration) block (lines 81-85 in the controller).

Suggested Code:

    public function test_api_user_can_not_update_non_existent_config()
    {
        $key = 'non-existent-key'; // Using a plausible key that isn't created
        $response = $this->actingAs($this->user)->putJson("/api/v1/configurations/{$key}", [
            'type' => 'string',
            'value' => 'some value',
        ]);
        $response->assertStatus(404);
        $response->assertJsonFragment(['message' => 'Configuration not found.']);
    }

Verdict: APPROVE

Posted as a comment because posting a review failed.

[nfebe]

This basically re-implements the feature which means our package is poorly designed.

This too : #114

Is problematic, packages are used to prevent these kind of things.

I will do some changes in the package.

[sourceant]

Review complete. See the overview comment for a summary.

[sourceant]

Review complete. No specific code suggestions were generated. See the overview comment for a summary.

[sourceant]

Review complete. See the overview comment for a summary.

[nfebe]

Made some update solving 1 critical issue and closing opportunities for mistakes.

For example you where still using version 1.0.4 of the package even though you created a 1.0.5 for this feature. The reason the test could not catch is because you had a complete rewrite of the package so it did not matter what version you are using.

All now fix, please review if you catch anything if not I will merge.

cc: @kofimokome

[kofimokome]

@nfebe configurations are syncable. So your recent changes have removed the syncable functionality

[kofimokome]

I actually updated to 1.0.5. Looks like I forgot to commit the composer.json file

[nfebe]

@nfebe configurations are syncable. So your recent changes have removed the syncable functionality

Not true.

[kofimokome]

@nfebe configurations are syncable. So your recent changes have removed the syncable functionality

Not true.

We still need to override the configuration controller. The endpoint to get Syncables has extra parameters like synced_since and no_client_id.

[nfebe]

We still need to override the configuration controller. The endpoint to get Syncables has extra parameters like synced_since and no_client_id.

Still not true.

We can append on the custom model you added.

Which was NOT modified and which is why you don't see it here.

[kofimokome]

I have tested with the recent changes, and the filtering is not working because the filters are applied at the controller level. using the applyApiQuery() method.
Example (from Wallets)

 public function index(Request $request): JsonResponse
    {
        $user = $request->user();
        $walletsQuery = $user->wallets();

        try {
            $data = $this->applyApiQuery($request, $walletsQuery);

            return $this->success($data);
        } catch (\InvalidArgumentException $e) {
            return $this->failure($e->getMessage(), 422);
        }
    }```

[kofimokome]

Or we can add a hook to the model controller. Trakli can use the hook to apply the filters 🤷

[nfebe]

I have tested with the recent changes, and the filtering is not working because the filters are applied at the controller level. using the applyApiQuery() method.
Example (from Wallets)

Yes this a geniune problem and it can be solved the way you mentioned. However, the core problem has been solved without needing to basically maintain a "fork" of the package here.

So please you can go ahead and implement that at the package level then add one final commit here! I am now much happier with this direction as opposed to the previous one.