Forum Message UI Improvement (Thesis Board #3470) #61

tozh4728 · 2024-12-18T09:35:44+01:00

tozh4728 commented

2024-12-18 09:35:44 +01:00

Requirements

On supervisor's start page, a overview of the projects being supervised is shown as a table. One of columns is a flag and on clicking action the browser is redirected to forum page of that project.

Two improvements were asked:

If there is unread messages, show a tooltip if mouse is placed over the flag.
A number is shown to indicate number of unread messages.

How to test

Go to https://scipro-3470-forum-msg-ui-improvement.branch.dsv.su.se/
Log in as eric@example.com
Go to the project overview
Write something in the forum
Mark as unread (click the flag)
Go back to overview

Fix #63 **Requirements** On supervisor's start page, a overview of the projects being supervised is shown as a table. One of columns is a flag and on clicking action the browser is redirected to forum page of that project. Two improvements were asked: 1. If there is unread messages, show a tooltip if mouse is placed over the flag. 2. A number is shown to indicate number of unread messages. **How to test** 1. Go to https://scipro-3470-forum-msg-ui-improvement.branch.dsv.su.se/ 2. Log in as `eric@example.com` 3. Go to the project overview 4. Write something in the forum 5. Mark as unread (click the flag) 6. Go back to overview

tozh4728 added 8 commits 2024-12-18 09:35:45 +01:00

3470: Show tooltip if there are unread messages b58a8a2d8b

* AbstractReadStatePanel is expanded with option to show tooltip.

3470: Enhance ProjectForumStateColumn of SupervisorMyProjectPanel 499aaaa402

* it contains 2 components, one with flag, one with number of messages.
* since table cell can only contain one component, Wicket Fragment is used.

3470: Write a-element in one line to avoid white spaces: 2eb7f1bf14

* since this component is put in a table cell, and when there are more than one component in the cell, the white spaces breaks the visual with additional underscore like character.

3470: Move variable 'icon' from class level to method level: 0d612d44ee

This reduces the visibility of the variable icon, which usually is a good thing. The 'icon' is never accessed anywhere else.

3470: Write a method to get unread messages count as POC 552aaade6e

3470: Add new panel to show unread message count (as POC so far) 74b2396376

3470: Add logic to retrieve actual message counts 0525f89560

3470: Use JPA native query to reduce number of calls to database

Build and test / build-and-test (push) Successful in 6m56s

Details

3e71bc1c9e

tozh4728 added 1 commit 2024-12-18 13:26:11 +01:00

3470: Refactor how message counting works:

Build and test / build-and-test (push) Successful in 6m51s

Details

b20b1239a0

1. Now we retrieve at first all threads, then use another database call to get count of threads which have unread posts. It makes possible to reuse the logic of unread-post-counting somewhere else.
2. We use QueryDSL instead of native SQL, also left join from thread table instead of exists().

tozh4728 added 2 commits 2024-12-18 15:17:16 +01:00

3470: Optimize import c8930ec5fe

3470: Use Wicket behavior (less intrusive) to modify AbstractReadStatePanel

Build and test / build-and-test (push) Successful in 6m57s

Details

379b5c34fc

tozh4728 added 2 commits 2024-12-18 15:31:53 +01:00

Merge branch 'develop' into 3470-forum-msg-ui-improvement 2ad0061a09

# Conflicts:
#	core/src/main/java/se/su/dsv/scipro/forum/BasicForumService.java
#	core/src/main/java/se/su/dsv/scipro/forum/ForumPostRepository.java
#	core/src/main/java/se/su/dsv/scipro/forum/ForumPostRepositoryImpl.java
#	core/src/main/java/se/su/dsv/scipro/forum/ProjectThreadRepository.java
#	core/src/main/java/se/su/dsv/scipro/forum/ProjectThreadRepositoryImpl.java
#	view/src/main/java/se/su/dsv/scipro/forum/panels/AbstractReadStatePanel.java
#	view/src/main/java/se/su/dsv/scipro/supervisor/panels/SupervisorMyProjectsPanel.java

3470: Reformat code with prettier

Build and test / build-and-test (push) Failing after 42s

Details

08f40959fd

tozh4728 added 1 commit 2024-12-18 15:38:37 +01:00

3470: Reformat code with prettier for BasicForumService

Build and test / build-and-test (push) Successful in 15m43s

Details

1f2670b501

tozh4728 changed title from ~~WIP: Forum Message UI Improvement (3470)~~ to Forum Message UI Improvement (3470)

2024-12-18 15:45:54 +01:00

tozh4728 referenced this pull request

2024-12-18 15:49:42 +01:00

Supervisor Project Overview - Forum Message UI Improvement (#3470 in Thesis Leankit) #63

ansv7779 added 1 commit 2024-12-19 10:46:10 +01:00

Merge branch 'develop' into 3470-forum-msg-ui-improvement

Build and test / build-and-test (push) Successful in 21m10s

Details

Deploy to branch.dsv.su.se / deploy (pull_request) Successful in 3m42s

Details

6bcc869e4d

ansv7779 self-assigned this 2024-12-19 11:24:36 +01:00

ansv7779 removed their assignment 2024-12-19 11:24:50 +01:00

ansv7779 requested changes 2024-12-19 11:25:15 +01:00

Dismissed

ansv7779 left a comment

Some suggestions for improvement.

Things that should be fixed

NumberOfMessagesPanel

This panel has no behaviour and just shows a label whose data comes from outside. Can move the Label component to where it is used (SupervisorMyProjectsPanel). The label's IModel does not need to include the parenthesis, they should be moved to the HTML since they have to do with presentation rather than the data to display.

Old `hasUnreadThreads` method in `ProjectForumService`

This method was replaced with the getUnreadThreadsCount method and should be removed, its associated test should be updated to test the new getUnreadThreadsCount method.

Wicket gotcha's that's good to know

Risk of stale information

Due to the way the unread thread count is retrieved in SupervisorMyProjectsPanel it may not updated as the underlying data (in the database) changes because the constructor is only run once. However, in this specific case, since it is done in a RefreshingView (the component that is responsible for rendering IColumns in a DataTable) that for implementation specific reasons does re-run its populateItem method it does indeed work. But had the fetching not been done in a RefreshingView it would not have worked as expected on page refresh.

To solve this the msgCount should be loaded in an IModel (because those get detached and re-attached at every refresh). The ideal implementation is LoadableDetachableModel to cache the value during each render if the associated getObject method is called multiple times. An IModels getObject should not be called in the constructer because then you have the some problem, instead of use methods on IModel such as map or call getObject from Wicket's lifecycle onConfigure, onClick, `onSubmit, and so on.

`getString` in "constructor"

Similar to the above problems with loading data outside of IModel exist for calling getString (line 287 in SupervisorMyProjectsPanel). In these cases the fix is very simple, replace getString(...) with new ResourceModel(...) that does the same thing just wrapped in an IModel and AttributeModifier has a constructor for accepting IModel instead of direct Strings. Again, this works now because we're in a RefreshingView otherwise you'd get a warning similar to what happened in #48.

Some suggestions for improvement. # Things that should be fixed ## NumberOfMessagesPanel This panel has no behaviour and just shows a label whose data comes from outside. Can move the `Label` component to where it is used (`SupervisorMyProjectsPanel`). The label's `IModel` does not need to include the parenthesis, they should be moved to the HTML since they have to do with presentation rather than the data to display. ## Old `hasUnreadThreads` method in `ProjectForumService` This method was replaced with the `getUnreadThreadsCount` method and should be removed, its associated test should be updated to test the new `getUnreadThreadsCount` method. # Wicket gotcha's that's good to know ## Risk of stale information Due to the way the unread thread count is retrieved in `SupervisorMyProjectsPanel` it may not updated as the underlying data (in the database) changes because the constructor is only run once. However, in this specific case, since it is done in a `RefreshingView` (the component that is responsible for rendering `IColumn`s in a `DataTable`) that for implementation specific reasons *does* re-run its `populateItem` method it does indeed work. But had the fetching not been done in a `RefreshingView` it would not have worked as expected on page refresh. To solve this the `msgCount` should be loaded in an `IModel` (because those get detached and re-attached at every refresh). The ideal implementation is `LoadableDetachableModel` to cache the value during each render if the associated `getObject` method is called multiple times. An `IModel`s `getObject` should not be called in the constructer because then you have the some problem, instead of use methods on `IModel` such as `map` or call `getObject` from Wicket's lifecycle `onConfigure`, `onClick`, `onSubmit, and so on. ## `getString` in "constructor" Similar to the above problems with loading data outside of `IModel` exist for calling `getString` (line 287 in `SupervisorMyProjectsPanel`). In these cases the fix is very simple, replace `getString(...)` with `new ResourceModel(...)` that does the same thing just wrapped in an `IModel` and `AttributeModifier` has a constructor for accepting `IModel` instead of direct `String`s. Again, this works now because we're in a `RefreshingView` otherwise you'd get a warning similar to what happened in #48.

tozh4728 changed title from ~~Forum Message UI Improvement (3470)~~ to Forum Message UI Improvement (Thesis Board #3470)

2024-12-19 12:13:04 +01:00

tozh4728 added 3 commits 2024-12-19 12:15:52 +01:00

3492: Move counter to a Label instead of Panel, which simplifies a lot. 4a9232dab0

3492: Remove unused method hasUreadThreads in ProjectForumService d5bc04e90c

Merge remote-tracking branch 'refs/remotes/origin/3470-forum-msg-ui-improvement' into 3470-forum-msg-ui-improvement

Build and test / build-and-test (push) Failing after 45s

Details

Deploy to branch.dsv.su.se / deploy (pull_request) Successful in 2m10s

Details

8f647c67d4

tozh4728 added 1 commit 2024-12-19 12:18:44 +01:00