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

[ISSUE-1163] Inject OpenPaaS user contacts into Tmail's auto-complete database #1215

Merged
merged 33 commits into from
Oct 25, 2024
Merged

[ISSUE-1163] Inject OpenPaaS user contacts into Tmail's auto-complete database #1215

merged 33 commits into from
Oct 25, 2024

Conversation

HoussemNasri
Copy link
Member

@HoussemNasri HoussemNasri commented Oct 3, 2024 •
edited
Loading

Fix #1163

chibenwa
chibenwa reviewed Oct 3, 2024
View reviewed changes
.../src/main/scala/com/linagora/tmail/james/jmap/contact/openpaas/OpenPaasContactsConsumer.java Outdated
Comment on lines 41 to 50
Boolean.TRUE.equals(channelPool.getChannelMono()
.map(channel -> {
try {
channel.queueDeclarePassive(QUEUE_NAME);
return true;
}
catch (IOException e) {
return false;
}
}).block());
Copy link
Member

Choose a reason for hiding this comment

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

That's a VERY good finding, I wasn't aware of this methods.

So if the queue already exist but with slightly different arguments does it throws? Can you please quickly test that? If no, then we have a solution to our major RabbitMQ migration nightmares Cc @QuangHoang1210 @Arsnael @vttranlina and we could use it more widely in the source code...

quantranhong1999
quantranhong1999 reviewed Oct 4, 2024
View reviewed changes
Copy link
Member

@quantranhong1999 quantranhong1999 left a comment

Choose a reason for hiding this comment

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

Read it

chibenwa
chibenwa reviewed Oct 4, 2024
View reviewed changes
...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java Outdated
.durable(DURABLE)),
sender.declareQueue(QueueSpecification
.queue(QUEUE_NAME)
.durable(evaluateDurable(DURABLE, commonRabbitMQConfiguration.isQuorumQueuesUsed()))),
Copy link
Member

Choose a reason for hiding this comment

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

Queue MUST always be durable.

Copy link
Member

Choose a reason for hiding this comment

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

And ideally queue settings shall be obtained from commonRabbitMQConfiguration

Copy link
Member Author

Choose a reason for hiding this comment

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

There isn't any isDurable() method in commonRabbitMQConfiguration, should I add one?

chibenwa
chibenwa reviewed Oct 7, 2024
View reviewed changes
.../extensions/src/main/java/com/linagora/tmail/james/jmap/contact/jCardObjectDeserializer.java Outdated Show resolved Hide resolved
.../extensions/src/main/java/com/linagora/tmail/james/jmap/contact/jCardObjectDeserializer.java Outdated Show resolved Hide resolved
.../extensions/src/main/java/com/linagora/tmail/james/jmap/contact/jCardObjectDeserializer.java Outdated Show resolved Hide resolved
...backend/jmap/extensions/src/main/java/com/linagora/tmail/james/jmap/contact/jCardObject.java Outdated Show resolved Hide resolved
...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java Outdated Show resolved Hide resolved
...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java Outdated Show resolved Hide resolved
...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java Outdated
Comment on lines 98 to 102
Mono.just(messagePayload)
.map(message -> gson.fromJson(message, OpenPaasContactMessage.class))
.<ContactAddedRabbitMqMessage>handle((message, sink) -> {
Copy link
Member

Choose a reason for hiding this comment

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

We likely do not need that "just" and can inline the reactor transformations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how...

...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java Outdated Show resolved Hide resolved
...nsions/src/test/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumerTest.java Outdated Show resolved Hide resolved
...extensions/src/main/java/com/linagora/tmail/james/jmap/contact/OpenPaasContactsConsumer.java Outdated Show resolved Hide resolved
@HoussemNasri HoussemNasri mentioned this pull request Oct 8, 2024
Closed
chibenwa
chibenwa reviewed Oct 8, 2024
View reviewed changes
Copy link
Member

@chibenwa chibenwa 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.

Cool but IMO we would deserve to have dedicated tests for OpenPaaSWebClient class.

(Test in isolation for this)

@HoussemNasri HoussemNasri force-pushed the sync-openpaas-contact branch 2 times, most recently from 54c2718 to 4dbd165 Compare October 9, 2024 12:42
chibenwa
chibenwa reviewed Oct 22, 2024
View reviewed changes
...rd-party/openpaas/src/test/java/com/linagora/tmail/contact/OpenPaasContactsConsumerTest.java Outdated
Comment on lines 346 to 353
/*
* The automatic contact indexing is triggered when you send or receive a message
* from someone, then their contact info be automatically indexed in the contacts' search engine.
* */
@Test
void automaticContactIndexingShouldNotOverrideContactInfoFromOpenPaas() {
//
}
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we have defined the expected behavior. i.e. OpenPaas is the single source of truth for contacts, this test case become irrelevant.

chibenwa
chibenwa reviewed Oct 22, 2024
View reviewed changes
Copy link
Member

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

We are getting closer ;-)

HoussemNasri and others added 8 commits October 22, 2024 15:32
@HoussemNasri @chibenwa
Update tmail-backend/tmail-third-party/openpaas/src/main/java/com/lin…
98e68c7 
…agora/tmail/contact/OpenPaasContactsConsumer.java

Co-authored-by: Benoit TELLIER <[email protected]>
@HoussemNasri
Add import
a8bec38
@HoussemNasri @chibenwa
Update tmail-backend/tmail-third-party/openpaas/src/test/java/com/lin…
98ef683 
…agora/tmail/contact/JCardObjectTest.java

Co-authored-by: Benoit TELLIER <[email protected]>
@HoussemNasri @chibenwa
Update tmail-backend/tmail-third-party/openpaas/src/main/java/com/lin…
82c6afa 
…agora/tmail/contact/OpenPaasContactsConsumer.java

Co-authored-by: Benoit TELLIER <[email protected]>
@HoussemNasri @chibenwa
Update tmail-backend/tmail-third-party/openpaas/src/test/java/com/lin…
199aecd 
…agora/tmail/contact/OpenPaasContactsConsumerTest.java

Co-authored-by: Benoit TELLIER <[email protected]>
@HoussemNasri @chibenwa
Update tmail-backend/tmail-third-party/openpaas/src/main/java/com/lin…
4a1c7ae 
…agora/tmail/contact/ContactAddedRabbitMqMessage.java

Co-authored-by: Benoit TELLIER <[email protected]>
@HoussemNasri
Refactor
564082b
@HoussemNasri
Fix compilation
890f7b5
@HoussemNasri @chibenwa
Update tmail-backend/tmail-third-party/openpaas/src/test/java/com/lin…
543992d 
…agora/tmail/contact/OpenPaasContactsConsumerTest.java

Co-authored-by: Benoit TELLIER <[email protected]>
chibenwa
chibenwa approved these changes Oct 23, 2024
View reviewed changes
Copy link
Member

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

Should be more or less good to me

quantranhong1999
quantranhong1999 approved these changes Oct 25, 2024
View reviewed changes
Copy link
Member

@quantranhong1999 quantranhong1999 left a comment

Choose a reason for hiding this comment

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

LGTM

@chibenwa chibenwa merged commit 40331b2 into linagora:master Oct 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vttranlina vttranlina vttranlina left review comments

@quantranhong1999 quantranhong1999 quantranhong1999 approved these changes

@chibenwa chibenwa chibenwa approved these changes

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

Successfully merging this pull request may close these issues.

Tmail: Inject user contacts into auto-complete database
4 participants
@HoussemNasri @chibenwa @quantranhong1999 @vttranlina