Closed (fixed)
Project:
Automated Logout
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
30 Nov 2020 at 09:31 UTC
Updated:
1 Aug 2022 at 16:34 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
bcizej commentedComment #3
sabina.h commentedProviding a patch for this issue.
Comment #4
bcizej commentedNice work thanks @sabina.h
Couple of minor issues to correct here:
You can use
for this variable.
Here as well.
Method summary should be in 3rd person. https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...
Tests that configurable text is displayed in the popup.The timeout and sleep values could be lowered.
If autologout settings are not updated because of caching then this is a problem that needs to be further investigated and find a fix.
Comment #5
sabina.h commentedThank you for you review. Providing the corrected patch with the interdiff. I have also tested the autologout settings and came to a conclusion that there aren't any cache issues.
Comment #6
deaom commentedHi, I tested the provided patch and it applies and also tests go through on my local machine. If I understood @bcizej correctly, he also suggested to remove the
drupal_flush_all_caches();all other suggestions are added to the patch. I tested it without that line and the tests are still working. So marking this as Needs work, for that line to be removed, everything else looks good to me.When uploading the interdiff file either choose the do not test option or upload with the .txt extension, so the file does not get tested :).
Comment #7
sabina.h commentedThank you @DeaOm for your review. Here is the corrected patch.
Comment #9
bcizej commentedTests are failing because of #3190324: Settings are not updated on cached pages
So we have to wait for that issue to get merged before testing again.
Comment #10
bcizej commentedComment #11
bcizej commentedComment #12
bcizej commentedMeanwhile I cleaned the code a bit.
Removed unneeded node module in test and since this is a Functional Javascript test we can use
instead of
Also we don't need to assert if user is logged in so I removed those assertions.
$this->drupalLogin()already asserts that though.Comment #13
bcizej commented#3190324: Settings are not updated on cached pages has been commited so this is ready for review.
Comment #14
deaom commentedTested the patch on the newest dev version of the module (which has the cache correction) locally on Drupal core 8.9.11 and test are also passing. The added changes did not break anything, so everything seems fine from my side. Marking this as RTBC.
Comment #15
japerryAdded to 1.4 release issue #3293639: Issues for 8.x-1.4 release
Comment #16
japerry