[Bugfix:Forum] Page refresh on out-of-sync clock.#12339
[Bugfix:Forum] Page refresh on out-of-sync clock.#12339Eli-J-Schwartz wants to merge 10 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12339 +/- ##
============================================
- Coverage 21.67% 21.66% -0.02%
- Complexity 9618 9637 +19
============================================
Files 268 268
Lines 36158 36222 +64
Branches 486 487 +1
============================================
+ Hits 7837 7847 +10
- Misses 27839 27892 +53
- Partials 482 483 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
williamjallen
left a comment
There was a problem hiding this comment.
Won't this erase fields with text entered? My initial thought is that the side effects of reloading the page outweigh the benefits of handling this case, but I could be convinced otherwise.
I have specific comments about the implementation itself as well, but let's first resolve the broader question of whether this is a good idea in the first place.
|
Whether or not text fields are erased seems to depend on the reloaded page. From my testing, written text will stay on forum posts, responses, and gradable submissions, but not for creating a new gradable. However, for the new behavior to be triggered, either the browser window or tab needs to be closed, or a link within the page to be clicked. When this happens, the browser will prompt the user anyway, warning them that data might be lost. |
roye2
left a comment
There was a problem hiding this comment.
While I am not convinced that this is a necessary change, I don't see any issues with this PR. The changes made seem to work as intended, and I was unable to find any errors. The code seems well documented and is easily readable. I believe when we spoke in person you mentioned that the failed test was not caused by your changes. If that is the case, then everything looks good to me!
There appeared to be a linting issue on the formatting of the return statement. I have updated it to fix the error.
|
@williamjallen Updates on new behavior: This can be tested by looking at verify_time.js every time the page is reloaded. Through my testing, going forwards, backwards, clicking links, closing and reopening the tab, and closing and reopening the window, all result in verify_time.js being properly updated with the current time. When verify_time.js is modified to think the server time is 0, the banner displays as expected. What are your thoughts on this? |
aconfo
left a comment
There was a problem hiding this comment.
I performed only a functionality test. When testing the changed code, I managed to replicate the issue. The main difference I saw was that the warning message for being out of sync was no longer present. However, It did not automatically update the discussion form
As can be seen in the image below, the message I wrote did not appear, but neither did the warning for being out of sync.
However, when trying to reload on that same page as was, the page itself did not refresh. Only by clicking on "discussion forum" did the post become visible
This remained true when testing again on the page seen in the image above. After reopening the page, this was what was shown up:
However, unlike the previous, reloading the page here did display the new message
Assuming that the intention is for it to automatically pop up when reopening the page, then I believe that these changes don't fully fix the problem as intended, and changes should be made to fix these problems. I would also recommend that the banner for the time being out of sync be added back in.
|
I definitely agree the current existing behavior here is not ideal. Just a simple back button is enough to trigger this warning (if enough time has passed). I'm not a huge fan of this new approach for a couple reasons:
I would argue that this warning should strictly be on the pages requiring precise client/server time sync. The only one I'm tracking is the gradeable submit page with a timed gradeable. But if there's more, please let me know. This is how we do it for the websocket banner where it only impacts the pages that use the websocket server. That gradeable submit page already has ways to fetch the server time and should be able to frequently check clock skew issues. And any JS logic needed to fix the warning when it happens can be addressed there. |
williamjallen
left a comment
There was a problem hiding this comment.
I think setting cache control headers to tell the browser not to cache pages makes sense. We have plenty of dynamic content, and I don't really see a benefit to caching the rendered Twig templates. Assets should remain cached, of course.
That said, I think the verify_time.js route should be removed here. All that's necessary is setting the headers. Any objections to this if the only changes are the site/app/controllers/GlobalController.php changes @cjreed121?
|
I modified the PR to only set cache control headers, without any of the unnecessary routing from before. I believe this will solve both the issue of stale pages, since any dynamic content will no longer be cached by the browser and will be reloaded any time the page is refreshed, as well as the incorrect time banner, since the current server time will also be reloaded. Any thoughts? |
There was a problem hiding this comment.
I primarily did a functionality test, and can confirm that when reopening the closed page, the discussion forum is automatically refreshed. There was however a sort of edge case, where if the user is in this window:
The page does not refresh upon being reopened, and there is no warning message. Oddly enough, while trying to reload this page from here, it fails to update the discussion forum, despite the fact that when reloading the page I can see the new post for about a fraction of a second. This problem is however for specifically this page, and not the main discussion form view like in here:
Where this page functions as it should. Clicking on the discussion forum from the first image page brought me back to the main discussion form view in which it was refreshed. I myself am not too sure of the importance of fixing this specific page problem in particular, but I figured I should mention it in my review since it could be a problem.
aconfo
left a comment
There was a problem hiding this comment.
I retested the changes and found that the errors that I had previously mentioned have been resolved.
williamjallen
left a comment
There was a problem hiding this comment.
Looks good to me, but I'll defer to @cjreed121 here if he has a strong opinion otherwise.
| } | ||
|
|
||
| private function setCacheControlHeaders(): void { | ||
| header("Cache-Control: no-cache, no-store, must-revalidate"); // HTTP 1.1. |
There was a problem hiding this comment.
Does this get overwritten now? https://github.com/Submitty/Submitty/blob/main/site/public/index.php#L114
|
Have you done any sort of testing on what impact this has to the server? We do have some "static" content that is served dynamically such as poll images, course materials, etc. I'm not 100% sure on the current caching behavior with those but was curious if it makes a difference with this change. |
|
@Eli-J-Schwartz please reply to @cjreed121 comment above. |
|
Sorry for the delayed response, @cjreed121 .
You are correct in that the 'static' content is no longer cached in some instances (polls, notebook homeworks). However, course materials are still cached, because they are served files, not dynamic pages, and aren't affected by the change I made. The course materials overview page is affected by the change, but it just includes the directories and file names, and none of the actual content.
I tested this, and some pages, such as the login check system and course materials still use the headers set there. I think keeping it is the best option for now. Please let me know what you think. |
I was not suggesting doing load testing so this should not need a performant machine. Here's an example of a test you could do: Add some code to the server to log each time a PHP routed request happens. Come up with a list of actions to do on the website (things like visiting pages, refreshing, hitting the back button, etc) and then run that before and after this PR. Now compare the values of that log with the changes before and after this PR. You will likely need to ensure your browser cache (and other related client side things) is cleared/reset before each test. It is good to know how much impact a PR will have on a server when dealing with performance, cache headers, etc.
So for normal pages, what is the value of |
|
Regarding performance, here is what I found: Regarding the redundant cache-control set, here is what I found: Given this information, I think it would make the most sense to move my new code over to index.php, to ensure that all pages are consistently loaded with the same headers. The "private" header's effects are redundant given the "no-store" header, so removing the old code will not cause any security problems. Additionally, course materials are already being refreshed automatically, adding new headers to them should not cause any additional performance issues. One potential concern I know of is that pressing the back button would cause a php-routed request, which could be an issue depending on how often users click the forward and backward buttons. What are your thoughts on this potential solution? |
|
Sorry for the delayed response. Yeah moving the headers to index.php would definitely be a better idea. If private is no longer needed, then please remove that too in there. I'm not a huge fan of the extra requests on forward/back buttons but I suppose we can see if there's any noticeable impact on the production server. |
Why is this Change Important & Necessary?
Fixes #12338
What is the New Behavior?
Old behavior was that a reopened page would not automatically retrieve the most recent data, such as in a discussion forum. For example:
New forum post is made:
Reopened page contains error message and not the new post:
Manual reload displays the new post:
New behavior is that the page is automatically reloaded if it detects that the users system clock is out of sync with the time when the page was originally sent from the server. For example:
A new post is made:
The post is immediately displayed in a reloaded tab, with no error message:
What steps should a reviewer take to reproduce or test the bug or new feature?
Reproduce old behavior:
Reproduce new behavior:
Automated Testing & Documentation
This is currently no testing for this fix, as I am unfamiliar with the current testing framework. Once I get accustomed to the testing design, I will add tests for this.
Other information
The page is reloaded if a difference of more than 2 minutes is detected. This amount of time is arbitrary, but generally sufficient for automatic refreshes.