Review Board 1.5.5

Use student's user_name as repository name when instructor uses CSV upload file too

Updated 1 year, 8 months ago

Severin Gehwolf Reviewers
trunk markus_developers
537
None MarkUs Source Code Repository 
------ Updated May 12, 2010

This review request is only the first part of a lengthy discussion. See reviews 
http://review.markusproject.org/r/537/ and
http://review.markusproject.org/r/538/ for (maybe) an end of this story :-)


------ Original Description


To be honest, I didn't consider the CSV upload part at all. Sorry, this
has been forgotten. Thinking of it, an implementation (i.e. application
of the attached patch) would have the following consequences:

RepositoryCollisions might be raised during CSV upload. For instance if
an instructor uploads a CSV file in which two different group names
exist with the very same student as the only member. I'm handling them,
but this might have other consequences, too.

Also consider the following scenario:
Say A1 is a single person assignment and A2 isn't. If the instructor
uses CSV uploads to create the groups for both assignments and uses the
very same group_name in the CSV for one or more records, the repository
of A1 (named after the user_name of the only student member of the group
of A1) will be used, although A2 might be set up to be a group
assignment. I.e. we could end up linking to a repository named after a
student's user_name, but this repo would be used as a group repository,
for another assignment. This is just something to consider. It's
probably a very unlikely case.
assignment_test.rb passes. We should add checks for this, though.
Mike Conley
Posted 2 years ago (January 12th, 2010, 5:41 p.m.)
Severin:

Great work!  See notes below,

-Mike
Maybe I've been staring at the screen too long, but I can't see what's changed here...
  1. Nothing really. I believe it's the white spaces, which got shuffled around and caused this to be highlighted.
Hm.  I know why we're doing this, but still, silently ignoring exceptions kinda freaks me out a little.

Maybe we could have an array of warning messages ("warning - repository collision...")...and maybe some logging here too, in case this gets hairy and we need to do some debugging.

What do you think?
  1. I'm not sure, if it makes much sense to report something about this to the user. Wouldn't they wonder what that means and if they do, can't do much about it... But I don't mind doing what you proposed. I'm just not 100% convinced if it makes sense to provide this info to the user.

Logging, on the other hand seems to me very useful. Will add that.

What I'm mostly worried about is the corner case of somebody creating a single student assignment (uses CSV upload for groups - c5anthei amongst them) and later creates a second assignment for which groups are allowed. Then assume for the second assignment the instructor uses the following CSV upload file:

c5anthei, bla, c5anthei, c5bloche

Then c5bloche would suddenly see c5anthei's work of A1. I am probably just haunted by some bad ghosts :-)
Fernando Garces
Posted 2 years ago (January 12th, 2010, 5:51 p.m.)

   

  

   
    
    
   


  




   



    

     
     
    

    

     
spaces maybe?

     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
messages would be nice for the user to see that he did something that he might have not wanted to and some logging will ease the pain when debugging.

This might not be the best solution but since our repository creation is starting to get so flexible that there so many cases to take into account how about create a file with the repository properties,
usernames:
group or individual:
assignments:

or something like that ( maybe xml)

     

  

   
    
    
   


  



    

    

   


  
 




  

 














 

  




 

   Review request changed
   
Updated 2 years ago (January 29th, 2010, 11:10 p.m.)

 

 

  

  
  
I've updated the csv_upload method in the assignment model to also use the student's username in certain conditions.

With this update the permissions problem Karen had recently with one student goes away, too. Permissions have been updated before the memberships were written. Hence, no changes to the svn_authz file were written.

I'm afraid, I won't get to test this soon. Apart from that, I'm still struggling how we might automatically test this (repo permissions written to authz and if the student's username is used)... Any ideas welcome!


 




  

 













 

  




 

 

  
  
Severin Gehwolf

  
Posted 2 years ago (January 29th, 2010, 11:12 p.m.)

 

 

   

   

  

   
    
    
   


  




   



    

     
     
    

    

     
I won't commit this. It has been remove already from my local changes.

     

  

   
    
    
   


  



    

    

   


  
 




  

 














 

  




 

   Review request changed
   
