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

[BE] Feature/ita profiles collaborators #106 and #107 #111

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

brycoti
Copy link
Collaborator

@brycoti brycoti commented Jan 8, 2024

Pull Request Description #106

Proposed Changes

This PR introduces several modifications in CollaboratorsController.php to enhance collaborator management based on different areas.

Change Details

  1. Refactored conditions in the index method to improve code readability and maintenance.
  2. Added comments indicating the need for assistance with the provided token and the use of reverse engineering.

here's an example function to test a scenario where the provided token does not have the necessary permissions:

public function getUsers()
{
// Define la URL de la API de GitHub
$url = 'https://api.github.com/';

// Define la ruta para obtener colaboradores del repositorio
$repositoryPath = '/repos/AnUser/testingApi/collaborators';

// Construye la URL completa
$fullUrl = $url . $repositoryPath;

// Realiza la solicitud a la API de GitHub
$response = Http::withToken(env('MY_TOKEN'))->get($fullUrl);

// Verifica si la solicitud fue exitosa
if ($response->successful()) {
    // Obtiene los datos de la respuesta JSON
    $data = $response->json();

    // Procesa los datos y devuelve la información requerida
    $users = [];
    foreach ($data as $collaborator) {
        $users[] = [
            'name' => $collaborator['login'],
            'photo' => $collaborator['avatar_url'],
            'url' => $collaborator['html_url'],
        ];
    }

    return response()->json($users);
} else {

    return response()->json(['error' => 'Error al obtener colaboradores'], $response->status());
}

}

   Route::get('/get-users', [CollaboratorsController::class, 'getUsers']);     http://127.0.0.1:8000/api/get-users  

Affected Areas

  1. Wiki: Updated collaborator retrieval for the wiki.
  2. Profiles: Enhanced collaborator retrieval for both frontend and backend parts.

Testing

Tests have been added in the CollaboratorTest.php file to ensure that the changes function correctly.

Pull Request Description [#107 ]

  1. Swagger doc: Updated collaborator parameters with an enum list and 404 error.

@brycoti brycoti requested a review from FranEnLaNube January 8, 2024 12:12
@brycoti brycoti changed the title Feature/ita profiles collaborators [BE] Feature/ita profiles collaborators #106 Jan 9, 2024
@brycoti brycoti changed the title [BE] Feature/ita profiles collaborators #106 [BE] Feature/ita profiles collaborators ##106 Jan 9, 2024
@brycoti brycoti changed the title [BE] Feature/ita profiles collaborators ##106 [BE] Feature/ita profiles collaborators #106 Jan 9, 2024
@brycoti brycoti self-assigned this Jan 10, 2024
@brycoti brycoti linked an issue Jan 16, 2024 that may be closed by this pull request
@brycoti brycoti linked an issue Jan 16, 2024 that may be closed by this pull request
Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@brycoti brycoti linked an issue Jan 17, 2024 that may be closed by this pull request
@brycoti brycoti changed the title [BE] Feature/ita profiles collaborators #106 [BE] Feature/ita profiles collaborators #106 and #107 Jan 17, 2024
Copy link
Collaborator

@FranEnLaNube FranEnLaNube left a comment

Choose a reason for hiding this comment

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

Really good job @brycoti , especially approaching the difficulties with the GitHub token and making the reverse engineering to check the endpoints!
I've made my comments in the files.

@@ -60,7 +60,6 @@ public function uniqueCollaborators(...$arrays)

public function collaboratorLanding()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function collaboratorLanding()
public function collaboratorLanding()
{
$collaboratorPhp = '/ita-landing-backend/collaborators?affiliation=direct';
$collaboratorReact = '/ita-landing-frontend/collaborators?affiliation=direct';
$php = $this->collaboratorLogic($collaboratorPhp);
$react = $this->collaboratorLogic($collaboratorReact);
// Return unique (not repeated) collaborators from php and react projects.
return array_unique(array_merge($php, $react));
}

@@ -40,7 +41,6 @@ public function collaboratorLogic($collaborator)
}

return $allCollaborators;

}

public function uniqueCollaborators(...$arrays)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this function it's not needed by aplicate the change suggested at L61. And that change can be applied at the rest of methods

return $this->collaboratorLogic($collaboratorWiki);
$wiki = $this->collaboratorLogic($collaboratorWiki);

$uniqueCollaborators = $this->uniqueCollaborators($wiki);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Applying the L61 suggestion we also avoid the following SonarCloud message:
"Immediately return this expression instead of assigning it to the temporary variable "$uniqueCollaborators"."

@@ -60,7 +60,6 @@ public function uniqueCollaborators(...$arrays)

public function collaboratorLanding()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to change the method name if suggestion at index() is accepted.

Suggested change
public function collaboratorLanding()
public function collaboratorItaLanding()

@@ -24,7 +26,6 @@ public function index($area)

public function collaboratorLogic($collaborator)
{

$url = env('URL_SERVER_API', 'https://api.github.com');
$response = Http::withToken(env('MY_TOKEN'))->get($url.$collaborator);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the token ba passed by a query parameter? I reckon frontend users won't be able to edit .env file.

@@ -15,6 +15,8 @@ public function index($area)
return $this->collaboratorItaWiki();
} elseif ($area === 'challenges') {
return $this->collaboratorItaChallenges();
} elseif ($area === 'profiles') {
return $this->collaboratorItaProfiles();
}

return response()->json([
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've found another way to approach this:

Suggested change
return response()->json([
$projects = ['landing', 'wiki', 'challenges', 'profiles'];
if (!in_array($area, $projects)) {
return response()->json([
'message' => 'this area is invalid',
], 404);
} else {
$methodName = 'collaboratorIta' . ucfirst($area);
return $this->{$methodName}();
}```
I can't add the suggestion at L10. IDK way.

Copy link
Collaborator

@FranEnLaNube FranEnLaNube Jan 17, 2024

Choose a reason for hiding this comment

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

If CollaboratorController::collaboratorLanding() is changed as suggested, we may don't forget to change it here at L57

@@ -9,20 +9,26 @@ class AnnotationsCollaborators
* path="/collaborators/{area}",
* tags={"Collaborators"},
* summary="User Collaborators",
* description="This endpoint is used to get persons that work on the specific project",
* description="This endpoint is used to get persons that work on the specific project,
* URL parameters: area (landing, wiki, challenges, profiles).",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is not in the next line at Swagger, I mean, it's all togheter with an asterisc in the middle, at least in my browser.I've tried to fix it but I couldn't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't check these tests due the well-known problem with the token of the IT-Academy repository.
Perhaps could be a good idea to approach this by using mocking.

@brycoti brycoti assigned pupadevs and unassigned pupadevs Jan 18, 2024
@brycoti brycoti requested a review from pupadevs January 18, 2024 17:38
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.

[BE] Update collaborators Swagger
3 participants