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

DEV: move post flags into database #26951

Merged
merged 10 commits into from May 21, 2024
Merged

DEV: move post flags into database #26951

merged 10 commits into from May 21, 2024

Conversation

lis2
Copy link
Contributor

@lis2 lis2 commented May 9, 2024

This is preparation for a feature that will allow admins to define their custom flags. Current behaviour should stay untouched.

A basic guardian was also defined. In addition, default translations are handled. Old flags will use translations, but at this point, admin defined flags will not be translated.

@github-actions github-actions bot added the i18n PRs which update English locale files or i18n related code label May 9, 2024
Copy link
Contributor

@martin-brennan martin-brennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few initial comments, will leave it there for now though.

app/models/post_action_type.rb Outdated Show resolved Hide resolved
app/models/post_action_type.rb Outdated Show resolved Hide resolved
app/models/post_flag.rb Outdated Show resolved Hide resolved
app/models/post_flag.rb Outdated Show resolved Hide resolved
app/models/post_flag.rb Outdated Show resolved Hide resolved
app/models/post_flag.rb Outdated Show resolved Hide resolved
config/locales/client.en.yml Outdated Show resolved Hide resolved
@lis2 lis2 force-pushed the custom-flags branch 3 times, most recently from 286d8ad to b0161a0 Compare May 13, 2024 05:39
@github-actions github-actions bot added the chat PRs which include a change to Chat plugin label May 13, 2024
@lis2 lis2 marked this pull request as ready for review May 14, 2024 03:07
@lis2 lis2 requested a review from martin-brennan May 14, 2024 03:07
Copy link
Contributor

@martin-brennan martin-brennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay there is a lot here, but we are building the foundation :) Sorry if some of this is a bit unclear, it's been a long day, please let me know if you need clarification on anything. The main thing I am concerned about is the various different "things" each flag can be used against (post, topic, chat message) and how the existing legacy post action type and reviewable score records tie into all of this.

app/models/flag.rb Show resolved Hide resolved
app/models/post_action_type.rb Outdated Show resolved Hide resolved
db/migrate/20240423054323_create_flags.rb Show resolved Hide resolved
app/models/flag.rb Show resolved Hide resolved
spec/lib/guardian/flag_guardian_spec.rb Outdated Show resolved Hide resolved
db/migrate/20240423054323_create_flags.rb Show resolved Hide resolved
db/migrate/20240423054323_create_flags.rb Show resolved Hide resolved
app/models/flag.rb Show resolved Hide resolved
db/fixtures/003_flags.rb Show resolved Hide resolved
@martin-brennan
Copy link
Contributor

OK re-evaluating based on your internal comments and guidance on the _type columns Kris, I think the main change we need to make here is to:

  • Add an applies_to column which would be either an array type or just a simple string type with a separator like Post|Topic|Chat::Message
  • Remove the topic_type since it is covered by the above
  • Do the name/name_key changes suggested, I think that will help us split the translation out and it will be good to cook the name key underscore replacement stuff on create
  • Add a description column

Then also just the few other suggestions here that are more minor then it can be good to go.

@lis2
Copy link
Contributor Author

lis2 commented May 16, 2024

@martin-brennan thank you for the review. Everything should be fixed 🤞

Copy link
Contributor

@martin-brennan martin-brennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good foundation Kris, thanks for taking the time to address review comments :)

app/models/flag.rb Outdated Show resolved Hide resolved
@lis2 lis2 merged commit 7aff980 into discourse:main May 21, 2024
16 checks passed
@lis2 lis2 deleted the custom-flags branch May 21, 2024 03:15
lis2 added a commit that referenced this pull request May 21, 2024
lis2 added a commit that referenced this pull request May 21, 2024
CvX pushed a commit that referenced this pull request May 21, 2024
This is preparation for a feature that will allow admins to define their custom flags. Current behaviour should stay untouched.
CvX pushed a commit that referenced this pull request May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat PRs which include a change to Chat plugin i18n PRs which update English locale files or i18n related code
2 participants