Updated 2 years ago (January 29th, 2010, 11:16 p.m.)

 

 

  
    

  


  
  
Added a screenshot showing a repository collision.


 




  

 













 

  




 

 

  
  
Mike Conley

  
Posted 2 years ago (February 1st, 2010, 3:29 p.m.)

 

 

   
Severin:

Just a few questions / notes below!  Nothing major - really close to a ship it,

-Mike

   

  

   
    
    
   


  




   



    

     
     
    

    

     
Hm.  For me, this sort of thing begs for more use of the begin/rescue blocks.  And line_nr can be handled by each_with_index.  Perhaps:

begin
  FasterCSV.parse(params[:group][:grouplist]).each_with_index do |row, line_nr|
  @assignment.add_csv_group(row)
  num_update += 1
rescue CSVLineError => e
  flash[:invalid_lines] << "Line #{line_nr}: " + e.message
rescue Exception => e
  flash[:error] = "Big, unrecoverable error"
  raise ActiveRecord::Rollback
end

And then maybe create some CSVLineError class that extends from Exception to distinguish, and put the logic/messages inside the Assignment.add_csv_group model method.

Just an idea to refactor.  Doesn't have to be done this time, but maybe write up a ticket for it.

     

  

   
    
    
   


  



    

    

    

     
     

      

       
        

         
          branches/release_0.6/app/models/assignment.rb
          

           (Diff revision 2)

          
         
        

       
       
        
 


        
 


        
 


        
 


        
 


        
 


       
      

     

    

    

     
Can be shortened to:

group = Group.find_or_create_by_group_name(row[0])

     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
Another good place for refactoring (and a ticket if you don't get to it) - I remember Mike G mentioning that logic like this is often made more readable if turned into a model method, like:

self.conditions_perfect_for_student_named_repository(row)

     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
Hm...good question.  I think regardless, it'd be a good idea to log this.

     

  

   
    
    
   


  



    

    

    

     
     

      

       
        

         
          branches/release_0.6/app/models/assignment.rb
          

           (Diff revision 2)

          
         
        

       
       
        
 


        
 


        
 


        
 


        
 


       
      

     

    

    

     
Another spot for refactoring.  This is more of a personal thing, but when there's a trivial case like this, I like to put it near the top of the method, and get it out of the way.  Again, just a personal thing.  :D

     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
Question - why are these two lines here, and not after line 298 (grouping.assignment_id = self.id)?

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    IIRC this is another side effect of repositories and group/groupings. The assignment folder is created by an after_create hook of grouping. And modifying memberships involves writing permissions for members (but uses the grouping - potentially a newly created one - to get to the group and the repository. I.e. grouping not saved, no membership modification possible. I could be totally wrong, but I got bitten by something of that sort at some point. Those callbacks are neat, but if things go wrong, hard to debug :-)
    
    
    
   
    2. 

   


  



    

    

    

     
     

      

       
        

         
          branches/release_0.6/app/models/group.rb
          

           (Diff revision 2)

          
         
        

       
       
        
 


        
 


        
 


        
 


        
 


       
      

     

    

    

     
Another question:

Is it a safe assumption that if an Instructor has set it up so that, for it's first assignment, commits must be made externally, that all following assignment submission commits will be made externally?

     

  

   
    
    
   


  



    

    

    

     
     

      

       
        

         
          branches/release_0.6/app/models/student.rb
          

           (Diff revision 2)

          
         
        

       
       
        
 


        
 


        
 


        
 


        
 


       
      

     

    

    

     
Good catch!

     

  

   
    
    
   


  



    

    

   


  
 




  

 
















 

  




 

 

  
  
Severin Gehwolf

  
Posted 2 years ago (February 1st, 2010, 5:05 p.m.)

 

 

   

   

  

   
    
    
   


  




   



    

     
     

      

       
        

         
          branches/release_0.6/app/models/group.rb
          

           (Diff revision 2)

          
         
        

       
       
        
 


        
 


        
 


        
 


        
 


       
      

     

    

    

     
I don't think this is a safe assumption. For instance, Karen uses for her CSC209 class an external commit set-up. But for her labs she wants to allow students to submit via the Web UI.

Why are you asking?

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Sorry - should have made my question more clear.  Because this method is simply looking at the first assignment that this Group belongs to in order to determine whether or not repository's have external commits only, we may be overlooking the case where group G belongs to assignments A1 and A2, where A1 has allow_web_submits = true, and A2 has allow_web_submits = false.  Am I missing something here?
    
    
    
   
    2. 

   


  



    

    

   


  
 




  

 
















 

  




 

 

  
  
Severin Gehwolf

  
Posted 2 years ago (February 1st, 2010, 5:06 p.m.)

 

 

   

   

  

   
    
    
   


  




   



    

     
     
    

    

     
It's logged in group.rb :-) The whole message including path.

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Ah. Coolbeans.
    
    
    
   
    2. 

   


  



    

    

   


  
 




  

 
















 

  




 

 

  
  
Benjamin Vialle

  
Posted 2 years ago (February 1st, 2010, 5:37 p.m.)

 

 

   

   

  

   
    
    
   


  




   



    

     
     

      

       
        

         
          branches/release_0.6/app/controllers/groups_controller.rb
          

           (Diff revision 2)

          
         
        

       
       
        
 


        
 


        
 


        
 


        
 


        
 


        
 


       
      

     

    

    

     
Shall we add an I18n.t() method here ?

     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
Idem there with I18n.t()

     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
idem missing I18n.t() ?

     

  

   
    
    
   


  



    

    

   


  
 




  

 
















 

  




 

 

  
  
Severin Gehwolf

  
Posted 2 years ago (February 1st, 2010, 5:37 p.m.)

 

 

   

   

  

   
    
    
   


  




   



    

     
     

      

       
        

         
          branches/release_0.6/app/models/group.rb
          

           (Diff revision 2)

          
         
        

       
       
        
 


        
 


        
 


        
 


        
 


       
      

     

    

    

     
What I was trying to do here is to get the assignment this group object belongs to. But this may be a huge bug... Is there a way to find this assignment? At the moment, it doesn't seem obvious to me... :-(

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Gotcha - no worries.  :)

