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 polls edit and delete. #1126

Merged

Conversation

gyakkun
Copy link
Contributor

@gyakkun gyakkun commented Feb 12, 2020

Now the button Edit and Delete in Staff/polls page should work.

Add necessary language items in resources/lang for zh-CN, zh-TW and en.

( Note: As I just begin writing php this week, the code may lack exception handling and so on. Serious review is needed. )

@HDVinnie
Copy link
Collaborator

@gyakkun Im going to review this tonight when home. I do think however this whole polls system needs to be re-written from scratch which I plan to do. It was poorly designed and made. Possible refactor might suffice. Ill keep you updated. Thanks again for the contributions and look forward to your translation (i18n) pull requests.

@gyakkun gyakkun force-pushed the patch_impl-poll-edit-delete branch from bef57c1 to b46107b Compare February 12, 2020 19:11
@gyakkun gyakkun force-pushed the patch_impl-poll-edit-delete branch from b46107b to 089fad5 Compare February 12, 2020 19:18
@HDVinnie
Copy link
Collaborator

@gyakkun what's your thoughts on removing IPs altogether in this PR. See this issue it would close for ref. #1114

Changes to be committed:
	new file:   2020_02_14_185120_add_foreign_key_to_options_table.php

(For search: migration script.)
@gyakkun
Copy link
Contributor Author

gyakkun commented Feb 14, 2020

@gyakkun what's your thoughts on removing IPs altogether in this PR. See this issue it would close for ref. #1114

I share my opinion of removing ip_check with @HVRVKVT in #1114 . It's better to be done in this PR #1130 .

@gyakkun
Copy link
Contributor Author

gyakkun commented Feb 14, 2020

BTW I reckon it as a must for a private tracker to record user's ip, for auditing and cheat check.

The approch we adpot on a running PT in my organization is, adding an independent ip logging table.

Still, the ip logging should be configurable if the staff want the site running public.

@HDVinnie
Copy link
Collaborator

BTW I reckon it as a must for a private tracker to record user's ip, for auditing and cheat check.

The approch we adpot on a running PT in my organization is, adding an independent ip logging table.

Still, the ip logging should be configurable if the staff want the site running public.

IP Logging is 99% inefficient. I don't think its worth logging users IP addresses to catch the few that are to stupid or don't care enough to use a VPN...

@gyakkun
Copy link
Contributor Author

gyakkun commented Feb 14, 2020

@gyakkun what's your thoughts on removing IPs altogether in this PR. See this issue it would close for ref. #1114

Removed in 957ac57 and 4199867 .

As it's fomerly a configurable option, remove it or not are both acceptable. All up to you and feel free to skip with revert or cherry pick.

@HDVinnie
Copy link
Collaborator

@gyakkun what's your thoughts on removing IPs altogether in this PR. See this issue it would close for ref. #1114

Removed in 957ac57 and 4199867 .

As it's fomerly a configurable option, remove it or not are both acceptable. All up to you and feel free to skip with revert or cherry pick.

I feel its best to fully remove the option. You agree?

@gyakkun
Copy link
Contributor Author

gyakkun commented Feb 14, 2020

@gyakkun what's your thoughts on removing IPs altogether in this PR. See this issue it would close for ref. #1114

Removed in 957ac57 and 4199867 .
As it's fomerly a configurable option, remove it or not are both acceptable. All up to you and feel free to skip with revert or cherry pick.

I feel its best to fully remove the option. You agree?

Agree.

@gyakkun
Copy link
Contributor Author

gyakkun commented Feb 14, 2020

@gyakkun what's your thoughts on removing IPs altogether in this PR. See this issue it would close for ref. #1114

Removed in 957ac57 and 4199867 .
As it's fomerly a configurable option, remove it or not are both acceptable. All up to you and feel free to skip with revert or cherry pick.

I feel its best to fully remove the option. You agree?

What about replace it with "Result is only visible after votes?", it may be more useful.

@HDVinnie
Copy link
Collaborator

@gyakkun what's your thoughts on removing IPs altogether in this PR. See this issue it would close for ref. #1114

Removed in 957ac57 and 4199867 .
As it's fomerly a configurable option, remove it or not are both acceptable. All up to you and feel free to skip with revert or cherry pick.

I feel its best to fully remove the option. You agree?

What about replace it with "Result is only visible after votes?", it may be more useful.

I think the poll system can indeed use a lot more. Your idea is good. Others like this on polls table would be neat so you can close a poll at a certain date?:

            $table->boolean('closed')->default(false);
            $table->timestamp('closes_at')->nullable()->default(null);

Also on voters table it might be nice to add option_id so on the poll results page we can highlight what the said user voted for. If you know what I mean.

@gyakkun
Copy link
Contributor Author

gyakkun commented Feb 15, 2020

BTW I reckon it as a must for a private tracker to record user's ip, for auditing and cheat check.
The approch we adpot on a running PT in my organization is, adding an independent ip logging table.
Still, the ip logging should be configurable if the staff want the site running public.

IP Logging is 99% inefficient. I don't think its worth logging users IP addresses to catch the few that are to stupid or don't care enough to use a VPN...

I would better explain the necessity in a new open issue #1151. I think it's something more important than efficiency.

@gyakkun
Copy link
Contributor Author

gyakkun commented Feb 15, 2020

I think the poll system can indeed use a lot more. Your idea is good. Others like this on polls table would be neat so you can close a poll at a certain date?:

            $table->boolean('closed')->default(false);
            $table->timestamp('closes_at')->nullable()->default(null);

Also on voters table it might be nice to add option_id so on the poll results page we can highlight what the said user voted for. If you know what I mean.

So that can the style add to result page and make the voted option highlight according to the option_id , right? That sounds really nice. It would better be another story in another PR. Hope my PRs on poll feature won't bother your refactor plan, and I will move my hands off from it since I'm not so experienced 😂

Now things have all been done to remove ip_checking, and #1130 can be closed.

@HDInnovations HDInnovations deleted a comment from torman365 Feb 15, 2020
@HDVinnie HDVinnie merged commit d393ac7 into HDInnovations:development Feb 16, 2020
@gyakkun gyakkun deleted the patch_impl-poll-edit-delete branch February 21, 2020 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants