-
-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue#952 feature progressive tips #961
Conversation
27834dc
to
520efc5
Compare
Code looks good. I'd like to see an screenshot, too. |
Please implement the bibliotheca side of this issue and then I will merge it. |
per @flbulgarelli's request: |
@fedescarpa please review |
app/models/submission/submission.rb
Outdated
assignment.update! results | ||
assignment.failed_submissions_count += 1 unless results[:status] == :passed | ||
assignment.update results | ||
assignment.save! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flbulgarelli, update
+ save!
is not the same that update!
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But only in the scenario you did not assign unsaved attributes before update.
@fedescarpa please propose UI improvements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please @flbulgarelli view my comment. The rest of the PR LGTM
About the UI I don't have greater ideas, I think it's fine just like this |
520efc5
to
e2693da
Compare
e2693da
to
c10292b
Compare
Closing. This PR is included in #1072 |
Do not merge yet.
The real location for the tips still needs to be decided.
Fixes #952.