Apache Allura™

One thing we'll need to do when adding a 3rd-party file like checklist.js is track the licensing for it. The Apache Software Foundation and its projects are quite stringent about that, to make sure everything is compliant with Apache Licenses and well documented.

https://github.com/FND/markdown-checklist doesn't say much about which open source license it uses (many have it in a LICENSE or readme file). But I finally found it mentioned as MIT license at https://github.com/FND/markdown-checklist/blob/master/markdown_checklist/init.py#L64

So what you'll need to do is update our LICENSE and Allura/LICENSE files and add a little section to mention that filename and the MIT license like the other sections in that file. And then add it into our rat-excludes.txt file which is for the "release audit tool" which checks licenses of all our files.

Besides that I think this is looking good for just the wiki.

Hi.. Dave

Thanks for guidance. I have pushed a commit for that. Thereafter I will make it happen for other modules too

I added for ForgeWiki, ForgeBlog, ForgeTracker and ForgeDiscussion and nested threads also work. I had to make some fix in checklist.js too. And I didn't check rate limit for those. existing rate_limit will redirect when error. if we use rate limiting probably we need to write another without redirect.

Please advice next steps

I hope you don't mind all the feedback & suggestions. I know its a lot, but this is a fairly big change and it is pretty good so far but want it to work perfectly everywhere and the code to be really good too :)

I think rate_limit is ok and even good to skip for this situation since it should be just a very small change happening.

Instead of modifying checklist.js like this, could the container that we pass into it be the right one? Then checklist.js stays as the original in case we upgrade it.

-        var index = $("ul" + self.checkboxSelector, self.container).index(this);
+        var index = $("ul" + self.checkboxSelector, $(this).closest('.active-md')).index(this);

For comments, you should check for 'edit' permission instead of 'moderate' so normal users that aren't admins or anything. They can edit their own comments so should be able to update their checklists.

We will need new Checklists... code to run on pages that have a comment thread (for example a ticket) even if the user doesn't have permission to edit the ticket. And there is already 4 copies of the new Checklists... code which is many lines long so we really don't want to have that much duplicated. I would recommend moving it to a common place like allura-base.js, and then all the {% if h.has_access checks can go around the class="active-md" part. That way the JS code is on all pages but the markdown is only marked as active when the permissions are ok.

Lastly, it would be good to have some "functional" tests of the new update_markdown and get_markdown functions. They are pretty simple (especially get_markdown) but its good practice and helps us make sure that if we change things in the future that we cannot accidentally break this functionality. It doesn't have to be a complex test, just one for each tool, in the "functional" directory there are lots of existing tests that hit URLs for the tools, and so you should be able to reference those as examples and to add new ones.

Excited to have this soon!

Thanks for the feedback Dave. Okay we will not use rate_limit checking here. I used edit permission in comment section instead moderate persmission. I have moved the common js snippet for Checklist to allura-base.js since we don't want to do repeating code writing.

I have added unit test cases for those new methods and you can see diff here https://forge-allura.apache.org/u/shalithasuranga/allura/ci/c9cb485e2af7370c7cb2c884700b4afb41a7dd99/

I added permission check too for each update_markdown testcases since we are only allowing authorized people to edit markdown (I mean edit status of checkboxes)

Btw. I am not sure if there is an approach to make it happen without changing the checklist.js without additional effort. I will check further.

Thanks

Hi Shalitha. I think we're getting close. The changes you've made are good. Here's just a few more bits of feedback:

• In ForgeTracker/forgetracker/templates/tracker/ticket.html there's an extra } after the endif and that makes the ticket page have a bad layout in Chrome
• a non-admin user (who can edit their comments) still can't do checklists. The class=active-md isn't there. I think I was wrong earlier about post permissions and 'moderate' is correct - that's what is used for the actual edit button on each post.
• merge requests (Allura/allura/templates/repo/merge_request.html) is another place where markdown content is displayed. Do you want to make that work too? It does require a little setup, you have to have a repo initialized, then fork it, then make a merge request.

Hi.. Dave 1 and 2 are okay with last commit (no issues with unit test). Yeah of course this is good for merge requests too. I will check

Thanks

I pushed few commits covering ForgeGit's markdown interactive checklist support and wrote 2 test cases same as previous cases. Please check

Thanks

I think this is all set! So I took a look at the checklist.js modification to see if we could finalize this without changing it. I made it so it made a separate Checklist object for each active-md section. This seems to work in my testing. What do you think?

diff --git Allura/allura/public/nf/js/allura-base.js Allura/allura/public/nf/js/allura-base.js
index ed62d8f39..945d97ead 100644
--- Allura/allura/public/nf/js/allura-base.js
+++ Allura/allura/public/nf/js/allura-base.js
@@ -235,19 +235,22 @@ $(function(){

 // Interactive checkboxes
 $(function(){
-    new Checklists(".active-md", function(checkbox, callback) {
-        var uri = $(checkbox).closest('.active-md').data('markdownlink');
-        $.get(uri + 'get_markdown', callback);
-    }, function(markdown, checkbox, callback) {
-        var uri = $(checkbox).closest('.active-md').data('markdownlink');
-        $.ajax({
-            type: 'post',
-            url: uri + 'update_markdown',
-            data: {
-                'text' : markdown,
-                '_session_id' : $.cookie('_session_id')
-            },
-            success: callback
+    $('.active-md').each(function() {
+        var $active_md = $(this);
+        new Checklists($active_md, function(checkbox, callback) {
+            var uri = $active_md.data('markdownlink');
+            $.get(uri + 'get_markdown', callback);
+        }, function(markdown, checkbox, callback) {
+            var uri = $active_md.data('markdownlink');
+            $.ajax({
+                type: 'post',
+                url: uri + 'update_markdown',
+                data: {
+                    'text' : markdown,
+                    '_session_id' : $.cookie('_session_id')
+                },
+                success: callback
+            });
         });
     });
 });
diff --git Allura/allura/public/nf/js/checklist.js Allura/allura/public/nf/js/checklist.js
index 32642f755..c1edac29d 100644
--- Allura/allura/public/nf/js/checklist.js
+++ Allura/allura/public/nf/js/checklist.js
@@ -32,7 +32,7 @@ var Checklists = (function($) {
     Checklists.prototype.checkboxSelector = "> li > input:checkbox";
     Checklists.prototype.onChange = function(ev, self) {
         var checkbox = $(this).prop("disabled", true);
-        var index = $("ul" + self.checkboxSelector, $(this).closest('.active-md')).index(this);
+        var index = $("ul" + self.checkboxSelector, self.container).index(this);
         var reactivate = function() { checkbox.prop("disabled", false); };
         self.retriever(checkbox, function(markdown) {
             markdown = self.toggleCheckbox(index, markdown);

Nice trick! it is working I also tested. Could you add this commit too.