-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
More features & breaking v2 changes #74
Conversation
@@ -34,6 +34,7 @@ | |||
new Extend\Locales(__DIR__.'/resources/locale'), | |||
|
|||
(new Extend\Routes('api')) | |||
->post('/fof/polls', 'fof.polls.create', Controllers\CreatePollController::class) |
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.
Should this maybe be /posts/{id}/poll
instead ? It makes more sense for how the post ID is provided to the backend. 🤷
Adding/removing poll options still doesn't work, but that functionality is fixed in the follow-up PR #74
86e8594
to
7dab87b
Compare
Hello ! All the issues I reported on Flarum have been fixed. Congratulations ! 👍 Unfortunately, I detected more bugs.... But I can see the light at the end of the tunnel ! These are bugs linked to the "end date" option. How can I explain it? On a discussion or a post, you create the first poll with an end date. It works 👍 However, if you add another poll in the same post.... If you don't set an end date, it still creates a date automatically.... It's a bug was hard for me to understand and reproduce, so I hope that I made clear :) Also, It's not possible to remove the end date on any survey if you edit poll. Thank |
@Wlork Hm, I wasn't able to reproduce this... I created a poll with an end date, then on that same post, created another poll with no end date set. And it worked as intended - first poll has end date, second doesn't.
This is probably from my changes in #72. I need to check if v1 allowed you to remove an end date - there were already checks so that you couldn't make it sooner than it was set so I guess I assumed it didn't. |
I forgot to mention that you need to activate the multiple choice option. Sorry about that !
I think it's good to be able to delete if needed :) Like the other options where we can enable/disable |
@Wlork I still can't reproduce this issue. Can you post a recording (gif or video upload works here on GitHub) showcasing the issue?
I believe I have now reverted it back to the behavior in v1. Haven't built JS for it (too many commits with that 😅) but I'm not worried about that. |
Hello @dsevillamartin :) I don't have this bug anymore. Or maybe I just can't reproduce it 😅
I'm not sure if I've understood you correctly, but I still can't cancel existing poll's end date :( After the last update, I was able to run several tests. I detected another bug. As a member : 1- Create a discussion or a post with a poll Apart from these two bugs, everything seems to be running smoothly on my side. Once you've corrected these issues, I'll redo a complete test from A to Z :) |
I didn't build the JS assets for it, so unless you do it in your vendor folder, it won't be fixed for you. I'm not worried about it still being a bug though.
I've made it so that you need to be able to edit a post to edit and add polls. I feel like this makes sense, as polls are a part of the post. So this is intended behavior on my part. You have it set so you can no longer edit a post once it's been replied to, and that should (in my opinion) extend to its polls. The butttons being gone is good - them not being gone beforehand is something that can't really be helped. The Flarum post "edit" button remains until refresh as well, even though the user no longer has the permission to edit that post. What I'm wondering now is whether I took the "edit own polls" permission into consideration during this. I may add add a new permission for editing polls even if the user can no longer edit the post itself. |
OK, I understand :)
I just checked and I didn't give permission to edit their posts in my forum test.... That's why I had this "issue". I changed it and it's fixed 👍 I'm sorry :( Good.... We'll have to see how it works out in practice, but as far as I'm concerned, everything's OK :) |
@Wlork I've split permissions for poll editing for more control (polls you create vs polls others create on your posts). And built the JS. There's still some bugs and features that would be helpful, but I think this is good for an initial v2 release. If you're planning on re-testing everything, let me know & I'll wait. Otherwise I think I'm ready to merge & release #73 and this. Thank you for all your testing again. |
Hello @dsevillamartin I did all the tests from scratch :) I can delete the end date when I edit a poll. The bug has disappeared 👍 The bug was probably due to the numerous updates.... Apart from that, I haven't found any major bugs :) Just : I saw this bug once. It appears when you add a poll on an existing post with at least one poll. But it's very random. I don't know when exactly this bug appears.... We'll have to see in practice with several users, I hope !
Thank you for taking the time to develop these features ! I'll say it again, it's a really good extension you've developed. If you need any further tests, don't hesitate to let me know 👍 |
Adding/removing poll options still doesn't work, but that functionality is fixed in the follow-up PR #74
27e2daa
to
ed7ab24
Compare
* Move poll relation from one per discussion to many per post * Rename permissions & add tag scoping to start, view results, and vote * Change error message to explicitly state the user cannot start a poll * Improve migrations - permissions delete on down, log deleted poll IDs * Split discussion->post migrations to avoid half-ran state if migration fails within DB transaction * Check all poll attributes in subtree check Adding/removing poll options still doesn't work, but that functionality is fixed in the follow-up PR #74 * Always return 'voteCount' field in API response to properly update in frontend
810e3ed
to
c9e41b4
Compare
🔴 THIS PR IS NOT BACKWARDS COMPATIBLE. DO NOT TEST IN PRODUCTION. 🔴
This PR is intended to be merged after #73. Will need to change base to v2 & rebased.
Fixes #37
Fixes #38
Fixes #16
Changes proposed in this pull request:
PollSerializer
Reviewers should focus on:
Screenshot
![image](https://private-user-images.githubusercontent.com/6401250/251563311-52c896b3-94ad-4bbf-9aed-13fe78bf87a0.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYwNjM2MDIsIm5iZiI6MTcyNjA2MzMwMiwicGF0aCI6Ii82NDAxMjUwLzI1MTU2MzMxMS01MmM4OTZiMy05NGFkLTRiYmYtOWFlZC0xM2ZlNzhiZjg3YTAucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkxMSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MTFUMTQwMTQyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NWE3NzQ5ZWJhZDczZDU2NjQ0NTc4NmFjZWFlM2ZhNDI4MWJjMDhhN2MxODZkMzQzNDU1ZTlhODNhNGUzNjZjZiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.LuwkcU0S2JfW9N2kDkeIEnx6_KFmEKAJlws56kMoiFY)
Confirmed
composer test
).