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

Move posts to polls #73

Merged
merged 12 commits into from
Jul 12, 2023
Merged

Move posts to polls #73

merged 12 commits into from
Jul 12, 2023

Conversation

dsevillamartin
Copy link
Member

@dsevillamartin dsevillamartin commented Jul 6, 2023

🔴 THIS PR IS NOT BACKWARDS COMPATIBLE. DO NOT TEST IN PRODUCTION. 🔴

Fixes #67

Changes proposed in this pull request:
Intending on this to be released as v2.0.0 so many API breaking changes & backwards compatibility removed alongside this. Those changes will be made after merging this.

  • Change polls to have an associated post instead of discussion
    • Migration takes all polls and updates their discussion_id to the discussion's first_post_id or post with number = 1 (if first post is deleted, there can still be a first post where the poll is currently appearing)
    • Relationship is hasMany for easier changes in future to allow for multiple posts
  • Rename permissions & allow tag scoping of start, view results, & voting
    • Start discussion composer can't check whether user has permission to start in selected tags, which isn't ideal. Easiest solution is moving poll creation flow completely to after the discussion is created.
  • Create API call to retrieve one poll at /fof/polls/{id}
  • Editing poll doesn't require page refresh to update

Reviewers should focus on:

  • I don't like deleting the polls that couldn't have an associated post found. That's the scariest part of this. But I don't know what else would be done with those polls; they would never be shown except off API calls.
  • Still showing the discussion badge in case the first post has a discussion. Not sure what other functionality that should appear on.

Screenshot

Details

image
image
image
image
image

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

->update(['polls.discussion_id' => $db->raw('first_posts.id')]);

// Delete polls that don't have an associated post
// TODO Is this a good idea? Not sure what situations can result in first_post_id being null but the discussion still existing with an associated first post.
Copy link
Member Author

Choose a reason for hiding this comment

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

The previous two updates should take care of

  • existing first post relation through first_post_id
  • post with number = 1

I'm very much not an SQL expert, and I have barely touched joins myself. I don't trust myself to merge this without a lot of testing done to avoid polls disappearing that shouldn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

From further testing, the second scenario (no first post relation but post with number ID 1) cannot be achieved simply by deleting the first post in the discussion. If there are other posts, they retain their numbering (i.e. post numbering starts at 2). Extensions such as fof/merge-discussions and others renumber posts, in which case the poll would reappear (in v1).

I don't have a problem with keeping that scenario. Just providing some more info, since a poll is currently only shown for post number #1.

migrations/2023_07_05_000000_alter_polls_use_post_id.php Outdated Show resolved Hide resolved
@dsevillamartin dsevillamartin changed the base branch from master to v2 July 6, 2023 14:17
@dsevillamartin dsevillamartin marked this pull request as ready for review July 12, 2023 15:19
@dsevillamartin dsevillamartin merged commit 1d54e7d into v2 Jul 12, 2023
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.

Add polls in a post
2 participants