Remark Request Old Marks Not Appearing Initially #904

Closed
joelburford opened this Issue Oct 25, 2012 · 20 comments

Comments

Projects
None yet
5 participants
@joelburford
Contributor

joelburford commented Oct 25, 2012

Mark and release an assignment.
As student request remark.
As administrator navigate to assignment.

The old marks do not initially appear on the rubrics.

If you click on a new grade, then refresh the page, the old mark will appear as well as the new one. Those old marks should appear right away.

@joelburford

This comment has been minimized.

Show comment
Hide comment
@joelburford

joelburford Nov 9, 2012

Contributor

If you select a grade for the bottom criterion and then refresh the page all of the old marks will appear.

Contributor

joelburford commented Nov 9, 2012

If you select a grade for the bottom criterion and then refresh the page all of the old marks will appear.

@joelburford

This comment has been minimized.

Show comment
Hide comment
@joelburford

joelburford Nov 15, 2012

Contributor

In Summary under "Show Old Mark Summary" the old marks will also all be zero until the above fix is performed. (click mark, refresh page)

Contributor

joelburford commented Nov 15, 2012

In Summary under "Show Old Mark Summary" the old marks will also all be zero until the above fix is performed. (click mark, refresh page)

@mikeing2001

This comment has been minimized.

Show comment
Hide comment
@mikeing2001

mikeing2001 Nov 16, 2012

Contributor

Here are my findings.

in result_controller.rb submission.result field doesn't return the correct result. Instead of returning the old result, it returns the latest result. However, I think this has to do with some permission issue. When a student creates a remark request or cancel a remark request, the old result is saved correctly and in submission.result field. So it looks like ta and admin can access the old result saved by the student when a remark is created. Also, it looks like TA and admins can access it once they submit a new mark. It could be related to issues with different database, which would explain why the demo server can't reproduce this but I don't think so.

Contributor

mikeing2001 commented Nov 16, 2012

Here are my findings.

in result_controller.rb submission.result field doesn't return the correct result. Instead of returning the old result, it returns the latest result. However, I think this has to do with some permission issue. When a student creates a remark request or cancel a remark request, the old result is saved correctly and in submission.result field. So it looks like ta and admin can access the old result saved by the student when a remark is created. Also, it looks like TA and admins can access it once they submit a new mark. It could be related to issues with different database, which would explain why the demo server can't reproduce this but I don't think so.

@ioev

This comment has been minimized.

Show comment
Hide comment
@ioev

ioev Nov 16, 2012

Contributor

So I tried to reproduce this issue on my own server, and wasn't able to. The old grades show up as soon as I load the grading page as admin. Joel, can you explain the steps you had to go through to reproduce this in a little more detail? Specifically, what grade was assigned, and what happens if you refresh the page before you click on a new grade, will this show the old grades? I feel like this might be a caching issue of some sort.

I also noticed that if the assignment is setup to use Flexible Grading, it won't display the old marks regardless of what I do. It's just not in the view at all to show them, but you can see them in the Summary with the rollover easily. This could probably be included in the same issue, but it will require that flexible marking actually works so you may not be able to fix it yet.

Also, while you're working on this, I get:
DEPRECATION WARNING: save(false) is deprecated, please give save(:validate => false) instead. (called from edit at /home/ioev/Projects/Markus/app/controllers/results_controller.rb:60)
Once per grading (rubric and flexible) item when loading the page. I don't think there's any reason it needs to be saving this every single time it loads the page, so this should probably be moved into a model of some sort, or if we are creating the old_marks when the remark request is submitted, then just change this to use .find() instead. Or just update the .save call to remove the deprecation warning.

Contributor

ioev commented Nov 16, 2012

So I tried to reproduce this issue on my own server, and wasn't able to. The old grades show up as soon as I load the grading page as admin. Joel, can you explain the steps you had to go through to reproduce this in a little more detail? Specifically, what grade was assigned, and what happens if you refresh the page before you click on a new grade, will this show the old grades? I feel like this might be a caching issue of some sort.

I also noticed that if the assignment is setup to use Flexible Grading, it won't display the old marks regardless of what I do. It's just not in the view at all to show them, but you can see them in the Summary with the rollover easily. This could probably be included in the same issue, but it will require that flexible marking actually works so you may not be able to fix it yet.

