Arad 🇷🇴
Account created on 13 April 2006, over 18 years ago
#

Merge Requests

More

Recent comments

🇷🇴Romania claudiu.cristea Arad 🇷🇴

claudiu.cristea made their first commit to this issue’s fork.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Maybe in few days, not later the end of the year.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

A merge request is welcommed

🇷🇴Romania claudiu.cristea Arad 🇷🇴

This is a legit request. I see several issue:

  • The class (stored in unread_notification_class) should be conditionally added. If it comes empty should not be added.
  • The URL should be passed as var along with from private_message_page_link.
  • The link should be constructed in Twig from the new UR: and new_items_count variables
  • I think unread_notification_class is opinionated (the 99+ thingie) and should be deprecated in 4.x and dripped in 5.x
🇷🇴Romania claudiu.cristea Arad 🇷🇴

I think you should:

  • Configure private_message_thread and private_message entity types as group content
  • Implement hook when a private_message_thread and private_message are create, detect the group context, and make those entities belonging to the detected groups.
  • Alter the routes of private_message and change their paths to be under the group path, so that the group context can be determined. E.g.: /private-messages -> /group/{group}/messages or /private-messages/{private_message_thread} -> /group/{group}/messages/{private_message_thread}, and so on...

This is not tested but it might work.

Alternatively, you can consider creating a private_message_group standalone module (or MR to add here as submodule) based the above concept.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

I see this is already in place. Unread threads get an unread-thread class. Closing.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Quoting from 💬 Automatically scroll down to last message when message thread is opened Active :

Why isn't there any "last" class at class="private-message-wrapper" ? How can I add it?
Why isn't there any anchor link to the last message in the thread? How can I add it?

