Review Board 1.5 RC1

Display annotation summaries on core entity index pages

Updated 2 years, 8 months ago

Oliver Charles Reviewers
Default
rob, lukas
None MusicBrainz git-svn mirror 
This patch display annotation summaries (the first paragraph of an annotation, formatted with Text::WikiFormat) on all core entity index pages.
Unit tests/general tests against test database
Oliver Charles
Posted 2 years, 8 months ago (May 24th, 2009, 6:53 p.m.)

   

  

   
    
    
   


  




   



    

     
     

      

       
        

         
          root/artist/index.tt
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
annotation/artist-annotation.tt or artist/annotation.tt?

     

  

   
    
    
   


  



    

    

   


  
 




  

 














 

  




 

   Review request changed
   
Updated 2 years, 8 months ago (May 24th, 2009, 7:03 p.m.)

 

 

  

  
  
Also show annotations on recordings


 




  

 













 

  




 

 

  
  
Lukáš Lalinský

  
Posted 2 years, 8 months ago (May 24th, 2009, 8:17 p.m.)

 

 

   
Apart from the one check the code looks good, but I have a comment about the URL structure. I'd very much prefer /<entity>/<mbid>/annotation over /annotation/<entity>/<mbid>. It makes it look like annotation is a first-class object, which it isn't. As far as I see, the actual controller for this is not yet implemented, but I'd really prefer if the entity controller was the entry point and the annotation controller just a helper.

   

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Yea, that makes sense. It was done this way because it would cause a lot of duplication - but now that Catalyst is Moose based, I can implement this as a role in the controller.
    
    
    
   
    2. 

   


  




   



    

     
     
    

    

     
I think this trusts user's data a bit too much. If we are going to use this approach, we need to only accept $type we know about. But I'd rather to handle it in the main entity controller, using an action that would call some generic function (or $c->dispatch and pass it arguments via $c->stash).

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Yea, I haven't written the controller really yet - this was more of a stub. We will need some proper validation (or probably not if we have annotations as a controller role).
    
    
    
   
    2. 

   


  



    

    

   


  
 




  

 
















 

  




 

 

  
Ship it!

  
Lukáš Lalinský

  
Posted 2 years, 8 months ago (May 24th, 2009, 11:16 p.m.)

 

 

   
+1 then.

   

  

   
    
    
   


  




  
 




  

 














 

  




 

   Review request changed
   
Updated 2 years, 8 months ago (May 25th, 2009, 2:16 a.m.)

 

 

  

  
  
Annotation controller is now a controller role. This requires bleeding edge versions of Catalyst::Runtime & MooseX::MethodAttributes.

No huge changes, but I wanted to run this by people one more time.


 




  

 















 

  




 

 

  
Ship it!

  
Lukáš Lalinský

  
Posted 2 years, 8 months ago (May 25th, 2009, 8:31 a.m.)

 

 

   

   

  

   
    
    
   


  




   



    

     
     

      

       
        

         
          lib/MusicBrainz/Server/Controller.pm
          

           (Diff revision 3)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
Why does this need to be in BEGIN?

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Because actions use code attributes (:Local :Path, etc) which are compile - thus we need to extend before that code is compiled, or you'll get an error. (It's pretty standard Catamoose stuff, and the only agreed ugliness that Catalyst 5.8 adds)
    
    
    
   
    2.