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

Update data validation for publisher commentActions endpoint #1

Merged
merged 2 commits into from
Jul 13, 2017

Conversation

ericagreene
Copy link
Collaborator

Description

There is a set of /publisher/commentActions/:action endpoints that we use to send comment decisions from CRNR to Moderator. It's basically the same as an /api/commentActions/:action endpoints, but it was split out at some point to insulate the publisher comment flow from other Moderator API changes.

Motivation and Context

When we switched from int IDs to string IDs, we updated the validation layer for the API commentActions endpoint, but not the publisher commentActions endpoint, which still requires number IDs (publisher version here vs regular api version here).

The other small change is to log errors for this endpoint. Right now they're swallowed and it makes debugging a little tricky.

How Has This Been Tested?

I manually tested it by running the backend-api locally and posting a comment to the /publisher/commentActions/approve endpoint.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have added tests to cover my changes.

@ericagreene ericagreene requested a review from iislucas July 12, 2017 23:36
Copy link
Collaborator

@iislucas iislucas left a comment

Choose a reason for hiding this comment

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

One suggested fix to make sure logs appear in stackdriver, and that you use the existing logger.

res.json({ status: 'success' });
next();
} catch (e) {
console.log(e)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably wants to be a console.error? Probably you want to use the logging code...

import { logger } from '@conversationai/moderator-backend-core';

...

logger.error(e);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, good point. I'll update it.

@ericagreene
Copy link
Collaborator Author

@iislucas I updated the logger, so this should be good to go.

@ericagreene ericagreene merged commit 5522f2f into master Jul 13, 2017
RufusHamade referenced this pull request in RufusHamade/conversationai-moderator-1 Sep 10, 2018
* Adds drone.yml with a build step

* Adds publish and deploy steps

* Trigger pub/deploy from setup branch

* Add google secrets

* Improve deploy

* Add recommended autoscaling and monitoring

* Specify client build

* Make npm install noisy for debugging

* Temporarily diable npm install for sane build debugging

* Debug publish flow

* Debug publish flow

* Add Dockerfile

* Resilence npm install

* Only 1 replica for dev

* Update port

* Update .app var name

* Update app name to avoid collisions with existing app

* Turning off dryrun... let's do this...

* Update to us.grc.io

* Debug

* Debug image pull

* Update image name

* Reintroduce npm install

* Removing silent from npm install to see why it is failing on drone

* Reintroducing silent... it just took 28 minutes...

* Update Dockerfile and port

* Copy frontend-web in dockerfile

* Introduce scripts from ugc-moderator

* Add shared cert

* Debug

* Remove GAE app.yaml generator

* Update drone sec

* Stop using npm packages

* Remove drone.bash

* Remove extraneous files

* Add google client id and secret placeholders

* npm install backend-core deps too

* Update client id dummy value so drone doesn't convert from int to string

* Also npm install the config package

* Try removing index to circumvent dist-commonjs require error

* Return to npm package pathing

* lerna install and build via drone

* Lerna bootstrap, not build

* Disable backend-api build script

* Add healthz endpoint and configure probes

* Update config to overwrite existing moderator client

* Add sanity check token to prove code source

* Removing sanity check token

* Restore correct triggers

* Add test block

* Silence npm install

* Debug tests block

* Add prd publish and deploy

* Remove tests from deploy

* Update README.md

* Remove npm modules & .sh pruning line

* Clearer placeholder values for unused google client vars

* Undo beautify formatting

* More undoing of prettification

* Revert, then recreate changes to undo prettify

* Slaying the last of the automatic updates
RufusHamade added a commit that referenced this pull request Mar 21, 2019
@sorensenjs sorensenjs deleted the fix-publisher-commentactions-data-validation branch October 13, 2020 10:31
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.

2 participants