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

Fixing voting #371

Closed
wants to merge 1 commit into from
Closed

Fixing voting #371

wants to merge 1 commit into from

Conversation

ineiti
Copy link
Member

@ineiti ineiti commented Sep 28, 2023

Using the endpoint 'forms/:formID/vote' doesn't work with the sendToDela method

Using the endpoint 'forms/:formID/vote' doesn't work with the sendToDela method
@ineiti ineiti requested a review from a team as a code owner September 28, 2023 09:11
@ineiti ineiti self-assigned this Sep 28, 2023
Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

Minor remarks :)

Comment on lines +245 to +248
if (!req.session.userId) {
res.status(401).send('Authentication required!');
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this will never be called based on the code in line 235-237.
That error should probably return a 401, like you've done here.

@@ -263,6 +238,30 @@ delaRouter.use('/*', (req, res) => {
}

const bodyData = req.body;

// special case for voting
const match = req.baseUrl.match('/api/evoting/forms/(.*)/vote');
Copy link
Contributor

Choose a reason for hiding this comment

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

why not set this up as a specific endpoint ? It would be more idiomatic, unless I'm missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what we did, but then sendToDela fails, because it uses baseUrl, which is different in delaRouter.Post and delaRouter.use.

Comment on lines +260 to +262
// DEBUG: this is only for debugging and needs to be replaced before production
console.warn('DEV CODE - randomizing the SCIPER ID to allow for unlimited votes');
bodyData.UserID = makeid(10);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use a global env. variable to define if we're in debug or not, instead of commenting out ? I'm afraid something like this will make its way back into production someday when someone's looking in the other direction :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely better to use a global variable. One of the problems is that we cannot use NODE_ENV === 'development', as we currently need to run the frontend in development mode due to the proxying.

@ineiti
Copy link
Member Author

ineiti commented Oct 4, 2023

Closing this in favour of c4dt#20

@ineiti ineiti closed this Oct 4, 2023
@ineiti ineiti mentioned this pull request Oct 4, 2023
@ineiti ineiti deleted the fix_votes branch October 4, 2023 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To Review
Development

Successfully merging this pull request may close these issues.

2 participants