Very nice plugin!

R_J

I think this could be very useful to a lot of forums.

Looking at the source, I've got some remarks. Please use YourPlugin\locale\en.php for language files. This is the only accepted naming convention in Vanilla 2.2. I know you are using 2.1 and so would not see any difference - especially because it only holds terms of another plugin

As far as I can see there is no check if the session user has access rights to the discussions category. You assume that everyone with DiscussionNote.Add permission also has access to the categories in Plugins.DiscussionNote.CategoryNums. It would make your plugin more universally usable if you add that PermissionCategory check. I guess you try to get around that with your $Simplekey but there are better ways to ensure that people only d the things they are allowed to.

Did you know that every table in Vanilla where it makes sense has a column called "Attributes" which is able to store different information in a serialized format? As long as you do not want to store information that should be used directly in a sql statement (sort by, where etc.), you should consider using that column. It is of type Text, so it holds 65k characters. Room enough for your 200 chars. Every WhateverModel extends Gdn_Model and that holds a useful function: saveToSerializedColumn(). So you could use $discussionModel->saveToSerializedColumn('Attributes', 'DiscussionNote', $Note).

rbrahmson

Thank you @R_J, very insightful and as far as I remember from scanning other discussions, it is a rare (or relatively rare) positive code review for a plugin developer - very insightful and useful. I hope more would follow your lead as I release to the public plugins we've been using in our intranet.

1. Will fix the locale - thanks!
2. Category access rights - yea, simplekey indeed goes around the problem but I introduced it to prevent others figuring out that the controller url contains the discussion number to be updated and decided to add a verification variable. Turns out this also protect against the category access rights issue... You suggested better other ways - care to share?
3. I saw the "attributes" but I intentionally used a separate column so I could filter on it with the FilterDiscussion plugin (we use that plugin extensively).
4. It crossed my mind that while one user edits the postit note another could be doing the same. I should there a way to warn the second user? If not, which update would be saved? Is there a locking mechanism in Vanilla? Is it up to the plugi developer to handle this complexity?
5. I was thinking of using this plugin code base to implement the plugin that updates the prefix of a discussion (using the discussion prefix plugin). But as it now stands the prefix is embedded in the title. How should I go about separating it (which should be done in the PrefixDiscussion plugin) while the rest implemented in the plugin based on this one?

