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/dat 571 se brancher avec lapi jcop #469

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

Feature/dat 571 se brancher avec lapi jcop #469

wants to merge 13 commits into from

Conversation

Samuelfaure
Copy link
Contributor

@Samuelfaure Samuelfaure commented Oct 4, 2024 •
edited
Loading

Vues à faire, mais dans une autre PR

Je sais pas trop qui tag en review donc je tag tout le monde mais ne vous sentez pas obligés x)

@linear Linear
Copy link

linear bot commented Oct 4, 2024

DAT-571 Se brancher avec l'API JCOP

skelz0r
skelz0r reviewed Oct 4, 2024
View reviewed changes
app/models/malware_scan.rb Outdated Show resolved Hide resolved
@skelz0r
Copy link
Member

skelz0r commented Oct 4, 2024

En diagonale ça me semble clair.

Gros point ici #469 (comment) => on veut analyser tous les documents, donc autant s'attacher directement aux ActiveStorage models (surtout que AuthorizationDocument n'est pas celui des requêtes)

@Samuelfaure Samuelfaure force-pushed the feature/dat-571-se-brancher-avec-lapi-jcop branch 4 times, most recently from 91aeb14 to bbb00f4 Compare October 11, 2024 11:45
@Samuelfaure Samuelfaure marked this pull request as ready for review October 11, 2024 11:45
Isalafont
Isalafont reviewed Oct 14, 2024
View reviewed changes
spec/factories/active_storage_blob.rb Outdated Show resolved Hide resolved
app/organizers/update_authorization_request.rb Outdated
@@ -7,7 +7,8 @@ class UpdateAuthorizationRequest < ApplicationOrganizer
organize AssignParamsToAuthorizationRequest,
VerifyContactsEmailsAsynchronously,
CreateAuthorizationRequestEventModel,
DeliverAuthorizationRequestNotification
DeliverAuthorizationRequestNotification,
RunMalwareScanOnAttachments
Copy link
Contributor

Choose a reason for hiding this comment

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

Je pense que ce n'est pas nécéssaire de lancer RunMalwareScanOnAttachments pour le update car l'user doit de toute façon "submit" ses modifications pour que l'instructeur puisse accéder aux modifications effectuées.

@Isalafont
Copy link
Contributor

Super PR !

JeSuisUnCaillou
JeSuisUnCaillou requested changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@JeSuisUnCaillou JeSuisUnCaillou left a comment

Choose a reason for hiding this comment

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

Plutôt stylé.

Il manque juste une gestion claire des scans pas finis je trouve.

app/models/malware_scan.rb Outdated Show resolved Hide resolved
app/jobs/malware_scan_job.rb Show resolved Hide resolved
app/models/concerns/active_storage_attachment_extension.rb Show resolved Hide resolved
app/services/je_clique_ou_pas_api_client.rb Show resolved Hide resolved
app/jobs/malware_scan_retrieve_state_job.rb Outdated Show resolved Hide resolved
app/jobs/malware_scan_retrieve_state_job.rb Outdated Show resolved Hide resolved
spec/jobs/malware_scan_retrieve_state_job_spec.rb Show resolved Hide resolved
spec/jobs/malware_scan_retrieve_state_job_spec.rb Outdated Show resolved Hide resolved
spec/jobs/deliver_authorization_request_webhook_job_spec.rb Show resolved Hide resolved
@Samuelfaure Samuelfaure force-pushed the feature/dat-571-se-brancher-avec-lapi-jcop branch from bbb00f4 to 15775b0 Compare October 16, 2024 11:23
@Samuelfaure Samuelfaure force-pushed the feature/dat-571-se-brancher-avec-lapi-jcop branch from 15775b0 to f7c40c8 Compare October 16, 2024 14:21
JeSuisUnCaillou
JeSuisUnCaillou approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@JeSuisUnCaillou JeSuisUnCaillou left a comment

Choose a reason for hiding this comment

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

Juste deux dernières remarques, mais je valide pour que tu puisses merge une fois que tu les aura traitées, je pense que c'est pas bloquant non plus.

app/models/malware_scan.rb Outdated Show resolved Hide resolved
spec/jobs/malware_scan_retrieve_state_job_spec.rb Outdated Show resolved Hide resolved
@Samuelfaure Samuelfaure force-pushed the feature/dat-571-se-brancher-avec-lapi-jcop branch from f7c40c8 to df7c515 Compare October 17, 2024 07:05
@JeSuisUnCaillou
Copy link
Contributor

Note : Ne pas oublier de mettre les credentials jcop en sandbox aussi

@Samuelfaure
Copy link
Contributor Author

Bien vu

@Samuelfaure Samuelfaure force-pushed the feature/dat-571-se-brancher-avec-lapi-jcop branch from df7c515 to 4d53651 Compare October 17, 2024 12:47
@Samuelfaure Samuelfaure force-pushed the feature/dat-571-se-brancher-avec-lapi-jcop branch from 93d6300 to b545edb Compare October 18, 2024 06:48
@Samuelfaure
Copy link
Contributor Author

re-requesting pour les derniers commits (8c78ea9 et suivants)

@JeSuisUnCaillou
Copy link
Contributor

Tu pourrais joindre quelques screenshots pour qu'on puisse voir l'UI sans avoir à run le malware scan ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skelz0r skelz0r Awaiting requested review from skelz0r

@jbfeldis jbfeldis Awaiting requested review from jbfeldis

@SchweisguthN SchweisguthN Awaiting requested review from SchweisguthN

@JeSuisUnCaillou JeSuisUnCaillou Awaiting requested review from JeSuisUnCaillou

@Isalafont Isalafont Awaiting requested review from Isalafont

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Samuelfaure @skelz0r @Isalafont @JeSuisUnCaillou