add ids to fieldsets in review request details template

Review Request #7721 — Created Oct. 21, 2015 and submitted

Information

Review Board
release-2.0.x

Reviewers

Adds id attributes to fieldsets in review request details template.

This allows extensions to hide an entire fieldset from the UI only without
impacting backend operations, or to style selected fieldsets differently from
others.

eg. an extension may render a fieldset in a different manner, but store the
data in the standard fields.  Unregistering the fieldset would break
functionality when all that's required UI hiding.

updated template, ensured id attributes where rendered in the generated html.

reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. 
      
GL
mike_conley
  1. Hey glob,

    The fields API allows us to wholesale remove any fieldsets we don't want, I believe - like, I think the following code would remove the "Info" fieldset:

    info_fieldset = get_review_request_fieldset("info")
    unregister_review_request_fieldset(info_fieldset)
    

    Note that you'd want to probably put that fieldset back when the extension shuts down. I think we do something similar with the Testing Done field.

    1. Is the above sufficient, or are there usecases that you have where the wholesale removal of a fieldset is not going to work for you?

    2. unfortunately unregistering fieldset doesn't work for us -- we need to still track data in those fields and have control over how they are represented in the UI.
      
      when i unregistered the fieldset, reviewboard stopped seeing those fields as part of a changedesc so we weren't able to update those fields.  monkey-patching all the fields to return should_render=false left the fieldset header with no fields (and monkey-patching for this feels wrong).
    3. So the fieldsets must remain, their fields tracked, and all that, but you don't want them shown in the UI at all, is that correct?

      I don't mind having IDs in the template. That's totally fine. I'm just trying to see if we should be accounting for anything else in our design. Perhaps allowing visibility to be forcibly turned off for a fieldset/field in Python, keeping it registered but not outputting to the page. Though, that's likely a bigger change, and maybe not one that people will need as much.

  2. 
      
chipx86
  1. Hi,

    I'm going to need a lot more detail in the description, sentence casing in the summary, and a detailed description of how you tested this. See https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/.

    1. oops; sorry about that.

    2. No worries, thanks! :)

  2. 
      
GL
mike_conley
  1. Looks good to me. Thanks glob!

  2. 
      
brennie
  1. I'm going to land this and sentence case the summary.

  2. 
      
GL
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (bfc8760)
Loading...