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

feat(conversation): WB-3630, added 3 new endpoints #559

Merged
merged 10 commits into from
Dec 20, 2024
Merged

feat(conversation): WB-3630, added 3 new endpoints #559

merged 10 commits into from
Dec 20, 2024

Conversation

jcbe-ode
Copy link
Contributor

@jcbe-ode jcbe-ode commented Dec 17, 2024 •
edited by jira bot
Loading

Description

  • Correction/modifiaction de scripts/conf maven + docker + jenkins (à discuter)
  • Ajout de 3 nouveaux endpoints pour conversation (to be reviewed).

Cette PR contient la PR #558 dont elle dépend, je la mettrai à jour une fois l'autre PR mergée.

Fixes

WB-3630

Type of change

Please check options that are relevant.

  • Chore (PATCH)
  • Doc (PATCH)
  • Bug fix (PATCH)
  • New feature (MINOR)

Which packages changed?

Please check the name of the package you changed

  • admin
  • app-registry
  • archive
  • auth
  • cas
  • common
  • communication
  • conversation
  • directory
  • feeder
  • infra
  • portal
  • session
  • test
  • tests
  • timeline
  • workspace

Tests

  1. Describe here the tests you performed
  2. Step by step
  3. With expected results

Reminder

  • Security flaws

  • Performance impacts (think bulk !)

  • Unit tests were replayed

  • Unit tests were added and/or changed

  • I have updated the reminder for the version including my modifications

  • All done ! 😃

juniorode
juniorode requested changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@juniorode juniorode left a comment

Choose a reason for hiding this comment

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

Il faudrai :

  • laisser les fichiers gradle.properties supprimés
  • modifier le gulpfile.js
  • laisser l'appel à init