Looking forward for your feedback (especially #2, #4, #5).
Thanks !!!

R_J

Number 2:

// You already did that in your code.
$DiscussionID = (int)$Args[0];
$DiscussionModel = new DiscussionModel();
$Discussion = $DiscussionModel->GetID($DiscussionID);

// That's new:
if(!Gdn::Session()->CheckPermission('Vanilla.Discussions.Edit', true, 'Category', $Discussion->PermissionCategoryID)) {
    // Stop if session user is not allowed to edit discussions in the category
    // of the discussion that has been specified.
    throw PermissionException('Vanilla.Discussions.Edit');
}

Number 4:
Get current note before user1 starts editing/adding/removing a postnote. Before saving it, get the current note again and present user1 with a dialogue if the post note differs.
Or add a new table: DiscussionNoteID | DiscussionID | InsertUserID | Note | Timestamp or something like that so that you get a history and you will be able to loop through the entries.
But don't you think this is overkill?

Number 5:
Not quite sure if I understand you. But if you want to change parts of the HTML without a refresh, you have to use JavaScript.

And yes, if you need to filter based on some entries, the Attributes column isn't the right choice. (Number 3)

rbrahmson

4-indeed overkill - I was just checking out whether there is a built-in function to handle this.

5-just wondering how to coordinate development of two plugins one of which is yours...

2-last but not least - thanks!

phreak

(It's simple joy to listen to two developers improving a plugin which can be super-helpful for community managers... hach, what a wonderful world)

rbrahmson

@R_J: Implemented a variation of the category permissions and already uploaded the new version. Tnx!
@phreak: Thanks for the kind words. You make the effort worthwhile!

R_J

Try this for your settings screen:

// Get all categories.
$categories = CategoryModel::categories();
// Remove the "root" categorie from the list.
unset($categories[-1]);

// Create config module.
$configurationModule = new ConfigurationModule($sender);

// Now let the admin choose from a nice list.
$configurationModule->initialize(
  array(
    'Plugins.DiscussionNote.CategoryNums' => array(
      'Control' => 'CheckBoxList',
      'LabelCode' => 'Limit Notes to discussions in specific categories',
      'Items' => $categories,
      'Description' => 'Bla, Bla, bla.',
      'Options' => array('ValueField' => 'CategoryID', 'TextField' => 'Name')
    )
  )
);
$configurationModule->renderAll();

---

Number 5:

I see now: you would like to write something like an "UpdateDiscussionPrefix" plugin, correct? And you thought about using the popup form/js update of this plugin as a blueprint. Okay, I think I got it now.

You would have to use JavaScript. You should be able to get the prefix with a selector like that: $('li#Discussion_' + discussionID + '>span.PrefixDiscussion') and thus you should be able to change it. I would include the JavaScript in the popup view and insert the DiscussionID right there:

echo '
<script type="text/javascript">
  ...
  $("li#Discussion_'.$DiscussionID.'>span.PrefixDiscussion").html("'.$newPrefix.'");
  ...
</script>
';

Don't forget to remove the old prefix class and set the new one.

rbrahmson

Hi @R_J, Thanks again. I'm out of the office in the next few days so I won't deal with it soon, but from looking at the above code I see it's a cool way to handle the categories!

Re #5: Yes, that's the concept, but I have one more change to the PrefixDiscussion plugin -- the one I suggested in a comment to the FilterDiscussion plugin (suggesting code change to PrefixDiscussion). I'd like to incorporate them all, and in a way not convinced that the right approach should not be to embed it all in PrefixDiscussion... What do you think?

R_J

Just make a pull request on GitHub and I'll take a look at it.
In my opinion, changing the prefix is like editing the discussion and if a user has the right to edit the discussion, he can already change the prefix. So I do not really see a use case for that feature.

rbrahmson

Maybe I am misunderstood. The two changes I like are:
1. Allow users (who are allowed to edit the discussion) to change the prefix from the discussion list. Not sure popup rather than dropdown is preferable but it may given #2 below.
2. Separate the prefix from the discussion title so that it could be connected to a different url -the one I suggest is a filter to show all discussions with the same prefix. The filter itself implementable via the PrefixDiscussion plugin.

In fact, I envision a much larger navigational approach - one where within the discussion list, every click on a piece of discussion data that make sense will navigate to a filtered discussion list based on what the user clicked. So that would work for Prefix, author/last updater (instead of link to the profile), like it already does for the category. Again, the actual filtering done via the FilterDiscussion plugin that can filter on any piece of discussion data via url parameter.

R_J

Maybe you should consider tags. If you set up one plugin that does some automatic tagging based on anything you like, you would be able to use the tagging plugin to do all the filtering for you.

As far as I know, the plugins are called in an alphabetical order. So if you name your plugin ZAnything, it will be called at last. And if you hook into discussion models BeforeSave, you would be able to set a tag based on the fields you are interested in.

rbrahmson

@R_J said:
Try this for your settings screen:

$configurationModule->initialize(
      array(
        'Plugins.DiscussionNote.CategoryNums' => array(
          'Control' => 'CheckBoxList',
          'LabelCode' => 'Limit Notes to discussions in specific categories',
          'Items' => $categories,
          'Description' => 'Bla, Bla, bla.',
          'Options' => array('ValueField' => 'CategoryID', 'TextField' => 'Name')`

Thanks! Great neat piece of code. I incorporated it into the new version that added the ability to display the note within the discussion list and the post.

hgtonight

Sweet icon, btw.

rbrahmson

Thanks! I couldn't have done it without you, @R_J, and the great support in this forum!

rbrahmson

@R_J said:

Try this for your settings screen:

$configurationModule->initialize(
      array(
        'Plugins.DiscussionNote.CategoryNums' => array(
          'Control' => 'CheckBoxList',
          'LabelCode' => 'Limit Notes to discussions in specific categories',
          'Items' => $categories,
          'Description' => 'Bla, Bla, bla.',
          'Options' => array('ValueField' => 'CategoryID', 'TextField' => 'Name')`

Hi R_J. A while ago I incorporated the above suggestion in few of my plugins and it worked perfectly. Alas, while testing 2.3b1 I discovered that saving arrays into config is no longer allowed (by design, I learned from @Linc). So obviously I am toying with the idea of saving the info in other formats (comma delimited lists, etc.). However, the plugin configuration settings is doing the saving internally and changing the Options field above will alter the display which is the entire point.

Sure, I can create a form and manage the entire setting myself, but I really like the elegance of using the configuration model managing the entire process.

Any thoughts, suggestions?

R_J

I'm already preparing an answer to your issue on GitHub. Just let me some more time to think about that.

What first comes to my mind is: either disallow CheckBoxList in configurationModule or extend saveToConfig() or c() to accept arrays. The result of a checkboxlist will always be an array and as such it really makes sense to have at least an agreed upon format for storing it in config, which means it would make sense to store arrays, which will make disallowing arrays useless. I hope I can point that out more understandable, but I really have no time right now...

rbrahmson

Thanks @R_J. I added more feedback to my issue on github. My old background came from an environment that backward compatibility was sacred...

R_J

@rbrahmson said:
@R_J said:
Hi R_J. A while ago I incorporated the above suggestion in few of my plugins and it worked perfectly. Alas, while testing 2.3b1 I discovered that saving arrays into config is no longer allowed (by design, I learned from @Linc). So obviously I am toying with the idea of saving the info in other formats (comma delimited lists, etc.). However, the plugin configuration settings is doing the saving internally and changing the Options field above will alter the display which is the entire point.

Sure, I can create a form and manage the entire setting myself, but I really like the elegance of using the configuration model managing the entire process.

Any thoughts, suggestions?

To be honest: I'm not sure how a custom settings view can be avoided. I know ways to change a value before it is saved to config, but I do not know how to change a config setting after is has been read from the config and before it is displayed in the form by the configurationModule.

Maybe @bleistivt has some insights on this. I think he uses the configurationModule and configurationModel much more versatile than I do:

https://vanillaforums.org/discussion/comment/233579/#Comment_233579
https://vanillaforums.org/discussion/comment/224540/#Comment_224540

But that config change has broken the configurationModule. To me this is a bug and must be fixed before 2.3 can be released. I don't know if it makes sense to start thinking about alternatives already.