diff --git a/swh/deposit/cli/client.py b/swh/deposit/cli/client.py --- a/swh/deposit/cli/client.py +++ b/swh/deposit/cli/client.py @@ -199,25 +199,30 @@ slug = generate_slug() if not metadata: + if metadata_deposit: + raise InputError( + "Metadata deposit must be provided for metadata " + "deposit, either a filepath with --metadata or --name and --author" + ) if name and authors: metadata = generate_metadata_file(name, slug, authors, temp_dir) elif not archive_deposit and not partial and not deposit_id: # If we meet all the following conditions: - # * there is not an archive-only deposit + # * this is not an archive-only deposit request # * it is not part of a multipart deposit (either create/update # or finish) # * it misses either name or authors raise InputError( - "Either a metadata file (--metadata) or both --author and " - "--name must be provided, unless this is an archive-only " - "deposit." + "For metadata deposit request, either a metadata file with " + "--metadata or both --author and --name must be provided. " + "If this is an archive deposit request, none is required." ) elif name or authors: # If we are generating metadata, then all mandatory metadata # must be present raise InputError( - "Either a metadata file (--metadata) or both --author and " - "--name must be provided." + "For metadata deposit request, either a metadata file with " + "--metadata or both --author and --name must be provided." ) else: # TODO: this is a multipart deposit, we might want to check that @@ -225,8 +230,8 @@ pass elif name or authors: raise InputError( - "Using a metadata file (--metadata) is incompatible with " - "--author and --name, which are used to generate one." + "Using --metadata flag is incompatible with both " + "--author and --name (Those are used to generate one metadata file)." ) if metadata_deposit: @@ -235,12 +240,6 @@ if archive_deposit: metadata = None - if metadata_deposit and not metadata: - raise InputError( - "Metadata deposit must be provided for metadata " - "deposit (either a filepath or --name and --author)" - ) - if not archive and not metadata and partial: raise InputError( "Please provide an actionable command. See --help for more information" diff --git a/swh/deposit/tests/cli/test_client.py b/swh/deposit/tests/cli/test_client.py --- a/swh/deposit/tests/cli/test_client.py +++ b/swh/deposit/tests/cli/test_client.py @@ -187,9 +187,7 @@ ) -def test_cli_metadata_validation( - sample_archive, mocker, caplog, tmp_path, requests_mock_datadir -): +def test_cli_validation_metadata(sample_archive, mocker, caplog, tmp_path): """Multiple metadata flags scenario (missing, conflicts) properly fails the calls """ @@ -205,7 +203,129 @@ runner = CliRunner() - # Test missing author + for flag_title_or_name, author_or_name in [ + ("--author", "no one"), + ("--name", "test-project"), + ]: + # Test missing author then missing name + result = runner.invoke( + cli, + [ + "upload", + "--url", + "https://deposit.swh.test/1", + "--username", + TEST_USER["username"], + "--password", + TEST_USER["password"], + "--archive", + sample_archive["path"], + "--slug", + slug, + flag_title_or_name, + author_or_name, + ], + ) + + assert result.exit_code == 1, f"unexpected result: {result.output}" + assert result.output == "" + expected_error_log_record = ( + "swh.deposit.cli.client", + logging.ERROR, + ( + "Problem during parsing options: " + "For metadata deposit request, either a metadata file with " + "--metadata or both --author and --name must be provided. " + "If this is an archive deposit request, none is required." + ), + ) + assert expected_error_log_record in caplog.record_tuples + + # Clear mocking state + caplog.clear() + + # incompatible flags: Test both --metadata and --author, then --metadata and + # --name + result = runner.invoke( + cli, + [ + "upload", + "--url", + "https://deposit.swh.test/1", + "--username", + TEST_USER["username"], + "--password", + TEST_USER["password"], + "--name", + "test-project", + "--deposit-id", + 666, + "--archive", + sample_archive["path"], + "--slug", + slug, + ], + ) + assert result.exit_code == 1, f"unexpected result: {result.output}" + assert result.output == "" + expected_error_log_record = ( + "swh.deposit.cli.client", + logging.ERROR, + ( + "Problem during parsing options: " + "For metadata deposit request, either a metadata file with " + "--metadata or both --author and --name must be provided." + ), + ) + assert expected_error_log_record in caplog.record_tuples + + # Clear mocking state + caplog.clear() + + # incompatible flags check (Test both --metadata and --author, + # then --metadata and --name) + result = runner.invoke( + cli, + [ + "upload", + "--url", + "https://deposit.swh.test/1", + "--username", + TEST_USER["username"], + "--password", + TEST_USER["password"], + "--archive", + sample_archive["path"], + "--metadata", + metadata_path, + "--author", + "Jane Doe", + "--slug", + slug, + ], + ) + + assert result.exit_code == 1, result.output + assert result.output == "" + expected_error_log_record = ( + "swh.deposit.cli.client", + logging.ERROR, + ( + "Problem during parsing options: " + "Using --metadata flag is incompatible with both " + "--author and --name (Those are used to generate one metadata file)." + ), + ) + assert expected_error_log_record in caplog.record_tuples + caplog.clear() + + +def test_cli_validation_no_actionable_command(caplog): + """Multiple metadata flags scenario (missing, conflicts) properly fails the calls + + """ + runner = CliRunner() + # no actionable command result = runner.invoke( cli, [ @@ -216,59 +336,29 @@ TEST_USER["username"], "--password", TEST_USER["password"], - "--name", - "test-project", - "--archive", - sample_archive["path"], - "--slug", - slug, + "--partial", ], ) - assert result.exit_code == 1, f"unexpected result: {result.output}" + assert result.exit_code == 1, result.output assert result.output == "" expected_error_log_record = ( "swh.deposit.cli.client", logging.ERROR, ( - "Problem during parsing options: Either a metadata file" - " (--metadata) or both --author and --name must be provided, " - "unless this is an archive-only deposit." + "Problem during parsing options: " + "Please provide an actionable command. See --help for more information" ), ) assert expected_error_log_record in caplog.record_tuples - # Clear mocking state - caplog.clear() - # Test missing name - result = runner.invoke( - cli, - [ - "upload", - "--url", - "https://deposit.swh.test/1", - "--username", - TEST_USER["username"], - "--password", - TEST_USER["password"], - "--archive", - sample_archive["path"], - "--author", - "Jane Doe", - "--slug", - slug, - ], - ) +def test_cli_validation_missing_metadata_flag(caplog): + """--metadata-deposit requires --metadata (or --name and --author) otherwise fails - assert result.exit_code == 1, result.output - assert result.output == "" - assert expected_error_log_record in caplog.record_tuples - - # Clear mocking state - caplog.clear() - - # Test both --metadata and --author + """ + runner = CliRunner() + # --metadata-deposit without --metadata flag fails result = runner.invoke( cli, [ @@ -279,29 +369,66 @@ TEST_USER["username"], "--password", TEST_USER["password"], - "--archive", - sample_archive["path"], - "--metadata", - metadata_path, - "--author", - "Jane Doe", - "--slug", - slug, + "--metadata-deposit", # should fail because missing --metadata flag ], ) assert result.exit_code == 1, result.output assert result.output == "" - expected_error_log_record_2 = ( + expected_error_log_record = ( "swh.deposit.cli.client", logging.ERROR, ( - "Problem during parsing options: Using a metadata file " - "(--metadata) is incompatible with --author and --name, " - "which are used to generate one." + "Problem during parsing options: " + "Metadata deposit must be provided for metadata " + "deposit, either a filepath with --metadata or --name and --author" ), ) - assert expected_error_log_record_2 in caplog.record_tuples + assert expected_error_log_record in caplog.record_tuples + + +def test_cli_validation_replace_with_no_deposit_id_fails( + sample_archive, caplog, tmp_path, requests_mock_datadir, datadir +): + """--replace flags require --deposit-id otherwise fails + + """ + runner = CliRunner() + metadata_path = os.path.join(datadir, "atom", "entry-data-deposit-binary.xml") + + for flags in [ + ["--replace"], + ["--replace", "--metadata-deposit", "--archive-deposit"], + ]: + result = runner.invoke( + cli, + [ + "upload", + "--url", + "https://deposit.swh.test/1", + "--username", + TEST_USER["username"], + "--password", + TEST_USER["password"], + "--metadata", + metadata_path, + "--archive", + sample_archive["path"], + ] + + flags, + ) + + assert result.exit_code == 1, result.output + assert result.output == "" + expected_error_log_record = ( + "swh.deposit.cli.client", + logging.ERROR, + ( + "Problem during parsing options: " + "To update an existing deposit, you must provide its id" + ), + ) + assert expected_error_log_record in caplog.record_tuples def test_cli_single_deposit_slug_generation(