HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.
Enhancements and Feedback
jackmaessen
✭✭✭
in Feedback
Place for feedback and enhancements
0
Comments
I like that plugin, but the way it is done has some drawbacks:
By giving each option a unique name, you a) are limited to 10 options and b) have to repeat your code quite often. Using an array would be more useful.
You are limiting participation on IP addresses. That way I would be able to vote from my laptop at home, from my desktop pc at work, from my mobile phone, from the internet cafe, etc.
You are not using Vanillas form class.
The markup you are using is not the standard panel box styling which makes it look strange.
The voting logic is in a view.
And there are three reasons why your plugin might not work (in fact it doesn't work for me).
1. File permissions. You try to write to a file. I do not know if it would end up in the root folder of Vanilla or the views folder, but on my server, the web server is only allowed to write to /cache, /config and /uploads.
2. The js makes a direct call to
/plugins/QuickPoll/views/quickpoll_vote.php. On my server, the only executable php file is called index.php, so your script isn't executed at all.3. Sometimes people use subfolders "domain.com/forum". I guess your plugin wouldn't work for them.
You should use more of the framework functions that are available.
Use
public function pluginController_quickVote_create($sender)as an endpoint for votes in your plugin. Create a table for votes or at least a column in the User table. Forget about IP addresses and identify users by their UserID.Think about making your plugin customizable through a settingsscreen where you could specify "Option","Option","Option","Option". That way you could even allow mods to change the polls. Otherwise it can only be changed from someone with file access on your server.
Thanks @R_J for that answer. I was already hoping for a feedback like that. Because i do not know to handle some things (efficiency and Vanilla structure). So i posted it in the hope for a feedback like this.
Please can you give me a push in the right position about it you write:
How can i work more efficient with an array?
Yes i totally agree it is better to trigger un UserID rather then IP address.
And it is better i think to put the data in DB instead of flat file. Need also some help with it.
And i passed the structure of Vanilla because i did not really know how to handle with that in a correct way.
So what i really need is a directive to make the plugin correctly
Yes, and you don't want to store your results in a text file either. From your instructions it looks like the ipaddresses can just be downloaded by anyone if they know the location of the text file. and put it in the url.
Look at Discussion Polls plugin. Tear it apart until you understand it. Take some basic ideas from it and learn how to put it in a panel and not require a discussion and write it with your own code. Open and closing poll would be handy as well. probably some sort of caching would be handy so you don't do a sql lookup on every page load, if you store in db.
and if you exapnd it make sure any user input is validated and all output is sanitized to prevent any security issues.
Pragmatism is all I have to offer. Avoiding the sidelines and providing centerline pro-tips.
You can start with that:
public function setup() { // Initialize some config settings so that you can use them later. touchConfig('quickPoll.Options', 'Chrome,FireFox,Internet Explorer,Opera'); touchConfig('quickPoll.Delimeter', ','); /* // That way you can use it later like that: $options = explode( c('quickPoll.Delimeter', ','), c('quickPoll.Options', '') ); */ $this->structure(); } public function structure() { // Create a table for the poll. If you make the user id the primary key, // you will prevent duplicate entries for users. Gdn::structure()->table('QuickPoll') ->primaryKey('QuickPollUserID', 'int(11)') ->column('Option', 'int') ->set(); } // Make your plugin do a request to /plugin/quickpoll/ID public function pluginController_quickPoll_create($sender, $args) { // $args[1] should be ID, check it like that: decho($args); Gdn::sql()->replace( 'QuickPoll', ['Option' => intval($args[1])], ['QuickPollUserID' => Gdn::session()->UserID] ); } // A call to settings/quickpollclear would clear the polls. // You should check for permissions! public function settingsController_quickPollClear_create($sender) { $sender->permission('Garden.Settings.Manage'); Gdn::sql()->truncate('QuickPoll'); }This is the database side of the plugin.
I forgot that your poll had the possibility to either accept one answer only or more than one. If you set the user ID in the database to be a primary key, you wouldn't be able to accept more than one question. You should change "primaryKey()" to
->column('QuickPollUserID', 'int(11)', false, 'key'). The value that you store in the database, by the way, would be the index of the answer in the answer array.$options[0] = first element in the array, $options[1] is the second and so forth. So if a user chooses "Opera" the index should be stored into the db. That means, that you shouldn't insert elements into your options afterwards or change their order!
In order to handle polls with multiple answers correctly, you should always do a DELETE * FROM QuickPoll WHERE QuickPollUserID = SessionUserID (see the file class.sqldriver.php for the method delete()) and afterwards insert all answers. You should add a config setting quickPoll.MultipleAnswersAlled = true/false which would be checked before an answer is written to database.
Now for the form part.
At first, look at the markup of a module you see in the forum. It normally has that form (for lists):
<div class="Box BoxModuleName"> <h4>Title, if needed</h4> <ul class="ModuleName"> <li class="OnlyIfYouWant/Need"> Some Content </li> <li class="OnlyIfYouWant/Need"> Content! Content! Content! </li> ... </ul> </div>In your modules toString method, you should use the form builder. Look at class.form.php what it has to say to check boxes and radio lists. You will find that there is not only a method radio() and a method checkBox, which would create one form element, there also is radioList and checkBoxList!
See what they can do for you:
public function pluginController_abracadabra_create() { $form = new Gdn_Form(); $options = ['FireFox', 'Opera']; echo $form->checkBoxList('Multi', $options); echo $form->radioList('Single', $options); }Now go to yourform/plugin/abracadabra - great, isn't it? The checkBoxList already is a list, only the radioList is not. But wait, have you really taken a look at radioList() in class.form.php? If not, do so! Really, it is for your very best. There is some
<ul>code in there. Look at the code and see what is needed to make radioList create an unordered list. It is not hard, just try for yourself!There is one thing left, though. When you look at the markup of what is created, you'll find that the values for the checkboxes are the texts, but for the radio buttons those values are the indexes. This is some inconsistency, but hey: no one is perfect!
It would be great if the result would always be the same. By looking at the code alone, it is hard to figure out what to do, so I'll tell you: if you pass an array of "text" => "value" to checkBoxList it would use "text" for the display and "value" as the value. Insert those both lines in our test function abracadabra:
See what happens? Yes, exactly! That way we end up with
public function pluginController_abracadabra_create() { $form = new Gdn_Form(); $options = ['FireFox', 'Opera']; echo $form->checkBoxList('Multi', array_flip($options)); echo $form->radioList('Single', $options, ['list' => true]); }This is all you have to do in order to get a nice form!
The form tags are missing, though ;-)
Again, browse through class.form.php and see if you find what you need. Maybe you already found that when looking for examples in Vanilla.
It would be as easy as echo
$form->open();and$form->close();. While you can use$form->close('Vote/View');, I prefer making it more explicit by using$form->button('Vote/View');and afterwards$form->close();. You could also add an onClick action$form->button('Vote/View', ['onClick' => 'whatever()']);(but I remember that I've read somewhere that this would be bad practice and script actions should be bound to form elements with a js call - I don't really know/care about that)Are you following my advice to look at Vanillas source? Then you might have stumbled upon the open() methods comment:
* @param array $Attributes An associative array of attributes for the form tag. Here is a list of * "special" attributes and their default values: * * Attribute Options Default * ---------------------------------------- * method get,post post * action [any url] [The current url] * ajax TRUE,FALSE FALSE * * @return string * * @todo check that missing DataObject parameter */ public function open($Attributes = array()) {AJAX! Yeah, there is something in there about that, so use
$form->open(['action' => url('plugin/quickpoll'), 'ajax' => true]);aaaaaaand: nothing. MAybe you have to add the js filejquery.gardenhandleajaxform.jsto it? But looking at that comment in thereThe form must be within an element with the "AjaxForm" class.tells us, we should wrap the form inside a div.AjaxForm. Now do so! Anything? Well, please tell me if you were successful and how you did. That's nothing that I really managed by now ;-)