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.

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

Jump to bottom

[FEATURE] ajoute le fitre lacune dans la route /assessment-results (pix-16350) #11392

Merged
merged 4 commits into from
Feb 17, 2025
Merged

[FEATURE] ajoute le fitre lacune dans la route /assessment-results (pix-16350) #11392

merged 4 commits into from
Feb 17, 2025

Conversation

lionelB
Copy link
Member

@lionelB lionelB commented Feb 11, 2025 •
edited
Loading

🥞 Problème

Dans le cadre de l'amélioration de la page résultat, on aimerait permettre de filtrer les participations qui n'ont pas obtenu un certain badge

🥓 Proposition

Ajouter un filtre supplémentaire unacquiredBadges pour filter les participations qui n'ont pas acquis un badge

🧃 Remarques

😋 Pour tester

  • récupérer un jwt token depuis orga
    faire un curl
# A modifier
ACCESS_TOKEN=$(curl -s 'https://pix-api-review-pr11392.osc-fr1.scalingo.io/api/token' --data-raw 'grant_type=password&username=admin-orga%40example.net&password=pix123&scope=pix-orga' | jq -r .access_token);
curl 'https://orga-pr11392.review.pix.fr/api/campaigns/104882/assessment-results?filter%5BunacquiredBadges%5D%5B%5D=6000&filter%5BunacquiredBadges%5D%5B%5D=6001&filter%5Bsearch%5D=&page%5Bnumber%5D=&page%5Bsize%5D=50' -H 'Accept: a
pplication/vnd.api+json' -H "Authorization: Bearer $ACCESS_TOKEN" | jq '.data'

et valider qu'on a bien les 2 participations sans badge

@lionelB lionelB requested a review from a team as a code owner February 11, 2025 13:32
@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@lionelB lionelB force-pushed the pix-16350/add-unaquired-badge-api branch 3 times, most recently from 7b2b79f to cdfb648 Compare February 11, 2025 15:57
Alexandre-Monney
Alexandre-Monney approved these changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@Alexandre-Monney Alexandre-Monney left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tech Ok 👍 , rien de bien bloquant en commentaires

...aign/infrastructure/repositories/campaign-assessment-participation-result-list-repository.js Outdated
@@ -32,7 +32,8 @@ function _getParticipantsResultList(campaignId, stageCollection, filters) {
.with('campaign_participation_summaries', (qb) => _getParticipations(qb, campaignId, stageCollection, filters))
.select('*')
.from('campaign_participation_summaries')
.modify(_filterByBadgeAcquisitionsOut, filters)
.modify(_filterByacquiredBadges, filters)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.modify(_filterByacquiredBadges, filters)
.modify(_filterByAcquiredBadges, filters)

...aign/infrastructure/repositories/campaign-assessment-participation-result-list-repository.js Outdated Show resolved Hide resolved
...infrastructure/repositories/campaign-assessment-participation-result-list-repository_test.js Outdated Show resolved Hide resolved
...infrastructure/repositories/campaign-assessment-participation-result-list-repository_test.js Outdated Show resolved Hide resolved
...infrastructure/repositories/campaign-assessment-participation-result-list-repository_test.js Outdated
Comment on lines 1092 to 1095
const participantExternalIds = participations.map((result) => result.participantExternalId);

// then
expect(participantExternalIds).to.exactlyContain([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const participantExternalIds = participations.map((result) => result.participantExternalId);
// then
expect(participantExternalIds).to.exactlyContain([]);
// then
expect(participations).to.be.empty

...infrastructure/repositories/campaign-assessment-participation-result-list-repository_test.js
// when
const { participations } = await campaignAssessmentParticipationResultListRepository.findPaginatedByCampaignId({
campaignId: campaign.id,
filters: { badges: [badge1.id], unacquiredBadges: [badge1.id] },
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion : Si je me réfère au 'it' c'est unacquiredBadges: [badge2.id] ou alors je comprends pas le test 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

l'intitulé du it n'est pas bon. Le but est d'expliciter le cas ou l'on passe le même id dans badges/unacquiredBadges

api/tests/prescription/campaign/unit/application/campaign-results-controller_test.js
@@ -64,4 +64,52 @@ describe('Unit | Application | Controller | Campaign Results', function () {
expect(errorCatched).to.be.instanceof(UserNotAuthorizedToAccessEntityError);
});
});

describe('#findAssessmentParticipationResults', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick : A sortir de ce commit ?

@Alexandre-Monney Alexandre-Monney added Tech Review OK 👀 Func Review Needed Need PO validation for this functionally and removed Development in progress labels Feb 13, 2025
@alicegoarnisson alicegoarnisson added Func Review OK PO validated functionally the PR and removed 👀 Func Review Needed Need PO validation for this functionally labels Feb 14, 2025
@alicegoarnisson
Copy link
Contributor

Func OK : on a bien les deux participations avec un tableau vide chacun pour les data des badges 🦦

alicegoarnisson
alicegoarnisson reviewed Feb 14, 2025
View reviewed changes
api/src/prescription/campaign/domain/errors.js
);
}
}

export {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion : pour rendre l'erreur plus explicite (parce qu'en soi, c'est possible de filtrer à la fois sur acquired et unacquired, juste pas avec le même id), préciser "Filtering on both acquired and unacquired for the same badge is impossible"

@lionelB lionelB force-pushed the pix-16350/add-unaquired-badge-api branch 2 times, most recently from afb19b3 to 375a8df Compare February 17, 2025 10:30
@pix-service-auto-merge pix-service-auto-merge force-pushed the pix-16350/add-unaquired-badge-api branch from 375a8df to 96318d9 Compare February 17, 2025 14:18
@pix-service-auto-merge pix-service-auto-merge merged commit 9f9eb0f into dev Feb 17, 2025
8 of 11 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the pix-16350/add-unaquired-badge-api branch February 17, 2025 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alicegoarnisson alicegoarnisson alicegoarnisson left review comments

@La-toile-cosmique La-toile-cosmique La-toile-cosmique approved these changes

@Alexandre-Monney Alexandre-Monney Alexandre-Monney approved these changes

Assignees

@lionelB lionelB

@La-toile-cosmique La-toile-cosmique

Labels
Func Review OK PO validated functionally the PR 🚀 Ready to Merge team-prescription Tech Review OK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lionelB @pix-bot-github @alicegoarnisson @La-toile-cosmique @Alexandre-Monney @pix-service-auto-merge