Also, while you're working on this, I get:
DEPRECATION WARNING: save(false) is deprecated, please give save(:validate => false) instead. (called from edit at /home/ioev/Projects/Markus/app/controllers/results_controller.rb:60)
Once per grading (rubric and flexible) item when loading the page. I don't think there's any reason it needs to be saving this every single time it loads the page, so this should probably be moved into a model of some sort, or if we are creating the old_marks when the remark request is submitted, then just change this to use .find() instead. Or just update the .save call to remove the deprecation warning.

@mikeing2001

This comment has been minimized.

Show comment
Hide comment
@mikeing2001

mikeing2001 Nov 16, 2012

Contributor

I think I figure out why it is reproducible on some machine and not on the other. It is related to database and ordering. When we have 2 results id pointing to 1 submission id, on some configuration,it will get the correct result. On other times it will get the new incorrect result. To fix this we may either have to create a specific field that will store the correct result id value or some statement like this: @old_result = Result.find(:all, :conditions => ["submission_id = ? and marking_state = 'complete'", @submission.id], :order =>["id ASC"])[0] replacing @old_result = @submission.result. This way it "should" find the correct submission. However, I think this might not be the best way of solving this.

Contributor

mikeing2001 commented Nov 16, 2012

I think I figure out why it is reproducible on some machine and not on the other. It is related to database and ordering. When we have 2 results id pointing to 1 submission id, on some configuration,it will get the correct result. On other times it will get the new incorrect result. To fix this we may either have to create a specific field that will store the correct result id value or some statement like this: @old_result = Result.find(:all, :conditions => ["submission_id = ? and marking_state = 'complete'", @submission.id], :order =>["id ASC"])[0] replacing @old_result = @submission.result. This way it "should" find the correct submission. However, I think this might not be the best way of solving this.

@ioev

This comment has been minimized.

Show comment
Hide comment
@ioev

ioev Nov 16, 2012

Contributor

If that were the case though, wouldn't it be showing the "old_marks" placeholders, but they would just have the value of the new_marks (likely 0)?

Contributor

ioev commented Nov 16, 2012

If that were the case though, wouldn't it be showing the "old_marks" placeholders, but they would just have the value of the new_marks (likely 0)?

@mikeing2001

This comment has been minimized.

Show comment
Hide comment
@mikeing2001

mikeing2001 Nov 16, 2012

Contributor

It won't since "old_result" is nil and there is a if @old_result so it wouldn't continue

Contributor

mikeing2001 commented Nov 16, 2012

It won't since "old_result" is nil and there is a if @old_result so it wouldn't continue

@joelburford

This comment has been minimized.

Show comment
Hide comment
@joelburford

joelburford Nov 21, 2012

