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

feat: add new ENO transformation PoguesJSONToLunaticJSON #314

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Conversation

laurentC35
Copy link
Contributor

@laurentC35 laurentC35 commented Aug 20, 2024

fix: #182

@laurentC35 laurentC35 changed the title feat: add new transformation PoguesJSONToLunaticJSON feat: add new ENO transformation PoguesJSONToLunaticJSON Aug 20, 2024
@laurentC35 laurentC35 added the deploy-snapshot To be used in PR to trigger snapshot deploy pipeline label Aug 20, 2024
@github-actions github-actions bot removed the deploy-snapshot To be used in PR to trigger snapshot deploy pipeline label Aug 20, 2024
Copy link

👋 Version 4.8.1-SNAPSHOT deployed on docker hub

@laurentC35 laurentC35 added the deploy-snapshot To be used in PR to trigger snapshot deploy pipeline label Aug 21, 2024
@github-actions github-actions bot removed the deploy-snapshot To be used in PR to trigger snapshot deploy pipeline label Aug 21, 2024
Copy link

👋 Version 4.8.2-SNAPSHOT deployed on docker hub

@RemiVerriez RemiVerriez changed the base branch from main to next October 30, 2024 09:45
@RemiVerriez RemiVerriez requested a review from nsenave October 30, 2024 09:45
@RemiVerriez RemiVerriez self-assigned this Oct 30, 2024
@RemiVerriez RemiVerriez merged commit a9b9d66 into next Oct 30, 2024
7 checks passed
@RemiVerriez RemiVerriez deleted the feat/#182 branch October 30, 2024 09:46
@FBibonne FBibonne restored the feat/#182 branch October 31, 2024 09:52
Copy link
Contributor

@nsenave nsenave left a comment

Choose a reason for hiding this comment

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

pas mal de refacto

@@ -13,7 +13,7 @@
<groupId>fr.insee</groupId>
<artifactId>Pogues-BO</artifactId>
<packaging>jar</packaging>
<version>4.8.1</version>
<version>4.8.2-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

should be 4.9.0 since its a feature

@@ -63,7 +69,7 @@ public String getDDITOLunaticJSON(String inputAsString, Map<String, Object> para
MultiValueMap<String,String> queryParams = new LinkedMultiValueMap<>();
String modePathParam = params.get("mode") != null ? params.get("mode").toString() : MODE;
String WSPath = BASE_PATH + "/lunatic-json/" + modePathParam;
queryParams.add("dsfr", Boolean.TRUE.equals(params.get("dsfr")) ? "true" : "false");
queryParams.add(DSFR_QUERY_PARAM, Boolean.TRUE.equals(params.get("dsfr")) ? "true" : "false");
Copy link
Contributor

Choose a reason for hiding this comment

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

"dsfr" est encore en dur dans params.get("dsfr")

Copy link
Contributor

Choose a reason for hiding this comment

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

peut-être une façon plus simple de cast le booléen en string qu'avec un ternaire

+ vérifier qu'on a bien ce qu'on veut si c'est null

Copy link
Contributor

Choose a reason for hiding this comment

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

à encadrer dans une méthode pour contrôler le truc

@@ -72,6 +78,19 @@ public String getDDITOXForms(String inputAsString) throws EnoException, PoguesEx
return callEnoApi(inputAsString, BASE_PATH+"/xforms");
}

@Override
public String getJSONPoguesToLunaticJson(String inputAsString, Map<String, Object> params) throws URISyntaxException, IOException, EnoException {
log.info("getJSONPoguesToLunaticJson - started");
Copy link
Contributor

Choose a reason for hiding this comment

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

peut-être que le log.info devrait être à un niveau plus élevé, log.debug ici ?

log.info("getJSONPoguesToLunaticJson - started");
MultiValueMap<String,String> queryParams = new LinkedMultiValueMap<>();
String modePathParam = params.get("mode") != null ? params.get("mode").toString() : MODE;
String WSPath = String.format("/questionnaire/pogues-2-lunatic/%s/%s",
Copy link
Contributor

Choose a reason for hiding this comment

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

nom de variable locale qui commence par une majuscule -> wsPath


private static final String DSFR_QUERY_PARAM = "dsfr";

private static final String BASE_PATH = "/questionnaire/" + DEFAULT_CONTEXT;
private static final String MODE = "CAWI";
Copy link
Contributor

Choose a reason for hiding this comment

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

à renommer en CAWI_MODE ou qqch de plus explicite ?

String WSPath = String.format("/questionnaire/pogues-2-lunatic/%s/%s",
DEFAULT_CONTEXT,
modePathParam);
log.info("WSPath : {} ",WSPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

log.debug ?

@Service
public class PoguesJSONToLunaticJSONImpl implements PoguesJSONToLunaticJSON {

@Autowired
Copy link
Contributor

Choose a reason for hiding this comment

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

Remplacer les autorwired par un constructeur

NB : peut être fait avec un record

private EnoClient enoClient;

@Override
public ByteArrayOutputStream transform(InputStream inputStream, Map<String, Object> params, String surveyName) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

🥲

@@ -69,8 +70,6 @@ public class VisualizeWithURI {
@Autowired
SuggesterVisuService suggesterVisuService;

private static final String CONTENT_DISPOSITION = HttpHeaders.CONTENT_DISPOSITION;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

.map(jsonToXML::transform, params, questionnaireName.toLowerCase())
.map(poguesXMLToDDI::transform, params, questionnaireName.toLowerCase())
.map(ddiToLunaticJSON::transform, params, questionnaireName.toLowerCase())
.map(poguesJSONToLunaticJSON::transform, params, questionnaireName.toLowerCase())
Copy link
Contributor

Choose a reason for hiding this comment

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

on pourrait passer null plutôt que le questionnaireName s'il ne sert pas dans la sous méthode

Copy link
Contributor

Choose a reason for hiding this comment

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

ou créer une nouvelle méthode map(Runable trunable, Map<String, Object> params) et utiliser celle là pour se préparer à virer celle avec le String questionnaireName

@FBibonne FBibonne deleted the feat/#182 branch October 31, 2024 15:34
nsenave pushed a commit that referenced this pull request Nov 13, 2024
* feat: add new transformation PoguesJSONToLunaticJSON

* bump: version to 4.8.2-SNAPSHOT
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.

[pogues2lunatic] - Simplify transformation
3 participants