The thing is, a Group has_many assignments through Groupings, and so they're not associated with any particular Assignment.  Might need a little refactoring here - hang on, let me do another review...
    
    
    
   
    2. 

   


  



    

    

   


  
 




  

 
















 

  




 

 

  
  
Mike Conley

  
Posted 2 years ago (February 1st, 2010, 6:50 p.m.)

 

 

   

   

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I'm adding Karen's message for reference: 

--8<---------

I may be coming into this discussion late in the game, but repos that are sometimes used for web submit and sometimes used for svn commits sounds like a problem waiting to happen.  In particular, if a group has svn submit for a1 and web submit for a2, they will still have svn permissions for the a2 directory, and the different semantics for web submit vs svn commit could get them into trouble.

I'm not sure how this affects what we set up for assignments.  Can we not allow carrying groups forward if the submission mechanism differs?

Karen

--8<---------


Karen, the key here is to use different group names for different assignments. If MarkUs doesn't find a group in the database, it'll create a new record. As a result a new repository would be created. Mike, is that right?
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Hm.  This works if Instructors are not cloning Groups forward.  But what if they are?

Perhaps on cloning groups forward, we should take a look at the Assignment properties to see if external commits are allowed.  If they aren't, pull the permissions.

There's an edge case, where A2 is created before A1 is completed, and A2 clones A1's Groups, and A2 wipes out the repository permissions...

Perhaps we should distinguish between cloning a *Group* forward, and cloning a *team of Students* forward.  The difference being that in the latter case, new Groups/Groupings are created, but StudentMemberships are copied.  

Any more thoughts on this?
    
    
    
   
    3. 

   


  




   



    

     
     
    

    

     
I think the Grouping should be responsible for knowing if we write_repo_permissions?, because the Grouping is associated with only one Assignment.

So maybe this should be:

return unless self.write_repo_permissions?

And move write_repo_permissions? into Grouping, which checks markus_config_repository_admin? and self.assignment.allow_web_submits.

What do you think?  How does that sound?

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Right. Sounds reasonable to me. That should work. I'll try to get to the refactoring soon. Thanks!
    
    
    
   
    2. 

   


  



    

    

   


  
 




  

 














 

  




 

   Review request changed
   
Updated 1 year, 9 months ago (May 2nd, 2010, 10:08 p.m.)

 

 

  

  
  
I did some refactoring on this and I believe all corner cases are dealt with "OK". There is also a new test suite (in an extra file) using SubversionRepository. It basically looks at what MarkUs creates in the file system. What are your thoughts? BTW: I've thought loudly about the corner cases I could think of here: http://blog.markusproject.org/?p=1535


 




  

 













 

  




 

 

  
  
Severin Gehwolf

  
Posted 1 year, 9 months ago (May 2nd, 2010, 10:08 p.m.)

 

 

   

   

  

   
    
    
   


  




   



    

     
     
    

    

     
I'll remove this, before committing.

     

  

   
    
    
   


  



    

    

   


  
 




  

 
















 

  




 

 

  
  
Severin Gehwolf

  
Posted 1 year, 9 months ago (May 6th, 2010, 12:36 p.m.)

 

 

   
Has anybody had a chance to look at this yet? Thanks! :-)

   

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Hey!  I hadn't forgotten - just been a little busy.  I'll get to it within a day or two.

Thanks!

-Mike
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Totally cool. I'll have to update it a little bit anyway. Take your time please :-)
    
    
    
   
    3. 

   


  




  
 




  

 














 

  




 

   Review request changed
   
Updated 1 year, 9 months ago (May 7th, 2010, 5:58 p.m.)

 

 

  
    

   
  • 


changed from branches/release_0.6 to trunk
    




   
    • 

  


  
  
Switching this to trunk so I can TestDrive.


 




  

 















 

  




 

 

  
  
Mike Conley

  
Posted 1 year, 9 months ago (May 7th, 2010, 7:19 p.m.)

 

 

   
Severin:

It looks pretty good - just some minor questions/nitpicks below.

Great work!

-Mike

   

  

   
    
    
   


  




   



    

     
     

      

       
        

         
          trunk/app/models/assignment.rb
          

           (Diff revision 3)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
Is this method supposed to return anything?

I see that it optionally returns collision errors...and it can also return nil.

The reason I ask, is because on line 441 of test/unit/assignment_test.rb, we assert @assignment.add_csv_group(group).  Asserting isn't going to work if it returns nil.  So currently, assignment_test.rb gives off two failing tests.

I've noticed this elsewhere too - methods that *sometimes* return things.  It's not a problem, but in this case, we either need to change add_csv_group, or update our test.

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Yes, I noticed the failing assignment_test.rb. I updated the tests. The tricky thing was collision_error message related. If there are collision errors, I can't just raise a CSVInvalidLineError or some other exception, because in that case groupings won't be created (but we want them created). Collision errors are merely warnings for the user and they shouldn't happen very often if at all, anyway (since we are looking up existent groups in the database).

Also, I *think* return nil if ... is equivalent to return if ... (The former is a bit more explicit and, hence, easier to read).
    
    
    
   
    2. 

   


  



    

    

    

     
     

      

       
        

         
          trunk/app/models/assignment.rb
          

           (Diff revision 3)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
While you're working in this area, an each loop is the preferred loop in Ruby.

start_index_group_members..(row.length - 1).each do |i|
...
end

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Sure, easy change :-)
    
    
    
   
    2. 

   


  



    

    

    

     
     
    

    

     
I'm curious why the new exception class - were we going to add some specialized functions/info to it?

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    No, I didn't plan to add anything. My thinking was that it's cleaner to raise a named exception instead of a generic RuntimeError using 'raise "something"' syntax... Thoughts?
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Yeah, I have no problem with that - I just think it maybe should be in the lib/ directory as opposed to models, since it isn't a model.  Just don't want to confuse anyone.  :)
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    I see. Makes sense. How about creating a directory in lib called "classes" and move this one and MarkusLogger there? What do you think?
    
    
    
   
    4. 

   


  



    

    

    

     
     

      

       
        

         
          trunk/app/views/groups/manage.html.erb
          

           (Diff revision 3)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
I18n, while you're here

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Will do.
    
    
    
   
    2.