Skip to content
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

E1778 OSS project Purple: UI fixes for assignment creation #1033

Closed
wants to merge 0 commits into from

Conversation

rajanalwan
Copy link
Contributor

Fixed issues #961 and #972.

By:
Vidhyalakshimi Sreenivasan - vsreeni
Rajan Alwan - ralwan

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.9%) to 47.938% when pulling c917b0e on rajanalwan:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.03%) to 43.821% when pulling 40a7ffb on rajanalwan:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.3%) to 43.539% when pulling 66ec5fe on rajanalwan:master into a26cf3a on expertiza:master.

@@ -118,6 +123,7 @@ def update
unless params.key?(:assignment_form)
@assignment = Assignment.find(params[:id])
@assignment.course_id = params[:course_id]
puts "Perry1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not write to stdout.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use "logger.debug" instead. and print messages that make sense e.g., @assignment.course_id instead of senseless strings.

@ferryxo ferryxo changed the title OSS Project Purple E1778 OSS project Purple: UI fixes for assignment creation Nov 8, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-10.8%) to 40.05% when pulling 692b18f on rajanalwan:master into a26cf3a on expertiza:master.

# Deleting Due date info from table if meta-review is unchecked. - UNITY ID: ralwan and vsreeni

@due_date_info = DueDate.find_each(parent_id: params[:id])
puts @due_date_info.inspect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use log

flash[:note] = 'The assignment was successfully saved....'

if params[:set_pressed][:bool] == 'false'
flash[:error] = "Submissions count greater than rounds of reviews. Please set the value before saving."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the error msg would be more useful if it's:

The available submissions for this assignment indicate that there are X rounds of review. Are you sure that you want to change it to Y rounds of review?

you have to calculate X first

@@ -186,7 +186,7 @@ def set_up_assignment_review

submissions = @assignment.find_due_dates('submission')
reviews = @assignment.find_due_dates('review')
@assignment.rounds_of_reviews = [@assignment.rounds_of_reviews, submissions.count, reviews.count].max
# @assignment.rounds_of_reviews = [@assignment.rounds_of_reviews, submissions.count, reviews.count].max
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either delete this, or add comment why you deactivate this LoC.

var original_round_count = <%= @assignment_form.assignment.rounds_of_reviews%>;

if(new_round_count < submissions_count){
if(!confirm("Submissions count greater than rounds of reviews. Are you sure you want to change rounds of reviews?")){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of confirm, alert and all blocking popup window. it's fine for now. but try to avoid them in the future.

@rajanalwan
Copy link
Contributor Author

New pull request for this project can be found here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants