diff --git a/CHANGELOG.md b/CHANGELOG.md index a679c5de..80d95b13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,11 +51,11 @@ Format: ### Fixed -- Now delete interviews upon interview completion +- Now delete interviews upon interview completion for signed in users ### Internal -- Test deleting interviews upon interview completion +- Test deleting interviews upon interview completion for signed in users - Added @temp_error to neutralize tests that are getting undesired errors right now, but which we anticipate being fixed soon. We want to ignore them for a short while. For example, an upstream change temporarily breaking some tests. ## [5.12.0] - 2024-06-06 diff --git a/docassemble/ALKilnTests/data/sources/da_api.feature b/docassemble/ALKilnTests/data/sources/da_api.feature index 60ad5e0c..08a03604 100644 --- a/docassemble/ALKilnTests/data/sources/da_api.feature +++ b/docassemble/ALKilnTests/data/sources/da_api.feature @@ -2,23 +2,18 @@ Feature: I use the docassemble API @fast @da1 @signin @sessions @delete -Scenario: After signing in, I delete an interview - Given I sign in with the email "USER1_EMAIL" and the password "USER1_PASSWORD" +Scenario: After signing in, I successfully delete an interview + Given I sign in with the email "USER1_EMAIL", "USER1_PASSWORD", "USER1_API_KEY" And I start the interview at "all_tests" # Must continue to get interview into "My interviews" list And I tap to continue And I start the interview at "all_tests" - # Must continue to get interview into "My interviews" list And I tap to continue Then of two interviews, I delete one __internal__ @fast @da2 @sessions @delete -Scenario: Without signing in, I delete an interview +Scenario: As an anonymous user, I silently fail to delete an interview Given I start the interview at "all_tests" - # Must continue to get interview into "My interviews" list - And I tap to continue - And I start the interview at "all_tests" - # Must continue to get interview into "My interviews" list + # Must continue to theoretically get interview into "My interviews" list And I tap to continue - Then of two interviews, I delete one __internal__ - #Then there are no interview_vars saved __internal__ + Then as an anonymous user, I fail to delete an interview and get no error __internal__ diff --git a/lib/docassemble/docassemble_api_REST.js b/lib/docassemble/docassemble_api_REST.js index 3f3f06bd..b91e4b67 100644 --- a/lib/docassemble/docassemble_api_REST.js +++ b/lib/docassemble/docassemble_api_REST.js @@ -54,16 +54,16 @@ da.delete_project = async function ( timeout ) { // node -e 'require("./lib/docassemble/docassemble_api_REST.js").delete_interview(...)' -da.delete_interview = async function ( timeout, { uid = null }) { +da.delete_interview = async function ( timeout, { session = null, api_key = null }) { /* Delete an interview with the given session id. */ let options = { method: 'DELETE', - headers: { 'X-API-Key': session_vars.get_dev_api_key() }, + headers: { 'X-API-Key': api_key }, // Can't use `api/session` that uses the API key to identify the user's // interview sessions. The test may have signed in with an account // that's different than the ALKiln dev account that sends this request. - url: `${ session_vars.get_da_server_url() }/api/interviews`, - params: { session: uid }, + url: `${ session_vars.get_da_server_url() }/api/user/interviews`, + params: { session }, timeout: timeout, }; @@ -76,16 +76,16 @@ da.delete_interview = async function ( timeout, { uid = null }) { }; // Ends da.delete_interview() // node -e 'require("./lib/docassemble/docassemble_api_REST.js").get_interview(...)' -da.get_interview = async function ( timeout, { uid = null }) { +da.get_interview = async function ( timeout, { session = null, api_key = null }) { /* Get a specific interview. */ let options = { method: 'GET', - headers: { 'X-API-Key': session_vars.get_dev_api_key() }, + headers: { 'X-API-Key': api_key }, // Can't use `/api/user/interviews` that uses the API key to identify the user's // interview sessions. The test may have signed in with an account // that's different than the ALKiln dev account that sends this request. - url: `${ session_vars.get_da_server_url() }/api/interviews`, - params: { session: uid }, + url: `${ session_vars.get_da_server_url() }/api/user/interviews`, + params: { session }, timeout: timeout, }; diff --git a/lib/docassemble/docassemble_api_interface.js b/lib/docassemble/docassemble_api_interface.js index 74c4d8fc..ae0e6ebb 100644 --- a/lib/docassemble/docassemble_api_interface.js +++ b/lib/docassemble/docassemble_api_interface.js @@ -201,48 +201,75 @@ da_i.delete_project = async function ( delete_options ) { }; // Ends da_i.delete_project() // node -e 'require("./lib/docassemble/docassemble_api_interface.js").quietly_try_to_delete_interviews(...)' -da_i.quietly_try_to_delete_interviews = async function ({ interviews = {}, options = {} }) { - /** Delete a user's interview. Do we need to watch for a timeout? */ +da_i.quietly_try_to_delete_interviews = async function ({ interviews = {}, api_keys = [], options = {} }) { + /** Try to delete the given interviews. Fail silently. Example failure: the test + * did not log in any users and the testing account's role prevents + * access to other user's interviews. Do we need to watch for a timeout? */ // Defaults let { timeout } = options || { timeout: DEFAULT_MAX_REQUEST_MS }; + let responses = []; for ( let id in interviews ) { - let interview_vars = interviews[ id ]; - if ( interview_vars && interview_vars.uid ) { - try { - log.debug({ type: `da api info`, code: `ALK0188`, pre: `Trying to delete the interview with the id ${ interview_vars.uid }.` }); - - // Actual work: - let response = await _da_REST.delete_interview( timeout, interview_vars ); - log.debug({ type: `da api info`, code: `ALK0189`, - pre: `No error when trying to delete interview ${ interview_vars.uid } at ${ interview_vars.i }.` - }); - - } catch ( error ) { - error.interview_vars = interview_vars; - let msg = `Error when deleting the interview. See https://docassemble.org/docs/api.html#session_delete.` - log.debug({ type: `da api info`, code: `ALK0190`, pre: msg, data: error }); - } + let interview = interviews[ id ]; + if ( interview && interview.uid ) { + let sub_responses = await da_i.quietly_delete_one_interview( timeout, { + interview, + api_keys + }); + responses.push( sub_responses ); } else { log.debug({ type: `da api info`, code: `ALK0191`, pre: `There is no interview to delete.` }); } } + return responses; }; // Ends da_i.quietly_try_to_delete_interviews() +// node -e 'require("./lib/docassemble/docassemble_api_interface.js").quietly_delete_one_interview(...)' +da_i.quietly_delete_one_interview = async function ( timeout, { interview = {}, api_keys = [] }) { + /** Try to delete an interview using all available API keys. Fail silently. + * Example failure: the test did not log in any users and the testing + * account's role prevents access to other user's interviews. */ + api_keys = normalize_api_keys( api_keys ); + let responses = []; + for ( let api_key of api_keys ) { + try { + log.debug({ type: `da api info`, code: `ALK0188`, + pre: `Trying to delete the interview with the id ${ interview.uid } and an API key.` + }); + + // Actual work: + let response = await _da_REST.delete_interview( timeout, { + session: interview.uid, + api_key + }); + responses.push( response ); + + log.debug({ type: `da api info`, code: `ALK0189`, + pre: `No error when trying to delete interview ${ interview.uid } at ${ interview.i }.` + }); + } catch ( error ) { + error.interview = interview; + let msg = `Error when deleting the interview. See https://docassemble.org/docs/api.html#session_delete.` + log.debug({ type: `da api info`, code: `ALK0190`, pre: msg, data: error }); + } + } // ends for api keys + return responses; +}; // Ends da_i.quietly_delete_one_interview() + // node -e 'require("./lib/docassemble/docassemble_api_interface.js").get_interviews(...)' -da_i.get_interviews = async function ({ interviews = {}, options = {} }) { +da_i.get_interviews = async function ({ interviews = {}, api_keys = [], options = {} }) { /** Return an object of interviews that exist on the server * that match the given interviews. Keys are the session ids. */ - // Defaults - let { timeout } = options || { timeout: DEFAULT_MAX_REQUEST_MS }; + + api_keys = normalize_api_keys( api_keys ); let fetched_interviews = {}; if ( interviews ) { for ( let id in interviews ) { - let interview_vars = interviews[ id ]; - let fetched = await da_i.get_interview({ interview_vars }); + let interview = interviews[ id ]; + let fetched = await da_i.get_interview({ interview, api_keys, options }); if ( fetched && fetched.session ) { fetched_interviews[ fetched.session ] = fetched; } @@ -253,30 +280,36 @@ da_i.get_interviews = async function ({ interviews = {}, options = {} }) { }; // Ends da_i.get_interviews() // node -e 'require("./lib/docassemble/docassemble_api_interface.js").get_interview(...)' -da_i.get_interview = async function ({ interview_vars, options = {} }) { +da_i.get_interview = async function ({ interview = {}, api_keys = [], options = {} }) { /** Try to get an interview from the server that matches the given interview data. */ // Defaults let { timeout } = options || { timeout: DEFAULT_MAX_REQUEST_MS }; + api_keys = normalize_api_keys( api_keys ); - if ( interview_vars && interview_vars.uid ) { - try { - log.debug({ type: `da api info`, code: `ALK0192`, pre: `Trying to get the interview with the id ${ interview_vars.uid }.` }); + if ( interview && interview.uid ) { + for ( let api_key of api_keys ) { + try { + log.debug({ type: `da api info`, code: `ALK0192`, pre: `Trying to get the interview with the id ${ interview.uid } and an API key.` }); - // Actual work: - let response = await _da_REST.get_interview( timeout, interview_vars ); - if ( response && response.data && response.data.items && response.data.items[0] && response.data.items[0].session ) { - log.debug({ type: `da api info`, code: `ALK0193`, pre: `Status ${ response.status }. Got ${ response.data.items[0].session }.`}); - return response.data.items[0]; - } else { - log.debug({ type: `da api info`, code: `ALK0194`, pre: `Status ${ response.status }. Cannot find ${ interview_vars.uid }`}); - } + // Actual work: + let response = await _da_REST.get_interview( timeout, { + session: interview.uid, + api_key + }); - } catch ( error ) { - error.interview_vars = interview_vars; - let msg = `${ error.status } "${ error.statusText }" Ran into an error when getting the interview ${ interview_vars.uid }. See https://docassemble.org/docs/api.html#interviews.` - log.debug({ type: `da api info`, code: `ALK0195`, pre: msg, data: error }); - } + if ( response && response.data && response.data.items && response.data.items[0] && response.data.items[0].session ) { + log.debug({ type: `da api info`, code: `ALK0193`, pre: `Status ${ response.status }. Got ${ response.data.items[0].session }.`}); + return response.data.items[0]; + } else { + log.debug({ type: `da api info`, code: `ALK0194`, pre: `Status ${ response.status }. Cannot find ${ interview.uid }`}); + } + } catch ( error ) { + error.interview = interview; + let msg = `${ error.status || `No error.status.` } ${ error.statusText || error.data || `No error.statusText.` } Ran into an error when getting the interview ${ interview.uid }. See https://docassemble.org/docs/api.html#interviews.` + log.debug({ type: `da api info`, code: `ALK0195`, pre: msg, data: error }); + } + } // ends for api_keys } else { log.debug({ type: `da api info`, code: `ALK0196`, pre: `There is no interview to get.` }); } @@ -284,3 +317,11 @@ da_i.get_interview = async function ({ interview_vars, options = {} }) { return null; }; // Ends da_i.get_interview() + +let normalize_api_keys = function ( api_keys = [] ) { + /** Ensure the dev API key is in the api_keys list. */ + let dev_key = session_vars.get_dev_api_key(); + if ( !api_keys ) { api_keys = [ dev_key ]; } + if ( !api_keys.includes( dev_key ) ) { api_keys.unshift( dev_key ); } + return api_keys; +}; diff --git a/lib/scope.js b/lib/scope.js index 45c42b64..82a5e7ef 100644 --- a/lib/scope.js +++ b/lib/scope.js @@ -2938,7 +2938,9 @@ module.exports = { await scope.afterStep(scope, {waitForShowIf: true}); }, // Ends scope.steps.set_al_address() - sign_in: async function ( scope, { email_secret_name, password_secret_name }) { + sign_in: async function ( scope, { + email_secret_name, password_secret_name, api_key_secret_name + }) { /** Allow the developer to log a user into their server using GitHub * secrets to authenticate. */ @@ -2991,6 +2993,16 @@ module.exports = { if ( password === undefined ) { reports.addToReport( scope, { type: `error`, code: `ALK0162`, value: password_msg }) } + // Save API key to clean up interviews later. Nice to have, not critical + if ( api_key_secret_name ) { + let api_key = process.env[ api_key_secret_name ]; + let api_key_msg = `The api_key GitHub SECRET "${ api_key_secret_name }" doesn't exist. Check for a typo and check your workflow file.` + if ( api_key === undefined ) { + reports.addToReport( scope, { type: `warning`, code: `ALK0199`, value: api_key_msg }); + } else { + scope.scenarios.get( scope.scenario_id ).api_keys.push( api_key ); + } + } expect( email, email_msg ).to.not.equal( undefined ); expect( password, password_msg ).to.not.equal( undefined ); diff --git a/lib/steps.js b/lib/steps.js index 01676ac6..8ab15550 100644 --- a/lib/steps.js +++ b/lib/steps.js @@ -140,6 +140,7 @@ Before(async (scenario) => { scope.scenario_id = uuidv4(); scope.scenarios.set( scope.scenario_id, {} ); scope.scenarios.get( scope.scenario_id ).interviews = {}; + scope.scenarios.get( scope.scenario_id ).api_keys = [ session_vars.get_dev_api_key() ]; scope.expected_status = null; scope.expected_in_report = null; scope.expected_not_in_report = null; @@ -280,7 +281,10 @@ Given(/I go to "([^"]+)"/i, {timeout: -1}, async ( url ) => { return result; }); -Given(/I (?:sign|log) ?(?:in)?(?:on)?(?:to the server)? with(?: the email)? "([^"]+)" and(?: the password)? "([^"]+)"(?: SECRETs)?/i, {timeout: -1}, async ( email, password ) => { +Given( + /I (?:sign|log) ?(?:in)?(?:on)?(?:to the server)? with(?: the email)? "([^"]+)",?(?: and)?(?: the password)? "([^"]+)"(?: SECRETs)?(?:,?(?: and)?(?: the API key)? "([^"]+)")?/i, + { timeout: -1 }, + async ( email, password, api_key ) => { /** Uses the names of environment variables (most often GitHub SECRETs) to * log into an account on the user's server. Must be SECRETs to stay secure. * @@ -288,13 +292,16 @@ Given(/I (?:sign|log) ?(?:in)?(?:on)?(?:to the server)? with(?: the email)? "([^ * I sign into the server with the email "USER1_EMAIL" and the password "USER1_PASSWORD" SECRETs * I sign on with the email "USER1_EMAIL" and the password "USER1_PASSWORD" SECRETs * I log in with "USER1_EMAIL" and "USER1_PASSWORD" + * I sign in with the email "USER1_EMAIL", the password "USER1_PASSWORD", and the API key "USER1_API_KEY" + * I sign in with "USER1_EMAIL", "USER1_PASSWORD", "USER1_API_KEY" */ // `page` timeout will deal with custom timeout // Couldn't test the timeout for tapping the button because there's not // enough of a pause between initial navigation and pressing button. await scope.steps.sign_in( scope, { email_secret_name: email, - password_secret_name: password + password_secret_name: password, + api_key_secret_name: api_key }); }); @@ -1190,24 +1197,26 @@ Then(/of two interviews, I delete one __internal__/i, async () => { */ // Make sure we have two interviews in `scope.scenario.interviews` (two session ids) let interviews = scope.scenarios.get( scope.scenario_id ).interviews; - let internal_keys = Object.keys( interviews ); + let interview_keys = Object.keys( interviews ); expect( - internal_keys.length, - `Of 2 interview sessions, ALKiln only saved ${ internal_keys.length }` + interview_keys.length, + `Of 2 interview sessions, ALKiln only saved ${ interview_keys.length }` ).to.equal( 2 ); // Check that both interviews exist on the server. 1 √ 2 √ - let existing_interviews = await da_api.get_interviews({ interviews }); + let api_keys = scope.scenarios.get( scope.scenario_id ).api_keys; + let existing_interviews = await da_api.get_interviews({ interviews, api_keys }); let existing_keys = Object.keys( existing_interviews ); expect( existing_keys.length, - `2 interviews should be on the server. Instead: ${ existing_keys.join(', ') }` + `2 interviews should be on the server. Instead: "${ existing_keys.join(', ') }"` ).to.equal( 2 ); // Delete one. 1 x 2 √ - let to_delete = interviews[ internal_keys[ 0 ]]; + let to_delete = interviews[ interview_keys[ 0 ]]; await da_api.quietly_try_to_delete_interviews({ - interviews: { [internal_keys[ 0 ]]: to_delete } + interviews: { [interview_keys[ 0 ]]: to_delete }, + api_keys }); // Check that only one session exists on the server. This tests that @@ -1215,7 +1224,7 @@ Then(/of two interviews, I delete one __internal__/i, async () => { // 2. We are fetching interviews correctly because we are getting one // back (e.g. not passing null into the api function) // 1 x 2 √ - let after_deletion = await da_api.get_interviews({ interviews }); + let after_deletion = await da_api.get_interviews({ interviews, api_keys }); let keys_after_deletion = Object.keys( after_deletion ); expect( keys_after_deletion.length, @@ -1224,16 +1233,29 @@ Then(/of two interviews, I delete one __internal__/i, async () => { // In After(), the other session should get deleted }); -// Then(/there are no interview_vars saved __internal__/i, async () => { -// /** Test that no interviews were saved internally */ -// // Check the number of interviews -// let interviews = scope.scenarios.get( scope.scenario_id ).interviews; -// let keys = Object.keys( interviews ); -// expect( -// keys.length, -// `0 interviews should exist, but at least one does: ${ keys.join(', ') }` -// ).to.equal( 0 ); -// }); +Then(/as an anonymous user, I fail to delete an interview and get no error __internal__/i, async () => { + /** Test that an interview passes even when unable to delete an interview. + * An API test + */ + // Make sure we started an interview + let interviews = scope.scenarios.get( scope.scenario_id ).interviews; + expect( + Object.keys( interviews ).length, + `The test should have saved one interview session` + ).to.equal( 1 ); + + // There should be no error about an unauthorized user + let api_keys = scope.scenarios.get( scope.scenario_id ).api_keys; + let existing_interviews = await da_api.get_interviews({ interviews, api_keys }); + let existing_keys = Object.keys( existing_interviews ); + expect( + existing_keys.length, + `Anonymous user should detect 0 interviews on the server. Instead: "${ existing_keys.join(', ') }"` + ).to.equal( 0 ); + + // Deleting with an anonymous user should fail silently + await da_api.quietly_try_to_delete_interviews({ interviews, api_keys }); +}); //##################################### @@ -1446,25 +1468,33 @@ After(async function(scenario) { // Now that we're no longer on the interview page, one way or // another, try to delete the interviews created during the test - try { - let interviews = scope.scenarios.get( scope.scenario_id ).interviews; - await da_api.quietly_try_to_delete_interviews({ interviews }); - let existing_interviews = await da_api.get_interviews({ interviews }); - let existing_keys = Object.keys( existing_interviews ); - // Deleting interviews is nice, but not non-critical. Don't bother the - // user with it. Don't want to cover up a more meaningful failure (like - // going to the wrong file). If it becomes a problem we can track it down later. - if ( existing_keys.length > 0 ) { - log.debug({ type: `internal warning`, code: `ALK0197`, - pre: `${ existing_keys.length } interviews were not deleted: ${ existing_keys.join( ', ' ) }` - }); + // Authors running this manually get to choose what to do with + // the interviews they're creating unless we hear otherwise. + if ( session_vars.get_origin() !== `playground` ) { + try { + let interviews = scope.scenarios.get( scope.scenario_id ).interviews; + let api_keys = scope.scenarios.get( scope.scenario_id ).api_keys; + await da_api.quietly_try_to_delete_interviews({ interviews, api_keys }); + let existing_interviews = await da_api.get_interviews({ interviews, api_keys }); + let existing_keys = Object.keys( existing_interviews ); + // Deleting interviews is nice, but not non-critical. Don't bother the + // user with it. Don't want to cover up a more meaningful failure (like + // going to the wrong file). If it becomes a problem we can track it down later. + if ( existing_keys.length > 0 ) { + log.debug({ type: `internal warning`, code: `ALK0197`, + pre: `${ existing_keys.length } interviews were not deleted: ${ existing_keys.join( ', ' ) }` + }); + } + } catch ( error ) { + // Deleting interviews is nice, but not non-critical. Don't bother the + // user with it. Don't want to cover up a more meaningful failure (like + // going to the wrong file). Example of OK failure: the user owns the + // interview is different than the API key that's trying to delete the + // interview. If it becomes a problem we can track it down later. + // Consider add "internal" to docs: "If you see this, make an + // issue in ALKiln with your debug/verbose log" + log.debug({ type: `internal warning`, code: `ALK0198`, pre: `Error while cleaning up interviews.`, data: error }); } - } catch ( error ) { - // Deleting interviews is nice, but not non-critical. Don't bother the - // user with it. Don't want to cover up a more meaningful failure (like - // going to the wrong file). If it becomes a problem we can track it down later. - // Consider add "internal" to docs: "If you see this, make an issue in ALKiln with your debug/verbose log" - log.debug({ type: `internal warning`, code: `ALK0198`, pre: `Error while cleaning up interviews.`, data: error }); } // Handle all the accumulated errors diff --git a/package.json b/package.json index b8400d98..7307c7af 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@suffolklitlab/alkiln", - "version": "5.12.0-delete1", + "version": "5.12.0-delete2", "description": "Integrated automated end-to-end testing with docassemble, puppeteer, and cucumber.", "main": "lib/index.js", "scripts": {