This is a more complex issue. If we put a class on the latest we should remove it after the user has seen the message. Say after 3-5 seconds. But if the list is big, some of them are not visible (because you'll need to scroll), so the JS code should be clever enough to remove only the classes form messages that were scrolled into view (with a delay of 3-5 secs). If we still need this feature it should be split in a new ticket

🇷🇴Romania claudiu.cristea Arad 🇷🇴

This is doable by placing the inbox block on the desired page (e.g., /private-messages), Feel free to reopen if you think

🇷🇴Romania claudiu.cristea Arad 🇷🇴

You can place the block ONLY on /private-messages and /private-messages/{private_message_thread} page and you have the inbox. On mobile you might need some JS tweaks to collapse the inbox when you select a thread so that the thread shows on the screen.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Both, 3.0x-dev and 4.x-dev seem to work. But I am not sure.

And I can not install them via composer.
And I will NOT install any dev version on my productive site.

We will release shortly 4.0.0.-alpha1 and 3.0.5 if the -dev are OK for you

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Merged to 4.x and 3.0.x. Thank you

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Thank you for this catch. The "test only" commit shows the bug. Also, that piece of code looks more simple and easy to read.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

We need more info:

  • Which was the value of "value" column from the "key_value" table for "collection" = "system.schema" and "name" = "private_message" before you've started the update?
  • Which was the initial module version before you've started the update to 3.0.4?
🇷🇴Romania claudiu.cristea Arad 🇷🇴

Thank you!

This refactoring will not be backported in 3.0.x

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Problems outlined in #5 are fixed. Approving...

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Here's one of the scenarios I've manually tested:

  • On a clean install, apart from UID1, I've created 9 regular users (user1, user2, ..., user9)
  • As UID1, I've created 9 threads: first with user1 message "THREAD 1", then user2 message "THREAD 2"... and so on.
  • Double-checked in the backend to make sure each thread has only one message and a distinct user
  • Placed the "Private Message Actions" and "Private Message Inbox" blocks inside the Sidebar region of Olivero theme
  • Browse to /private-messages page
  • Inbox shows correctly 5 threads. From top to bottom: THREAD 9, THREAD 8, ..., THREAD 5
  • The "Load Previous" link is there
  • I click "Load Previous" and expecting to append the following batch to the list (THREAD 4..1)
  • But only THREAD 4 and THREAD 2 are added
  • I wait a little until the Ajax URL https://private-message.ddev.site/private-message/ajax-callback/get_new_... is called
  • Ajax inserts also THREAD 3 in the right place
  • But THREAD 1 is still missing regadless of how many Ajax refreshes are performed

In parallel, I've installed a new instance with the current 4.x HEAD and there works as it should

🇷🇴Romania claudiu.cristea Arad 🇷🇴

@drupalfan2, you didn't answer: Have you tried 3.0.x-dev or 4.x-dev?

🇷🇴Romania claudiu.cristea Arad 🇷🇴

I did a review and I left some remarks in MR

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Let's clarify the scope

But we can imagine, if there are several new messages, the user should see, in first place, the oldest of the new ones.

If you have lots of unread messages and scroll to the oldest unread, the reply box will pe pushed down, out of the screen and it makes no sense to focus on that. We can either (a) focus on reply and scroll to latest or (b) scroll to oldest unread and not focus the reply area. I was looking o how popular messengers are working. Every time they are scrolling to latest and are focusing the reply. Let's go with (a)

🇷🇴Romania claudiu.cristea Arad 🇷🇴

📌 Support Drupal >=10.3 Active is merged. Unpostponing

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Everything goes first to 4.x and, tentatively, to 3.0.x

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Thanks for review. This is not going to be backported to 3.0.x

🇷🇴Romania claudiu.cristea Arad 🇷🇴

claudiu.cristea made their first commit to this issue’s fork.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Fixed latest remarks

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Implemented most of remarks and commented to others. Ready for review.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Thank you!

Merged in 4.x. This will NOT be backported to 3.0.x.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Tested also manually, the functionality works correct after the removal of jQuery dependency. Improvement of test assertions is also welcomed. Approving.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Thank you

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Thank you for catching this. Could you, please, test the MT?

🇷🇴Romania claudiu.cristea Arad 🇷🇴

claudiu.cristea made their first commit to this issue’s fork.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

@drupalfan2, in 3.0.x a lot of JS functionality has been messed up. We're in the middle of stabilizing the code, fixing bugs and ensuring more test coverage. This is an ongoing effort in the new 4.x branch but we're also backporting as much as we can in 3.0.x.

It would help if you can provide a JS test that shows the bug. In the meantime you can test with 4.x and 3.0.x branches maybe this issue is already fixed there.

Thank you for understanding

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Ready for review.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Update IS to covers what's in the MR

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Merged into 4.x and cherry-picked to 3.0.x. Thank you!

🇷🇴Romania claudiu.cristea Arad 🇷🇴

This looks good. Agree with the test split, it's more readable and maintainable now.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Merged into 4.x & 3.0.x. Thank you all

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Upgrade path added. This looks good. It has testing coverage and I've tested manually the update path. Approved!

🇷🇴Romania claudiu.cristea Arad 🇷🇴

We've opened the 4.x branch. Each new fix/feature should be be filled agains the highest branch. After merging we'll decide whether it makes sense to backport to 3.0.x

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Needs an upgrade path to ensure we're also cleaning potential orphan private messages and orphan entries in pm_thread_history table

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Great refactoring. Thank you

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Should we close this as duplicate of 📌 Full coverage for JS/Ajax functionality Active ?

🇷🇴Romania claudiu.cristea Arad 🇷🇴

claudiu.cristea changed the visibility of the branch 3489458-support-deseralization to hidden.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Nice refactoring. I have only some small remarks and suggestions.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Release 3.0.4 is out fixing this regression

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Source of bug: random string was used for real user name

Ran several pipelines with this MR and now looks stable.

🇷🇴Romania claudiu.cristea Arad 🇷🇴

I'm looking to this

🇷🇴Romania claudiu.cristea Arad 🇷🇴

Can I get credit for that?

That's weird because I remember I have checked you (otherwise it would not be added to commit message)

Production build 0.71.5 2024