@jcbe-ode jcbe-ode marked this pull request as ready for review December 18, 2024 09:46
@jcbe-ode jcbe-ode requested a review from juniorode December 18, 2024 09:47
mariusode
mariusode reviewed Dec 19, 2024
View reviewed changes
conversation/backend/src/main/java/org/entcore/conversation/util/MessageUtil.java
* @param userIndex Map of ID <-> {"id": user ID, "displayName": username}
* @param groupIndex Map of ID <-> {"id": group ID, "displayName": groupname}
*/
static public void computeUsersAndGroupsDisplayNames(
Copy link
Contributor

Choose a reason for hiding this comment

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

Je suis plus partisan de l'immutabilité des variables en paramètre des méthodes et du fait de retourner des objets qui résultent de l'information qu'on vient de manipuler.
Mais c'est peut-être plus une question de style de code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'avais initialement pensé retourner des uplets, mais on est en Java 8...
Et créer une classe pour retourner 2-3 données à la fois m'a paru inutile.

conversation/backend/src/main/java/org/entcore/conversation/util/MessageUtil.java Outdated
if (!(o2 instanceof String)) {
continue;
}
String[] a = ((String) o2).split("\\$");
Copy link
Contributor

Choose a reason for hiding this comment

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

idem, je trouve galère de manipuler l'information sous forme encodée. Est-ce qu'on ne pourrait pas créer un objet qui porte l'information, avec des noms parlants.
J'avais tenté ça dans le cadre du chantier userPosition.
Ça permet aussi de documenter les "règles" de l'encodage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comme discuté ensemble, je vais proposer une implémentation alternative.

conversation/backend/src/main/java/org/entcore/conversation/util/MessageUtil.java
* @param message message read from DB
* @param userInfos
* @param lang
* @param userIndex Map of ID <-> {"id": user ID, "displayName": username}
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi ne pas privilégier l'utilisation d'instances de classe et de map ? du genre Map<int, UserDisplayName>

Copy link
Contributor Author

@jcbe-ode jcbe-ode Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  • Map / JsonObject
    Cela ne fait pas de différence pour moi, c'est la même structure de donnée : une collection de clés/valeurs.
    En interne, un JsonObject est une instance de LinkedHashMap.
  • userIndex et groupIndex sont au format attendu par un Render de donnée en JSON.

La classe MessageUtil est dédiée au formatage des données pour les réponses du contrôleur.

conversation/backend/src/main/java/org/entcore/conversation/util/MessageUtil.java Outdated
* Replace recipients in a message (in DB format) with those found in an index.
* @param message message read from DB
* @param userIndex Map of ID <-> {"id": user ID, "displayName": username, "profile": profile}
* @param groupIndex Map of ID <-> {"id": group ID, "displayName": groupname, "nbUsers": nb users, "type": type, "subType": subtype}
Copy link
Contributor

Choose a reason for hiding this comment

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

idem : Map<int, GroupInfos> ?

mariusode
mariusode reviewed Dec 19, 2024
View reviewed changes
directory/src/main/java/org/entcore/directory/services/GroupService.java
@@ -35,6 +35,25 @@ public interface GroupService {
"FunctionGroup", "ManualGroup", "FuncGroup", "DeleteGroup", "DirectionGroup", "FunctionalGroup",
"DisciplineGroup", "CommunityGroup");

enum Field {
Copy link
Contributor

Choose a reason for hiding this comment

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

Je suis pas très familier du fonctionnement et de l'utilisation de ce genre d'enum avec des bitmask, je veux bien une explication à l'occasion :)

@jcbe-ode jcbe-ode merged commit f463381 into develop-b2school Dec 20, 2024
@jcbe-ode jcbe-ode deleted the feat-wb-3630 branch December 20, 2024 16:10
jcbe-ode added a commit that referenced this pull request Jan 6, 2025
@jcbe-ode
feat(conversation): WB-3630, added 3 /api endpoints under (#559)
b177606 
* chore: fix build scripts and POMs

* chore: added a buildBackend option to build.sh

* chore: rename gradle.properties files

* feat(conversation): #WB-3630, added GET /conversation/api/* endpoints

* feat(conversation, directory): #WB-3630, implement GET /conversation/api/folders/:folderId/messages

* feat(conversation): #WB-3630, implement GET git /conversation/api/folders

* feat(conversation): #WB-3630, implement GET /conversation/api/messages/{messageId} and cleanup code

* Revert "chore: rename gradle.properties files"

This reverts commit afa0667.

* chore: feedback from PR

* fix: feedback from PR
jcbe-ode added a commit that referenced this pull request Mar 4, 2025
@jcbe-ode
feat(conversation): WB-3630, added 3 /api endpoints under (#559)
f1af565 
* chore: fix build scripts and POMs

* chore: added a buildBackend option to build.sh

* chore: rename gradle.properties files

* feat(conversation): #WB-3630, added GET /conversation/api/* endpoints

* feat(conversation, directory): #WB-3630, implement GET /conversation/api/folders/:folderId/messages

* feat(conversation): #WB-3630, implement GET git /conversation/api/folders

* feat(conversation): #WB-3630, implement GET /conversation/api/messages/{messageId} and cleanup code

* Revert "chore: rename gradle.properties files"

This reverts commit afa0667.

* chore: feedback from PR

* fix: feedback from PR
Romu-C pushed a commit that referenced this pull request Mar 17, 2025
@jcbe-ode @Romu-C
feat(conversation): WB-3630, added 3 /api endpoints under (#559)
c164054 
* chore: fix build scripts and POMs

* chore: added a buildBackend option to build.sh

* chore: rename gradle.properties files

* feat(conversation): #WB-3630, added GET /conversation/api/* endpoints

* feat(conversation, directory): #WB-3630, implement GET /conversation/api/folders/:folderId/messages

* feat(conversation): #WB-3630, implement GET git /conversation/api/folders

* feat(conversation): #WB-3630, implement GET /conversation/api/messages/{messageId} and cleanup code

* Revert "chore: rename gradle.properties files"

This reverts commit afa0667.

* chore: feedback from PR

* fix: feedback from PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariusode mariusode mariusode left review comments

@juniorode juniorode Awaiting requested review from juniorode

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

Successfully merging this pull request may close these issues.
3 participants
@jcbe-ode @juniorode @mariusode