GNOME Bugzilla – Bug 741121
Lock down logout
Last modified: 2018-03-26 14:34:12 UTC
Created attachment 292138 [details] [review] a new page on disabling logout Hello, I've created a new page on disabling logout (lockdown section on the sysadmin guide). Feel free to review. Thank you for your time!
Review of attachment 292138 [details] [review]: Can you make this more topic/task oriented as opposed to a description of the keys? You are duplicating a lot of existing strings here and the page looks quite different from other help pages. Please make it more consistent with other reviewed pages, see desktop-lockscreen.page for an example.
Created attachment 302923 [details] [review] new page describing how to lock down logout Hello Kat, Thank you very much for the valuable feedback. I've applied your suggestions and I'm coming up with a new patch. Any more comments are warmly welcome! Jana
Review of attachment 302923 [details] [review]: Looks good to me. Thank you.
Thank you Petr. Pushed to master, commit: https://bugzilla.gnome.org/show_bug.cgi?id=741121 Closing the bug as fixed.
commit: 9916cd4cb6ac23382544b99f744d3e55b997fc0c
There was no link to Bugzilla in the commit message (so I had to search through bugzilla to find this bug, which was frustrating). The applied patch has validation errors: system-admin-guide/C/lockdown-logout.page:10: element item: validity error : ID dconf-profile-user already defined system-admin-guide/C/lockdown-logout.page:21: element item: validity error : ID dconf-profile-user-dir already defined system-admin-guide/C/lockdown-logout.page:39: element item: validity error : ID dconf-update already defined system-admin-guide/C/lockdown-logout.page:43: element item: validity error : ID dconf-logoutin already defined Petr, I think it is time that you stopped reviewing patches for gnome-user-docs if you are going to continue to be slapdash in your review comments. This patch was not ready to be accepted and merged.
(In reply to David King from comment #6) Dear David, Before I uploaded the patch, I made sure the "git apply --check <patch>" command did not return any error. When I applied the patch before committing to master I ensured the whole book builds by executing "yelp-check validate *.page", and again no error message was returned. When I tested the patch again today, I can see no error whatsoever, and the book builds as it is supposed to. The list of id errors you copy-pasted in your comment suggest that your git settings(?) refuse using dconf snippets twice in the same document, which is not harmful for the book IMHO. Petr obviously did not encounter any of the aforementioned errors as well; otherwise he would not approve of the patch. This issue raises a few questions: * What do you think I should have done differently? * How come Petr's and my git/yelp do not return the errors you mention when applying the patch? * Could anybody else test out the validity of the patch to find out the root cause? Concerning the documentation content: * Do you find the new page improved compared to my first draft? * Do you believe users can find it helpful? * Does the addition of the new page enhance our guide? Thank you very much for answering my questions and your time, jana
(In reply to Jana Svarova from comment #7) > (In reply to David King from comment #6) > > Dear David, > > Before I uploaded the patch, I made sure the "git apply --check <patch>" > command did not return any error. When I applied the patch before committing > to master I ensured the whole book builds by executing "yelp-check validate > *.page", and again no error message was returned. > > When I tested the patch again today, I can see no error whatsoever, and the > book builds as it is supposed to. > > The list of id errors you copy-pasted in your comment suggest that your git > settings(?) refuse using dconf snippets twice in the same document, which is > not harmful for the book IMHO. > > Petr obviously did not encounter any of the aforementioned errors as well; > otherwise he would not approve of the patch. Thanks for your explanation, Jana. I can only confirm what you say. Also, the applied patch validates for me, so I think we can close this bug.
(In reply to Petr Kovar from comment #8) > (In reply to Jana Svarova from comment #7) > > Before I uploaded the patch, I made sure the "git apply --check <patch>" > > command did not return any error. When I applied the patch before committing > > to master I ensured the whole book builds by executing "yelp-check validate > > *.page", and again no error message was returned. Mallard is validated using yelp-tools, specifically "yelp-check validate". I checked the validation after applying the patch to master, and I see the same errors as Dave mentioned in comment #6. > > When I tested the patch again today, I can see no error whatsoever, and the > > book builds as it is supposed to. > > > > The list of id errors you copy-pasted in your comment suggest that your git > > settings(?) refuse using dconf snippets twice in the same document, which is > > not harmful for the book IMHO. XML validation is not related to git settings. It is purely testing whether the page complies with the Mallard schema. > > Petr obviously did not encounter any of the aforementioned errors as well; > > otherwise he would not approve of the patch. > > Thanks for your explanation, Jana. I can only confirm what you say. Also, > the applied patch validates for me, so I think we can close this bug. Petr, we have a strict "must be valid XML" policy for docs. This has been the case for longer than I have been contributing and it is the reviewer's responsibility to make sure that patches validate. Patches which do not have valid XML must be fixed before they can be accepted. Apart from the validation issue, there are other improvements that should have been made to the patch so it should not have been approved for committing. I will review this patch tomorrow so that we can go through the review properly instead of compromising on quality. This patch has already delayed the 3.16.2 release, which is quite a serious issue.
Review of attachment 302923 [details] [review]: I have reverted this patch in commit 8bf62bfc97d611213100f29bb7bcbe02b11f6abd. You need to fix up general indentation before you resubmit the patch and make sure that it validates. Have you tested the instructions? ::: system-admin-guide/C/lockdown-logout.page @@ +7,3 @@ + <link type="guide" xref="user-settings#lockdown"/> + <link type="seealso" xref="dconf-lockdown" /> + <revision pkgversion="3.14" date="2014-05-04" status="draft"/> Is this page actually a draft or is it ready for review? @@ +23,3 @@ + <section id="user-logout"> + <title>Disable user logout</title> + <p>Follow these steps to prevent the user from logging out.</p> Why would I want to do this? Present a usecase. @@ +28,3 @@ + can thwart system administrator's intentions. That is the reason why it is + recommended to disable "user switching" as well to prevent this scenario + from occurring. See <link xref="#user-switch"/> for details.</p> Use simpler English @@ +51,3 @@ + <file>/etc/dconf/db/local.d/locks/lockdown</file>:</p> +<screen> +# Lock this key to disable user logout "Lock user logout": it's simpler and shorter, also matches the style on other pages. @@ +88,3 @@ + <file>/etc/dconf/db/local.d/locks/lockdown</file>:</p> +<screen> +# Lock this key to disable user switching As above
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-user-docs/issues/15.