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.

Sign up for GitHub

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

Jump to bottom

close task immediately #189

Merged
merged 2 commits into from Jan 4, 2017
Merged

close task immediately #189

merged 2 commits into from Jan 4, 2017

Conversation

skyhit
Copy link
Collaborator

@skyhit skyhit commented Jan 4, 2017

No description provided.

@skyhit skyhit requested a review from ajefts January 4, 2017 13:44
skyhit
skyhit commented Jan 4, 2017
View reviewed changes
...e_facade/src/java/main/com/topcoder/service/facade/contest/ejb/ContestServiceFacadeBean.java
@@ -4574,6 +4600,12 @@ public SoftwareCompetition updateSoftwareContest(TCSubject tcSubject, SoftwareCo
contest.setProjectData(projectData);
contest.setId(projectData.getProjectHeader().getId());


if (!isPrivateProject(contest) && ("1".equals(oldProject.getProperty(ProjectPropertyType.PRIVATE_PROJECT)))) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ajefts do we need this logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is saying that we'll remove registrants if we switch from private to public, then no...we don't want that. We shouldn't remove the registrants if we convert a private task to public.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it'll remove current registrants if we switch to public. So just remove those two lines if you want otherwise

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, thanks

ajefts
ajefts reviewed Jan 4, 2017
View reviewed changes
Copy link
Contributor

@ajefts ajefts left a comment

Choose a reason for hiding this comment

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

@skyhit Overall the code looks reasonable to me. Let's get it into dev so we can test it. Speaking of testing, could we get some automated tests going?

@ajefts ajefts merged commit 138cabf into appirio-tech:dev Jan 4, 2017
@ajefts
Copy link
Contributor

ajefts commented Jan 4, 2017 •
edited

@skyhit @deedee I'm getting this error when I try to load a closed task in Online Review. I'm thinking we might be missing a step(s) when creating the fake submission?

  1. Close the Task in Direct.
  2. Load the challenge/task in Online Review.
2017-01-04 16:54:45,000 INFO  [OnlineReview] [* TonyJ * 52.21.65.101, 10.15.45.5 * GET http://software.topcoder-dev.com/review/actions/ViewProjectDetails?pid=30050481 *]
2017-01-04 16:54:45,000 INFO  [OnlineReview] 2017/01/04 16:54:45 -   User ID : 8547899  Action : ViewProjectDetails  Project ID : 30050481
2017-01-04 16:54:45,294 ERROR [onlinereview] com.topcoder.management.deliverable.persistence.UploadPersistenceException: Error loading upload.
com.topcoder.management.deliverable.persistence.UploadPersistenceException: Error loading upload.
	at com.topcoder.management.deliverable.persistence.sql.SqlUploadPersistence.loadUpload(SqlUploadPersistence.java:2268)
	at com.topcoder.management.deliverable.persistence.sql.SqlUploadPersistence.loadSubmission(SqlUploadPersistence.java:2203)
	at com.topcoder.management.deliverable.persistence.sql.SqlUploadPersistence.loadSubmissions(SqlUploadPersistence.java:1574)
	at com.topcoder.management.deliverable.PersistenceUploadManager.searchSubmissions(PersistenceUploadManager.java:915)
	at com.cronos.onlinereview.util.ActionsHelper.getProjectSubmissions(ActionsHelper.java:1687)
	at com.cronos.onlinereview.util.PhasesDetailsServices.getPhasesDetails(PhasesDetailsServices.java:135)
	at com.cronos.onlinereview.actions.projectdetails.ViewProjectDetailsAction.execute(ViewProjectDetailsAction.java:467)

@deedee
Copy link
Contributor

deedee commented Jan 5, 2017

Could you attach the full log.

BTW I've tried to run OR docker image as directed on this https://github.com/appirio-tech/tc-common-tutorials/tree/master/docker/online_review. But can not login and always got this
ERROR [OnlineReview] Unable to retrieve current user handle.

@skyhit
Copy link
Collaborator Author

skyhit commented Jan 5, 2017

@deedee is this related to informix database? does informix docker up properly?

@deedee
Copy link
Contributor

deedee commented Jan 5, 2017

Informix is up.

run-online-review_1 | 2017-01-05 08:50:01,580 ERROR [OnlineReview] Unable to retrieve current user handle.
run-online-review_1 | 2017-01-05 08:50:01,580 INFO [OnlineReview] [* * 172.19.0.1 * GET http://localhost/review/actions/ListProjects ]
run-online-review_1 | 2017-01-05 08:50:01,581 INFO [OnlineReview] 2017/01/05 08:50:01 - Action : ListProjects
run-online-review_1 | 2017-01-05 08:50:01,619 ERROR [OnlineReview] Unable to retrieve current user handle.
run-online-review_1 | 2017-01-05 08:50:01,620 INFO [OnlineReview] [ * 172.19.0.1 * GET http://localhost/review/actions/ListProjects?scope=all *]
run-online-review_1 | 2017-01-05 08:50:01,620 INFO [OnlineReview] 2017/01/05 08:50:01 - Action : ListProjects
run-online-review_1 | 2017-01-05 08:50:02,287 INFO [STDOUT] 2017.01.05 08:50:02 SQL Warning: 0, SQLState: 01I01
run-online-review_1 | 2017-01-05 08:50:02,287 INFO [STDOUT] 2017.01.05 08:50:02 Database has transactions
run-online-review_1 | 2017-01-05 08:50:02,288 INFO [STDOUT] 2017.01.05 08:50:02 SQL Warning: 0, SQLState: 01I04
run-online-review_1 | 2017-01-05 08:50:02,288 INFO [STDOUT] 2017.01.05 08:50:02 Database selected

@skyhit
Copy link
Collaborator Author

skyhit commented Jan 5, 2017

@deedee did you login? is the page shown properly?

@deedee
Copy link
Contributor

deedee commented Jan 5, 2017

@ajefts @skyhit please check this #190

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

@deedee deedee deedee left review comments

@ajefts ajefts ajefts approved these changes

Assignees

@ajefts ajefts

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@skyhit @ajefts @deedee