Git Merge Request #295: #8230 Interactive checkboxes (merged)

Merging...

Merged

Something went wrong. Please, merge manually

Checking if merge is possible...

Something went wrong. Please, merge manually

Shalitha Suranga wants to merge 15 commits from /u/shalithasuranga/allura/ to master, 2018-10-15

This is for [#8230]

Commit Date  
[056165] (checkboxint) by Shalitha Shalitha

[#8230] Add unit tests for ForgeGit get_markdown and update_markdown

2018-10-13 15:30:45 Tree
[9ee795] by Shalitha Shalitha

[#8230] Add interactive checkbox support for ForgeGit merge requests

2018-10-13 15:00:35 Tree
[232d38] by Shalitha Shalitha

[#8230] Clean up codes and revert permission check for comments

2018-10-13 12:14:32 Tree
[c9cb48] by Shalitha Suranga Shalitha Suranga

[#8230] Add unit tests for get_markdown and update_markdown

2018-10-12 10:02:52 Tree
[4962fd] by Shalitha Suranga Shalitha Suranga

[#8230] Move new Checklists snippet to common place

2018-10-12 05:45:17 Tree
[bc0b40] by Shalitha Suranga Shalitha Suranga

[#8230] Change permission check to edit from moderate

2018-10-12 04:56:03 Tree
[c1040a] by Shalitha Shalitha

[#8230] Add interactive markdown checkbox support for ForgeDiscussion

2018-10-10 16:14:20 Tree
[0ce82f] by Shalitha Shalitha

[#8230] Update thread edit info when markdown changes

2018-10-10 16:13:19 Tree
[395032] by Shalitha Suranga Shalitha Suranga

[#8230] Add interactive checkbox support for general threads

2018-10-10 10:16:09 Tree
[c0ccce] by Shalitha Suranga Shalitha Suranga

[#8230] Fix checklist.js for multiple elements

2018-10-10 10:14:40 Tree
[adaa83] by Shalitha Suranga Shalitha Suranga

[#8230] Add interactive checkbox support for ForgeTracker

2018-10-10 07:57:28 Tree
[60af37] by Shalitha Suranga Shalitha Suranga

[#8230] Add interactive checkbox support for ForgeBlog

2018-10-10 06:30:51 Tree
[dd6186] by Shalitha Suranga Shalitha Suranga

[#8230] Update LICENSE for checklist.js

2018-10-10 05:02:03 Tree
[fb9e1f] by Shalitha Shalitha

[#8230] Add interactive checklist support for ForgeWiki

2018-10-09 16:21:14 Tree
[d48428] by Shalitha Shalitha

[#8230] Add checklist.js

2018-10-09 16:20:33 Tree

Discussion

  • Dave Brondsema

    Dave Brondsema - 2018-10-09

    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.

     
  • Shalitha Suranga

    Hi.. Dave

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

     
  • Shalitha Suranga

    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

     

    Last edit: Shalitha Suranga 2018-10-10
  • Dave Brondsema

    Dave Brondsema - 2018-10-11

    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!

     
  • Shalitha Suranga

    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

     
  • Dave Brondsema

    Dave Brondsema - 2018-10-12

    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.
     
  • Shalitha Suranga

    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

     
  • Shalitha Suranga

    • Summary: #8230 Interactive checkboxes - Updating --> #8230 Interactive checkboxes

    • Description:

    Diff:

    --- old
    +++ new
    @@ -1,3 +1,3 @@
    -I completed [#8230] for ForgeWiki Module. And will push for other modules too. Eventually we can review and merge 
    +This is for [#8230]
    
     

    Related

    Tickets: #8230

  • Shalitha Suranga

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

    Thanks

     
  • Dave Brondsema

    Dave Brondsema - 2018-10-15

    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);
    
     
  • Shalitha Suranga

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

     
  • Dave Brondsema

    Dave Brondsema - 2018-10-15
    • Status: open --> merged
     

Log in to post a comment.