Contributor
  1. Log in as admin

    -navigate to a submission for an assignment (copy the submitter's user name)

    -put a grade in each criterion, any value works

    -change status to complete using drop down box

    -check release mark box

    -logout

  2. Log in with copied user name from above

    -navigate to submission for assignment

    -select "results"

    -select "remark request"

    -press "submit request to instructor"

    -logout

  3. Log in as admin

    -navigate to the same submission

    -no old marks are visible

    -refresh page, no old marks visible

    -select a grade for a criterion

    -refresh page, one or more old marks should now be visible

    -the last step isn't consistent, sometime all old marks show, sometimes just the criterion selected

I get the bug every time on mine. Also, it looks like you're not running the latest code, I updated the deprecated save code and that error shouldn't be there. A few people have seen the bug happen when running the latest code. Apparently the bug does not appear on the demo server.

Contributor

joelburford commented Nov 21, 2012

  1. Log in as admin

    -navigate to a submission for an assignment (copy the submitter's user name)

    -put a grade in each criterion, any value works

    -change status to complete using drop down box

    -check release mark box

    -logout

  2. Log in with copied user name from above

    -navigate to submission for assignment

    -select "results"

    -select "remark request"

    -press "submit request to instructor"

    -logout

  3. Log in as admin

    -navigate to the same submission

    -no old marks are visible

    -refresh page, no old marks visible

    -select a grade for a criterion

    -refresh page, one or more old marks should now be visible

    -the last step isn't consistent, sometime all old marks show, sometimes just the criterion selected

I get the bug every time on mine. Also, it looks like you're not running the latest code, I updated the deprecated save code and that error shouldn't be there. A few people have seen the bug happen when running the latest code. Apparently the bug does not appear on the demo server.

@ioev

This comment has been minimized.

Show comment
Hide comment
@ioev

ioev Nov 21, 2012

Contributor

Hmm, so then what DB server are you using? And does anyone know what DB server the demo server is running?

Contributor

ioev commented Nov 21, 2012

Hmm, so then what DB server are you using? And does anyone know what DB server the demo server is running?

@joelburford

This comment has been minimized.

Show comment
Hide comment
@joelburford

joelburford Nov 21, 2012

Contributor

I'm using PostgreSQL, not sure about demo server.

Contributor

joelburford commented Nov 21, 2012

I'm using PostgreSQL, not sure about demo server.

@jerboaa

This comment has been minimized.

Show comment
Hide comment
@jerboaa

jerboaa Nov 21, 2012

Member

Demo is running on mysql.

Member

jerboaa commented Nov 21, 2012

Demo is running on mysql.

@ioev

This comment has been minimized.

Show comment
Hide comment
@ioev

ioev Nov 21, 2012

Contributor

And I'm running sqlite. I'll get things up and running with PostgreSQL and see if it's specific to that DB.

Contributor

ioev commented Nov 21, 2012

And I'm running sqlite. I'll get things up and running with PostgreSQL and see if it's specific to that DB.

@ioev

This comment has been minimized.

Show comment
Hide comment
@ioev

ioev Nov 21, 2012

Contributor

Alright, confirmed in PostgresSQL. Looking at the results table (before adding a new mark to make the old marks show up) I have 2 records associated with this submission:
(id;submission id;...)
"2";2;"complete";"";"2012-11-21 21:19:15.75717";"2012-11-21 21:21:15.347218";FALSE;23
"3";2;"partial";"";"2012-11-21 21:21:15.283608";"2012-11-21 21:21:15.325788";FALSE;0

In the debugger, I can see that the most recent record (id = 3) is being returned by @submission.result, probably because it has the lowest timestamp (not sure how since it would have been created AFTER id:2). I'm assuming that mysql/sqlite don't store the extended timestamp info that postgres is, and that's why this only happens in postgres. Sorting order is probably defaulting to most recent timestamp, then to id.

After adding a mark to the results with id 3, we then get:
"2";2;"complete";"";"2012-11-21 21:19:15.75717";"2012-11-21 21:21:15.347218";FALSE;23
"3";2;"partial";"";"2012-11-21 21:21:15.283608";"2012-11-21 21:40:29.198904";FALSE;2

And id = 2 is returned because it's timestamp is now lower.

Mike is correct though, and we need to make sure that the right result is always returned. First off though, I've noticed that the Submission model has a line:

has_one :result, :dependent => :destroy

Since we know for sure that these models will sometimes have more than one, I'd suggest changing has_one to has_many, and deal with the problems that it will cause, making sure that every use of @submission.result returns the one we actually actually want (even better, write a method in the Result model "get_result" that does this, and then write another "get_old_result" that returns the marks that are being requested to be remarked.)

Contributor

ioev commented Nov 21, 2012

Alright, confirmed in PostgresSQL. Looking at the results table (before adding a new mark to make the old marks show up) I have 2 records associated with this submission:
(id;submission id;...)
"2";2;"complete";"";"2012-11-21 21:19:15.75717";"2012-11-21 21:21:15.347218";FALSE;23
"3";2;"partial";"";"2012-11-21 21:21:15.283608";"2012-11-21 21:21:15.325788";FALSE;0

In the debugger, I can see that the most recent record (id = 3) is being returned by @submission.result, probably because it has the lowest timestamp (not sure how since it would have been created AFTER id:2). I'm assuming that mysql/sqlite don't store the extended timestamp info that postgres is, and that's why this only happens in postgres. Sorting order is probably defaulting to most recent timestamp, then to id.

After adding a mark to the results with id 3, we then get:
"2";2;"complete";"";"2012-11-21 21:19:15.75717";"2012-11-21 21:21:15.347218";FALSE;23
"3";2;"partial";"";"2012-11-21 21:21:15.283608";"2012-11-21 21:40:29.198904";FALSE;2

And id = 2 is returned because it's timestamp is now lower.

Mike is correct though, and we need to make sure that the right result is always returned. First off though, I've noticed that the Submission model has a line:

has_one :result, :dependent => :destroy

Since we know for sure that these models will sometimes have more than one, I'd suggest changing has_one to has_many, and deal with the problems that it will cause, making sure that every use of @submission.result returns the one we actually actually want (even better, write a method in the Result model "get_result" that does this, and then write another "get_old_result" that returns the marks that are being requested to be remarked.)

@mikeing2001

This comment has been minimized.

Show comment
Hide comment
@mikeing2001

mikeing2001 Nov 21, 2012

Contributor

That was the approach that me and @reidka was starting to discuss yesterday before i had to run off to class. The has_one tag should be changed as it is not 100% correct.

Joey, what you have sounds like a good approach. Joel, do you think you would be able to fix it based off of Joey details/suggestions? This is major bug we need to fix inorder order to release a newer verison of MarkUs.

Contributor

mikeing2001 commented Nov 21, 2012

That was the approach that me and @reidka was starting to discuss yesterday before i had to run off to class. The has_one tag should be changed as it is not 100% correct.

Joey, what you have sounds like a good approach. Joel, do you think you would be able to fix it based off of Joey details/suggestions? This is major bug we need to fix inorder order to release a newer verison of MarkUs.

@joelburford

This comment has been minimized.

Show comment
Hide comment
@joelburford

joelburford Nov 22, 2012

Contributor

Hm. This appears a bit over my head and might be better for someone else to tackle. I have a rough idea of what to do, but I might not do it well/correctly. Is it okay to pass this off? If so I'll peak into the last remaining remark request bug.

Contributor

joelburford commented Nov 22, 2012

Hm. This appears a bit over my head and might be better for someone else to tackle. I have a rough idea of what to do, but I might not do it well/correctly. Is it okay to pass this off? If so I'll peak into the last remaining remark request bug.

@antez

This comment has been minimized.

Show comment
Hide comment
@antez

antez Nov 23, 2012

Contributor

I can give it a try.

Contributor

antez commented Nov 23, 2012

I can give it a try.

@mikeing2001

This comment has been minimized.

Show comment
Hide comment
@mikeing2001

mikeing2001 Nov 28, 2012

Contributor

@antez, I have implemented a temporary fix (incase it is needed to release the new verison of MarkUs) that will work but it takes an assumption into account that we should not depend on.

Follow Joey's idea as it is a good approach to remove ambiguity that caused this issue in the first place.

Contributor

mikeing2001 commented Nov 28, 2012

@antez, I have implemented a temporary fix (incase it is needed to release the new verison of MarkUs) that will work but it takes an assumption into account that we should not depend on.

Follow Joey's idea as it is a good approach to remove ambiguity that caused this issue in the first place.

@antez

This comment has been minimized.

Show comment
Hide comment
@antez

antez Nov 30, 2012

Contributor

@mikeing2001, i found a fix by swapping the order of
@result.save
and
@old_result.save
inside function update_remark_request. Is that how you fix it?

Contributor

antez commented Nov 30, 2012

@mikeing2001, i found a fix by swapping the order of
@result.save
and
@old_result.save
inside function update_remark_request. Is that how you fix it?

@mikeing2001

This comment has been minimized.

Show comment
Hide comment
@mikeing2001

mikeing2001 Nov 30, 2012

Contributor

that is not, but i don't think that will work for mysql. Since mysql currently works, wouldn't swapping cause it to be out of order. My fix is how i query the old mark.
@old_result = Result.find(:all, :conditions => ["submission_id = ?", @submission.id], :order =>["id ASC"])[0]
The problem with this is that it will assume the smallest result id is the old mark which is correct and most likely always be correct but i do think there is a better/clearer way of solving this.

Contributor

mikeing2001 commented Nov 30, 2012

that is not, but i don't think that will work for mysql. Since mysql currently works, wouldn't swapping cause it to be out of order. My fix is how i query the old mark.
@old_result = Result.find(:all, :conditions => ["submission_id = ?", @submission.id], :order =>["id ASC"])[0]
The problem with this is that it will assume the smallest result id is the old mark which is correct and most likely always be correct but i do think there is a better/clearer way of solving this.

@jerboaa

This comment has been minimized.

Show comment
Hide comment
@jerboaa

jerboaa Dec 3, 2012

Member

Closed with PR #940. There should be a better fix for this, though. Please open new issue asking for better fix.

Member

jerboaa commented Dec 3, 2012

Closed with PR #940. There should be a better fix for this, though. Please open new issue asking for better fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment