The admin needs to be able to enable/disable markdown engine as well as configure its option(s). Currently, the only option is tab_width.

as markdown should be a per-user preference, shouldn't enabling/disabling be a user, not admin, preference?

We will also implement that. But, the admin should be able to disable the markdown altogether as some people might not like it.

(In reply to Koosha KM from comment #2) > We will also implement that. But, the admin should be able to disable the > markdown altogether as some people might not like it. just using user-preferences it's possible for an admin to disable markdown altogether; there's no need to duplicate that setting.

You probably mean "Default Preferences". I was misunderstood. What I intend to do is just let an admin enable/disable markdown and, based on that, show a check-box per comment for using markdown. I think it would be better to add a new admin param since we also want to have other options (tab_width) configurable.

Comment on attachment 8426439 [details] [diff] [review] admin_markdown.patch Review of attachment 8426439 [details] [diff] [review]: ----------------------------------------------------------------- ::: Bugzilla/Config/Markdown.pm @@ +12,5 @@ > + > +use Bugzilla::Config::Common; > + > +sub get_param_list { > + my $class = shift; nit: please use 4 hard spaces for indentions in all Perl code files, and 2 spaces for templates. @@ +18,5 @@ > + { > + name => 'enable_markdown', > + type => 'b', > + default => 1, > + }, Going with what glob mentioned, let's omit this param and go with the user preferences route. As glob mentioned you can set the global preferences to off and then disabled which essentially accomplishes the same thing. And we want to have the preference for people to enable/disable per user so that accomplishes two goals. Let's not worry that we only have the tabs parameter here for now as I am sure we will be adding more as we go. ::: template/en/default/admin/params/markdown.html.tmpl @@ +12,5 @@ > + > +[% param_descs = { > + enable_markdown => "Markdown is a text-to-html conversion tool that allows you to write an " _ > + "easy-to-read, easy-to-write plain text format, then convert it to " _ > + "structurally valid HTML. You can enable or disable this feature for comments.", We will move this text to global/setting-descs.none.tmpl and please make it shorter to be consistent with the other descriptions. What I would like to see eventually is a (help) link on the show_bug.cgi page next to the Markdown checkbox when entering a new comment. The help page can be more descriptive about what Markdown is and the different rules for the syntax. We can locate it in the help text pages/markdown.html.tmpl which we will need to create.

So, we don't make tab_width configurable right now, right?

(In reply to Koosha KM from comment #6) > So, we don't make tab_width configurable right now, right? I think that actually is a good idea for now. The more I have researched it seems that once we set the tab width to a value, we will not want to change it later as it will change the way user's markdown will look and they may not be happy about it. How about we make it a constant in Bugzilla/Constants.pm that an admin can manually change if they want to. Values in Constants.pm are changeable but are less encouraging to do so than an admin parameter. dkl

(In reply to David Lawrence [:dkl] from comment #7) > How about we make it a constant in Bugzilla/Constants.pm that an admin can > manually change if they want to. Values in Constants.pm are changeable but > are less encouraging to do so than an admin parameter. Defining a new constant would be also a good idea.

We'll define that constant in a separate bug.

Modified the description to make it more clear.

Comment on attachment 8428268 [details] [diff] [review] patch-v2.1 Review of attachment 8428268 [details] [diff] [review]: ----------------------------------------------------------------- r=dkl. Are we doing this incrementally where this patch will be incorporated into the next larger patch? We will need to figure out how to keep regressions from occurring if we proceed in this manner. ::: Bugzilla/Install.pm @@ +89,4 @@ > bugmail_new_prefix => { options => ['on', 'off'], default => 'on' }, > # 2013-07-26 joshi_sunil@in.com -- Bug 669535 > possible_duplicates => { options => ['on', 'off'], default => 'on' }, > + # 2014-05-24 koosha.khajeh@gmail.com -- Bug 1014164 nit: remove trailing whitespace @@ +89,5 @@ > bugmail_new_prefix => { options => ['on', 'off'], default => 'on' }, > # 2013-07-26 joshi_sunil@in.com -- Bug 669535 > possible_duplicates => { options => ['on', 'off'], default => 'on' }, > + # 2014-05-24 koosha.khajeh@gmail.com -- Bug 1014164 > + use_markdown => {options => ['on', 'off'], default => 'on'}, nit: space after { and before } ::: template/en/default/global/setting-descs.none.tmpl @@ +48,1 @@ > } go ahead and remove the trailing whitespace here as well since it is so close by.

Comment on attachment 8428268 [details] [diff] [review] patch-v2.1 Review of attachment 8428268 [details] [diff] [review]: ----------------------------------------------------------------- ::: Bugzilla/Install.pm @@ +89,4 @@ > bugmail_new_prefix => { options => ['on', 'off'], default => 'on' }, > # 2013-07-26 joshi_sunil@in.com -- Bug 669535 > possible_duplicates => { options => ['on', 'off'], default => 'on' }, > + # 2014-05-24 koosha.khajeh@gmail.com -- Bug 1014164 nit: remove trailing whitespace @@ +89,5 @@ > bugmail_new_prefix => { options => ['on', 'off'], default => 'on' }, > # 2013-07-26 joshi_sunil@in.com -- Bug 669535 > possible_duplicates => { options => ['on', 'off'], default => 'on' }, > + # 2014-05-24 koosha.khajeh@gmail.com -- Bug 1014164 > + use_markdown => {options => ['on', 'off'], default => 'on'}, nit: space after { and before } ::: template/en/default/global/setting-descs.none.tmpl @@ +44,4 @@ > "requestee_cc" => "Automatically add me to the CC list of $terms.bugs I am requested to review", > "bugmail_new_prefix" => "Add 'New:' to subject line of email sent when a new $terms.bug is filed", > "possible_duplicates" => "Display possible duplicates when reporting a new $terms.bug", > + "use_markdown" => "Use Markdown engine when I submit a new $terms.comment", Something I missed earlier is that the same preference will also enable/disable the rendering of other's comments that were written using Markdown. For example, if your preference is 'off', you will not see the markdown checkbox for any new comments you make, and other user's comments will display as plain text. So this should be worded better as: "Use Markdown when creating and displaying $terms.comments" @@ +48,1 @@ > } go ahead and remove the trailing whitespace here as well since it is so close by.

Going forward, glob mentioned that with the YUI migration project he is mentoring, that he had the developer fork the bugzilla repo on github instead of doing incremental patches that add more functionality over time. I think we should do the same and then when you are ready for feedback, commit your recent changes to the forked repo and let me know to take a look. Once we get all the way through, you would then make a full working patch from the repo and attach to the bug for full review. Sound good? dkl

(In reply to David Lawrence [:dkl] from comment #14) > Going forward, glob mentioned that with the YUI migration project he is > mentoring, that he had the developer fork the bugzilla repo on github > instead of doing incremental patches that add more functionality over time. > I think we should do the same and then when you are ready for feedback, > commit your recent changes to the forked repo and let me know to take a > look. Once we get all the way through, you would then make a full working > patch from the repo and attach to the bug for full review. > > Sound good? > > dkl To add to this, I can just watch the github fork you create and if you commit recent changes weekly, I can just watch the commits and make comments/feedback based on the commits along the way. dkl

(In reply to David Lawrence [:dkl] from comment #15) > (In reply to David Lawrence [:dkl] from comment #14) > > Going forward, glob mentioned that with the YUI migration project he is > > mentoring, that he had the developer fork the bugzilla repo on github > > instead of doing incremental patches that add more functionality over time. > > I think we should do the same and then when you are ready for feedback, > > commit your recent changes to the forked repo and let me know to take a > > look. Once we get all the way through, you would then make a full working > > patch from the repo and attach to the bug for full review. > > > > Sound good? > > > > dkl > > To add to this, I can just watch the github fork you create and if you > commit recent changes weekly, I can just watch the commits and make > comments/feedback based on the commits along the way. > > dkl Thanks David. It would be a good idea though I preferred to have a branch on the main repo and do it on BMO since other developers can also give us feedback. Anyway, please make sure that you review my commits carefully because reviewing and modifying a gigantic patch at the end would not be a smart thing to do and doubles our amount of work. So, what happens to this bug now?

(In reply to Koosha KM from comment #16) > Thanks David. It would be a good idea though I preferred to have a branch on > the main repo and do it on BMO since other developers can also give us > feedback. Anyway, please make sure that you review my commits carefully > because reviewing and modifying a gigantic patch at the end would not be a > smart thing to do and doubles our amount of work. I will review carefully. I also would rather not review a gigantic patch from scratch at the end if I can help it. The issue is we cannot commit these small changes to the main Bugzilla repo unless it is feature complete and approved. So it will have be to complete before we do so. Just let me know where the fork is and I will set up a watch on it. We (BMO) will hopefully be upgrading to 5.0 once it is complete so I am not sure if we will take the time to backport this to 4.2 or not. > So, what happens to this bug now? Once you have the branch setup and you commit this change, paste the commit message here and we can close it. dkl

(In reply to David Lawrence [:dkl] from comment #17) > > So, what happens to this bug now? > > Once you have the branch setup and you commit this change, paste the commit > message here and we can close it. > > dkl https://github.com/koosha--/bugzilla/commit/b811641067c2edd572ff0bd22ac7599140387e28 My repo: https://github.com/koosha--/bugzilla

(In reply to Koosha KM from comment #18) > https://github.com/koosha--/bugzilla/commit/ > b811641067c2edd572ff0bd22ac7599140387e28 > > My repo: https://github.com/koosha--/bugzilla Closing