Move "docid" from the URL to a JSON payload #233
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
epugh
added a commit
that referenced
this pull request
Nov 2, 2020
* swap JS side over to using payloads for doc ids * refactor to be more railsy is working
epugh
added a commit
that referenced
this pull request
Dec 7, 2020
* swap JS side over to using payloads for doc ids * refactor to be more railsy is working
worleydl
added a commit
that referenced
this pull request
Jan 17, 2021
* Broken dependencies * OMG this is partly working! * checkpoint * checkopint, fighjting with the any_of stuff * yeomans work on progress towards eliminating any_of, had to use raw SQL unfortuanntly * fixing up more tests! * clean up gems * lets run api tests first * try and run rails before js * one more try * one more try * fixing more tests * frustrated with circleci * so many fixes * more fixes * Welcome back jquery * fixes! * one more * more fixes * more datamodeling fun, especially caused by a force param that we dont use * upgrade for the cop * rubo * more fixes * more fixes * rubocopy you are so needy, upgrade you * started doing more rubocop * onemore * fixin * fixed * magic fix * for some reason after_create is now working * fix up * turns out we have better vlidation now in rails 5 then in 4 * getting closer * update doc * fixes * bumping to updated Ruby that Heroku uses, and removing 12factor since Rails 5 provides what Heroku wants * one more * get tests to run again * get all tests to run * we got rid of using any_of, so now remove gem * doh * rubocop * update * need to fix this (#223) * Move "docid" from the URL to a JSON payload (#233) * swap JS side over to using payloads for doc ids * refactor to be more railsy is working * Add import ltr format (#204) * hotfix: k=5 despite scores being changed to @10, i.e k should be 10 * the errant comma at the end of the line loading the scorer code was causing it to not run * Turns out we were updating a non communal ndcg scorer, hence not seeing the update for the communal * document fixes * fixes #178 with some messaging (#235) * document fix * turns out we need to work around the default DELETE in angular * document the bug * use fancier defaults to demo capablity (#236) * found two tickets fixed by same pr * add teams owner controller tests and add check for ownership before a… (#239) * add teams owner controller tests and add check for ownership before allowing it to change * new unit test uncovered deprecated method use Co-authored-by: Jacob Graves <jgraves@tableau.com> Co-authored-by: epugh <epugh@opensourceconnections.com> * control signup enablement via config (#238) * add customization SIGNUP_ENABLED defaults to true. currently just disables the controller, but does not change the frontend. * read SIGNUP_ENABLED in js app Pass it through the configuration service, similarly to the email marketing mode flag. Only display the signup div if signup is enabled. * add tests for configurationSvc.isSignupEnabled * Provide explicit defaults. I don't know if I like or dislike the way we do the defaults for environment variables, but lets keep following the pattern. Having the environment variable in the docker-compose.prod.yml reminds folks it's there, likewise in the .env.example. Co-authored-by: epugh <epugh@opensourceconnections.com> * uncovered a old debugging statement that leaves a undefined message in the console * Renaming a Case on the Teams Listing Page doesn't work (#240) * uncovered a old debugging statement that leaves a undefined message in the console * hey, we fixed it, but now have two ways of renaming a case * removing watches seems to actually let updates happen * jshint * remove old method * update unit test * found a lingering .rename * jshint * remove commented code * easy in place case name editing (#242) * easy in place case name editing * went ahead and made the try editable as well * Issue 231: Fixing CSV Injection (#245) * Issue 231: Fixing CSV Injection When a query starts with =,-,+,@ and is exported to csv, it can cause malicious commands to be ran when imported elsewhere. This fix adds a space for these queries when it is exported. This has been updated in the erb and the javascript to cover both cases. * Clean up syntax with rubocop * document vulnerability fix * often we add improvemnts, so make it an option * 232 stored xss - add new env flag to forbid user scorers and only allow communal scorers (#246) * wire up new flag and behaviour it controls * add tests for scorers controller to ensure respecting new environment flag * fix ruby syntax issues * switch to true boolean Co-authored-by: Jacob Graves <jgraves@tableau.com> * document fixes * Harden export import cycle for queries no docs (#252) * fixing up export import to deal with queries that had no docs * fix up the snapshot * rubocop * finalize rubocop * be smarter on generating ratings * rubocop * Remove sharing from communal scorers (#251) * document fix * document vulnerability fix * only show sharing for custom scorers * doc fix * doc fix * Initialize scorer.scaleWithLabels (#254) * Issue 225 (#253) * patch up nDCG logic for less than 10 result to fix #234 * fix #225 for AP@10 being inccorect if less than 10 docs are returned * mergin * one more fix Co-authored-by: epugh@opensourceconnections.com <> * mergin * fixes * roll * testing fix * harden test * rubocop * rubocop * fix up for now * rubocop * rubocop * rubocop * rubcop * rubocop * so much rubocop * more rubocop * fixin * backing out message cause this is still used * tweaks * ignore a rule * more rubo * rubocop * rubo * bug fix * update * Deal with cases not auto being created, so we need to create explictly a case. Go ahead and pretty up the output! * remove the CurrentUserDecorator, we don't actually need it.. looks like just a bit of syntactic sugar, and I think the abstraction doesn't actually help us. * while I like firstTime better, I'd rather have consistency on both sides! * rubocop * optimization of sprockets for files that only matter in test * Trying to resolve the gzip issue in running test:js led me down a path of upgrades.. webconsole burped, and we never use it, so ripping it out! * small optimization * explicit bundler version install * tweak name! * rubocop * fixes * Much better name than user.case for method. I thought I could get rid of the Finder classes, but apparently not! * docs * clean up! * rubocop! * fix ups * more fix up * one more fix * deal with bigint being default in rails 5, and we don't want that. * update docs * Strip out Spring since we are in a container world and don't get the benefit * rubo * rails 5 is stricter on params! * dan agrees we should simplify * solve the try_id versus try_number issue * small fix to prevent messages showing up till page has loaded all angular this only showed up in dev mode because of the long compile time, but was very confusion! * kill a constant we don't use * old debuggin * change up default, add some debuggin * rubo * Quit creating dummy Try Number 0, and instead start with 1, and at the end of the wizard, just update it in place. Dont' constantly genrate a try 0, then to 1 at the end of the wizard! * Update UPGRADING_RAILS_NOTES.md * use our new put method! * jshint * Some code from the dawn of Quepid that isn't used! * suspected this change would happen! * some per fixes * doc fixe * rubo * looks like we have some upgrades that elminiate this issue! * Test fix Co-authored-by: Daniel Worley <danworley@gmail.com> Co-authored-by: epugh@opensourceconnections.com <> Co-authored-by: jacobgraves <jacobgraves@hotmail.com> Co-authored-by: Jacob Graves <jgraves@tableau.com> Co-authored-by: Michael Lawrence <mikeklawrence@gmail.com> Co-authored-by: nicholaskwan <nkwan@tableau.com> Co-authored-by: Nathan (Nate) Day <nathancday@gmail.com>
worleydl
added a commit
that referenced
this pull request
Jan 17, 2021
* Broken dependencies * OMG this is partly working! * checkpoint * checkopint, fighjting with the any_of stuff * yeomans work on progress towards eliminating any_of, had to use raw SQL unfortuanntly * fixing up more tests! * clean up gems * lets run api tests first * try and run rails before js * one more try * one more try * fixing more tests * frustrated with circleci * so many fixes * more fixes * Welcome back jquery * fixes! * one more * more fixes * more datamodeling fun, especially caused by a force param that we dont use * upgrade for the cop * rubo * more fixes * more fixes * rubocopy you are so needy, upgrade you * started doing more rubocop * onemore * fixin * fixed * magic fix * for some reason after_create is now working * fix up * turns out we have better vlidation now in rails 5 then in 4 * getting closer * update doc * fixes * bumping to updated Ruby that Heroku uses, and removing 12factor since Rails 5 provides what Heroku wants * one more * get tests to run again * get all tests to run * we got rid of using any_of, so now remove gem * doh * rubocop * update * need to fix this (#223) * Move "docid" from the URL to a JSON payload (#233) * swap JS side over to using payloads for doc ids * refactor to be more railsy is working * Add import ltr format (#204) * hotfix: k=5 despite scores being changed to @10, i.e k should be 10 * the errant comma at the end of the line loading the scorer code was causing it to not run * Turns out we were updating a non communal ndcg scorer, hence not seeing the update for the communal * document fixes * fixes #178 with some messaging (#235) * document fix * turns out we need to work around the default DELETE in angular * document the bug * use fancier defaults to demo capablity (#236) * found two tickets fixed by same pr * add teams owner controller tests and add check for ownership before a… (#239) * add teams owner controller tests and add check for ownership before allowing it to change * new unit test uncovered deprecated method use Co-authored-by: Jacob Graves <jgraves@tableau.com> Co-authored-by: epugh <epugh@opensourceconnections.com> * control signup enablement via config (#238) * add customization SIGNUP_ENABLED defaults to true. currently just disables the controller, but does not change the frontend. * read SIGNUP_ENABLED in js app Pass it through the configuration service, similarly to the email marketing mode flag. Only display the signup div if signup is enabled. * add tests for configurationSvc.isSignupEnabled * Provide explicit defaults. I don't know if I like or dislike the way we do the defaults for environment variables, but lets keep following the pattern. Having the environment variable in the docker-compose.prod.yml reminds folks it's there, likewise in the .env.example. Co-authored-by: epugh <epugh@opensourceconnections.com> * uncovered a old debugging statement that leaves a undefined message in the console * Renaming a Case on the Teams Listing Page doesn't work (#240) * uncovered a old debugging statement that leaves a undefined message in the console * hey, we fixed it, but now have two ways of renaming a case * removing watches seems to actually let updates happen * jshint * remove old method * update unit test * found a lingering .rename * jshint * remove commented code * easy in place case name editing (#242) * easy in place case name editing * went ahead and made the try editable as well * Issue 231: Fixing CSV Injection (#245) * Issue 231: Fixing CSV Injection When a query starts with =,-,+,@ and is exported to csv, it can cause malicious commands to be ran when imported elsewhere. This fix adds a space for these queries when it is exported. This has been updated in the erb and the javascript to cover both cases. * Clean up syntax with rubocop * document vulnerability fix * often we add improvemnts, so make it an option * 232 stored xss - add new env flag to forbid user scorers and only allow communal scorers (#246) * wire up new flag and behaviour it controls * add tests for scorers controller to ensure respecting new environment flag * fix ruby syntax issues * switch to true boolean Co-authored-by: Jacob Graves <jgraves@tableau.com> * document fixes * Harden export import cycle for queries no docs (#252) * fixing up export import to deal with queries that had no docs * fix up the snapshot * rubocop * finalize rubocop * be smarter on generating ratings * rubocop * Remove sharing from communal scorers (#251) * document fix * document vulnerability fix * only show sharing for custom scorers * doc fix * doc fix * Initialize scorer.scaleWithLabels (#254) * Issue 225 (#253) * patch up nDCG logic for less than 10 result to fix #234 * fix #225 for AP@10 being inccorect if less than 10 docs are returned * mergin * one more fix Co-authored-by: epugh@opensourceconnections.com <> * mergin * fixes * roll * testing fix * harden test * rubocop * rubocop * fix up for now * rubocop * rubocop * rubocop * rubcop * rubocop * so much rubocop * more rubocop * fixin * backing out message cause this is still used * tweaks * ignore a rule * more rubo * rubocop * rubo * bug fix * update * Deal with cases not auto being created, so we need to create explictly a case. Go ahead and pretty up the output! * remove the CurrentUserDecorator, we don't actually need it.. looks like just a bit of syntactic sugar, and I think the abstraction doesn't actually help us. * while I like firstTime better, I'd rather have consistency on both sides! * rubocop * optimization of sprockets for files that only matter in test * Trying to resolve the gzip issue in running test:js led me down a path of upgrades.. webconsole burped, and we never use it, so ripping it out! * small optimization * explicit bundler version install * tweak name! * rubocop * fixes * Much better name than user.case for method. I thought I could get rid of the Finder classes, but apparently not! * docs * clean up! * rubocop! * fix ups * more fix up * one more fix * deal with bigint being default in rails 5, and we don't want that. * update docs * Strip out Spring since we are in a container world and don't get the benefit * rubo * rails 5 is stricter on params! * dan agrees we should simplify * solve the try_id versus try_number issue * small fix to prevent messages showing up till page has loaded all angular this only showed up in dev mode because of the long compile time, but was very confusion! * kill a constant we don't use * old debuggin * change up default, add some debuggin * rubo * Quit creating dummy Try Number 0, and instead start with 1, and at the end of the wizard, just update it in place. Dont' constantly genrate a try 0, then to 1 at the end of the wizard! * Update UPGRADING_RAILS_NOTES.md * use our new put method! * jshint * Some code from the dawn of Quepid that isn't used! * suspected this change would happen! * some per fixes * doc fixe * rubo * looks like we have some upgrades that elminiate this issue! * had to upgrade to Rails 5, but now we can try inviting users! * make matching on names case insentsive * first cut of adding invitations. * looking up by user name and email was too slow! * have the invite accept screen working! * lifecycle test is working! * rubocop! * rubocop * not sure why current_team_finder.rb was different than current_case_finder.rb however makign this change as one of our tests was flagging it as causing extra unrequired loads! also makes it the same as the others! * making same as others, and making it not happen unless called! * this appears to matter on circle ci, and we don't need it to be a variable. * Update test.rb * rubo * deal with the signup flag * now dealing with t and c and agreed time. * no longer need the true on callsbacks in rails 5.. * finally get arms around the boolean beting string or bool! * polish up emails * Properly track who has a pending invite! * track links * notes * figured out we were attempting to diff snapshot with deleted query docs * small fix * use better regex to pluck out the raw invite token. Thanks Nate! * Missed conflict Co-authored-by: Daniel Worley <danworley@gmail.com> Co-authored-by: epugh@opensourceconnections.com <> Co-authored-by: jacobgraves <jacobgraves@hotmail.com> Co-authored-by: Jacob Graves <jgraves@tableau.com> Co-authored-by: Michael Lawrence <mikeklawrence@gmail.com> Co-authored-by: nicholaskwan <nkwan@tableau.com> Co-authored-by: Nathan (Nate) Day <nathancday@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When rating documents, the doc_id is somethign the user controls, and can be any string. We historically have had it in the URL, which meant things like
.or/caused issues. For those, we would base64 encode the doc id, and then on the server side do some checks and then if it was base64, then decode it.However, we have found examples where the docid would erronosnly trigger the "is_base64" logic on the server side.
In digging, I noticed that the bulk rating capablity just passes the doc_id as part of the json payload, avoiding the URL issues.
Motivation and Context
This refactoring fixes #228 and makes the code work more like other quepid apis.
How Has This Been Tested?
manually and unit testing.
Screenshots or GIFs (if appropriate):
Types of changes
Checklist: