-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement read-only mode #1001
Implement read-only mode #1001
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small feedback about returning 400 on the website page. You also need to add checks in SlotSettingsPage
and protect the calls to ParseBase64Image
ProjectLighthouse.Servers.Website/Pages/UserSettingsPage.cshtml.cs
Outdated
Show resolved
Hide resolved
ProjectLighthouse.Servers.Website/Pages/UserSettingsPage.cshtml.cs
Outdated
Show resolved
Hide resolved
ProjectLighthouse.Servers.Website/Pages/UserSettingsPage.cshtml.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, however do you think it would be worth while having some additional information field in there, so we can link to the blog post?
I think that might be a bit too specific to our case, though feel free to object. An instance administrator can always link to it in the announcement config and create a web based announcement. |
I suspect that anyone who's putting their lighthouse instance into read only mode is doing so for a reason, and may want to communicate with their users as to why |
and this is why you don't use the github mobile app, because it will make you respond on your work account smh. why can't it use the account that's in the org where the PR is, rather than the wrong account at the wrong time |
I may just be OCD, but I feel like implementing a dedicated configuration option for something that can be done through existing config options (announceText) and website features (web announcements, which will be implemented into the game as well through #855) is a bit redundant. Though if this is something you wholeheartedly believe should be implemented, I will do so. Just thought I'd voice my concern on it before doing that. |
I suppose the important part is do our banner announcements show up front and centre on the home page of the instance in a way that doesn't require you to go digging? |
No, however I would not be opposed to implementing this, and would actually prefer doing it this way. I would probably just package those changes in with this PR since they are technically in scope. |
Accidentally tried to use markdown within the landing page... I'm rather smart aren't I
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only issue is returning a 400 status code on the website instead of redirecting back to the original page to stay consistent with the rest of the website.
This PR implements the ability to put an instance in a "read-only" state.
The following actions are limited or disabled while in read-only mode:
A message will also be appended to the base website layout, as well as the /announce endpoint.
This could probably be refactored into a simple middleware or something in the future.