-
Notifications
You must be signed in to change notification settings - Fork 35
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
Journal Event fixing bug front/back end #1304
Conversation
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.
Hi Yvan! Thanks so much for this great and very comprehensive PR! Really appreciate all the effort you're putting into this :)
I haven't finished reviewing it yet, but thought I'd submit the few comments I had so far just so I don't block your progress too much. Will try to finish the rest ASAP!
@@ -14,6 +14,7 @@ class Outcome(db.Model): | |||
id = db.Column(db.Integer(), primary_key = True, nullable = False) | |||
event_id = db.Column(db.Integer(), db.ForeignKey('event.id'), nullable = False) | |||
user_id = db.Column(db.Integer(), db.ForeignKey('app_user.id'), nullable = False) | |||
response_id = db.Column(db.Integer(), db.ForeignKey('response.id'), nullable=False) ## add response_id |
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 remove the comments "## add response_id" and "## add relationship"
@@ -28,5 +28,19 @@ def get_latest_for_event(event_id): | |||
@staticmethod | |||
def add(outcome): | |||
db.session.add(outcome) | |||
|
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.
It's probably worth removing the get_*_for_event methods now that outcome is linked to a response? That way we can also ensure all the relevant code paths are updated.
for response in responses: | ||
outcome = outcome_repository.get_latest_by_user_for_event_response(current_user_id,response.id, event_id) | ||
# get_latest_by_user_for_event(current_user_id, event_id) |
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 delete commented-out code
@@ -146,8 +147,8 @@ def post(self): | |||
user = user_repository.get_by_id(user_id) | |||
responses = response_repository.get_all_for_user_application(user_id, application_form_id) | |||
|
|||
if not application_form.nominations and len(responses) > 0: | |||
return errors.RESPONSE_ALREADY_SUBMITTED | |||
# if not application_form.nominations and len(responses) > 0: |
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.
I think we still need this check - some events and application forms should still only support a single response and some should support multiple responses and we should validate this is the case. How about changing the name nominations
to allows_many_responses
to make it more generic? Then default allows_many_responses
to True for event-type journal
@@ -0,0 +1,22 @@ | |||
"""empty message |
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 remove empty migration.
@@ -0,0 +1,22 @@ | |||
"""empty message |
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 remove empty migration
@@ -0,0 +1,22 @@ | |||
"""empty message |
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 remove empty migration
@@ -65,14 +65,20 @@ class EventStatus extends Component { | |||
applicationStatus = (event) => { | |||
const applyLink = `${event.key}/apply` | |||
const submissionLink = `${event.key}/apply/new` | |||
const viewLink=`${event.key}/apply/view` | |||
console.log(event); |
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 remove debug console logs
@@ -91,6 +97,19 @@ class EventStatus extends Component { | |||
|
|||
} | |||
else if (event.status.application_status === "Withdrawn") { | |||
if (event.event_type === "JOURNAL"){ |
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.
It seems like we need some bigger changes here to properly cater for multiple submissions with the event statusus. Currently this doesn't make sense that the event application status is "withdrawn" but the status here says "You have submitted your article"
@@ -147,7 +147,8 @@ const FormCreator = ({ | |||
/> | |||
{homeRedirect && <Redirect to={`/${eventKey}`} />} | |||
{errorResponse ? ( | |||
<div className='tooltiptext-error response-error'> | |||
// tooltiptext-error response-error |
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.
Thanks for the fix! Please remove commented code
Hi @avishkar58 , |
@@ -0,0 +1,408 @@ | |||
/* Universal */ |
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.
Is all of this additional CSS really needed? Also there seems to be a some very generally selectors like html button which should not belong inside a component-specific CSS file.
@@ -0,0 +1,121 @@ | |||
import React, { Component } from "react"; |
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.
This component doesn't seem to be used? Also seems to be a copy of ResponsePage/components/ReviewModal.js ?
response={selectedResponse} | ||
event={this.props.event} | ||
/> | ||
); | ||
} |
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.
In future, please can you keep formatting changes in a separate PR that only does formatting. When formatting changes are mixed up with other logic changes it makes it very hard for a reviewer to see what has actually changed
<Route | ||
exact | ||
path={`${match.path}/apply/new`} | ||
render={(props) => <Application {...props} event={event} journalSubmissionFlag={true} /> } | ||
/> | ||
|
||
{/*new route */} |
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.
Remove uneccessary comment
@@ -204,9 +204,13 @@ const ReviewForm = (props) => { | |||
active: currentStage !== 1 ? true : false | |||
}) | |||
setCreateMode(true); | |||
console.log(event); |
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.
Remove debug logging
@@ -85,6 +85,7 @@ function submit(applicationFormId, isSubmitted, answers) { | |||
|
|||
return axios.post(baseUrl + `/api/v1/response`, response, {headers: authHeader()}) | |||
.then(resp=> { | |||
console.log() |
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.
REmove
What's new?