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 @@ -198,26 +198,26 @@ if not slug: # generate one as this is mandatory slug = generate_slug() - if not metadata: + if not metadata and not metadata_deposit and not replace: 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 +225,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: @@ -238,7 +238,7 @@ if metadata_deposit and not metadata: raise InputError( "Metadata deposit must be provided for metadata " - "deposit (either a filepath or --name and --author)" + "deposit, either a filepath with --metadata or --name and --author" ) if not archive and not metadata and partial: 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,7 +187,7 @@ ) -def test_cli_metadata_validation( +def test_cli_validation_metadata( sample_archive, mocker, caplog, tmp_path, requests_mock_datadir ): """Multiple metadata flags scenario (missing, conflicts) properly fails the calls @@ -205,7 +205,131 @@ 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( + sample_archive, mocker, caplog, tmp_path, requests_mock_datadir +): + """Multiple metadata flags scenario (missing, conflicts) properly fails the calls + + """ + runner = CliRunner() + # no actionable command result = runner.invoke( cli, [ @@ -216,32 +340,31 @@ 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 +def test_cli_validation_missing_metadata_flag( + sample_archive, mocker, caplog, tmp_path, requests_mock_datadir +): + """Multiple metadata flags scenario (missing, conflicts) properly fails the calls + + """ + runner = CliRunner() + # --metadata-deposit without --metadata flag fails result = runner.invoke( cli, [ @@ -252,23 +375,31 @@ TEST_USER["username"], "--password", TEST_USER["password"], - "--archive", - sample_archive["path"], - "--author", - "Jane Doe", - "--slug", - slug, + "--metadata-deposit", ], ) assert result.exit_code == 1, result.output assert result.output == "" + expected_error_log_record = ( + "swh.deposit.cli.client", + logging.ERROR, + ( + "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 in caplog.record_tuples - # Clear mocking state - caplog.clear() - # Test both --metadata and --author +def test_cli_validation_replace_with_no_deposit_id_fails( + sample_archive, mocker, caplog, tmp_path, requests_mock_datadir +): + """Missing --deposit-id when --replace is passed fails + + """ + runner = CliRunner() result = runner.invoke( cli, [ @@ -279,29 +410,21 @@ TEST_USER["username"], "--password", TEST_USER["password"], - "--archive", - sample_archive["path"], - "--metadata", - metadata_path, - "--author", - "Jane Doe", - "--slug", - slug, + "--replace", ], ) 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: " + "To update an existing deposit, you must provide its id" ), ) - assert expected_error_log_record_2 in caplog.record_tuples + assert expected_error_log_record in caplog.record_tuples def test_cli_single_deposit_slug_generation(