Skip to content
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

Merged
merged 13 commits into from
Mar 30, 2024
Merged

Implement read-only mode #1001

merged 13 commits into from
Mar 30, 2024

Conversation

sudokoko
Copy link
Member

@sudokoko sudokoko commented Mar 28, 2024

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:

  • Level uploads or changes
  • Photo uploads
  • General asset uploads (/upload/hash)
  • Profile picture/biography changes
  • Post/delete comments
  • Post/delete reviews

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.

@sudokoko sudokoko requested review from Slendy and Zaprit March 28, 2024 03:02
Copy link
Contributor

@Slendy Slendy left a 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

Copy link
Member

@Zaprit Zaprit left a 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?

@sudokoko
Copy link
Member Author

sudokoko commented Mar 28, 2024

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.

@HenryAsbridge
Copy link

specific

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

@Zaprit
Copy link
Member

Zaprit commented Mar 28, 2024

specific

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

@sudokoko
Copy link
Member Author

specific

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

#1001 (comment)

An instance administrator can always link to it in the announcement config and create a web based announcement.

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.

@sudokoko sudokoko requested a review from Slendy March 28, 2024 16:10
@Zaprit
Copy link
Member

Zaprit commented Mar 28, 2024

specific

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

#1001 (comment)

An instance administrator can always link to it in the announcement config and create a web based announcement.

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?

@sudokoko
Copy link
Member Author

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.

@sudokoko sudokoko requested a review from Zaprit March 28, 2024 17:48
Accidentally tried to use markdown within the landing page... I'm rather smart aren't I
Copy link
Contributor

@Slendy Slendy left a 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.

@sudokoko sudokoko added this pull request to the merge queue Mar 30, 2024
Merged via the queue into main with commit 0ee8970 Mar 30, 2024
2 checks passed
@sudokoko sudokoko deleted the read-only branch March 30, 2024 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants