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.

Sign up for GitHub

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

Fix 2FA prompt when the package exists #505

Merged
merged 24 commits into from
Apr 27, 2020
Merged

Fix 2FA prompt when the package exists #505

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
e3d6c40
Allow for explicitly publishing private packages
Feb 12, 2020
efa0001
Booleanify variable name
Feb 12, 2020
b291752
Converge booleans into option
Feb 12, 2020
09e0fea
Update documentation
Feb 12, 2020
226c33a
Fix lint errors
Feb 12, 2020
74e91df
Provide defaults for options for the sake of tests
Feb 12, 2020
2816eb1
Remove unnecessary default
Feb 12, 2020
283cc27
Don't ask to publish publically if explicitly setting access to restr…
Feb 12, 2020
4d254c4
Adjust documentation
Feb 12, 2020
a4becb3
Change variable name
Feb 14, 2020
988260a
Remove unnecessary sentence from README
kylemh Feb 18, 2020
bdf7aad
Revert change to default values
kylemh Feb 20, 2020
3c51a26
Define defaults for test env
Feb 20, 2020
3eee2c8
Move test options into test file
Feb 21, 2020
a831ed9
Fix 2fa prompt when the package is existed
chinesedfan Feb 22, 2020
44129cc
Merge branch 'master' into allow-explicit-private-publish
kylemh Feb 22, 2020
74267ff
Merge branch 'allow-explicit-private-publish' into fix-exists
chinesedfan Feb 24, 2020
3e2e094
Add test case
chinesedfan Feb 24, 2020
26bd2c6
Merge branch 'master' into fix-exists
chinesedfan Feb 27, 2020
37d06f8
Refactor without preview mode
chinesedfan Feb 27, 2020
e80c54c
Fix the missing stub
chinesedfan Feb 27, 2020
4b20623
Fix the stub
chinesedfan Feb 27, 2020
37fa6cf
Existed means not available
chinesedfan Mar 4, 2020
8076b1a
Update index.js
sindresorhus Apr 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion source/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,10 @@ updateNotifier({pkg: cli.pkg}).notify();

const runPublish = flags.publish && !pkg.private;

const availability = await isPackageNameAvailable(pkg);
const availability = flags.publish ? await isPackageNameAvailable(pkg) : {
isAvailable: false,
isUnknown: false
};

const version = cli.input.length > 0 ? cli.input[0] : false;

Expand Down
2 changes: 1 addition & 1 deletion source/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ module.exports = async (input = 'patch', options) => {
]);

const isExternalRegistry = npm.isExternalRegistry(pkg);
if (!options.exists && !pkg.private && !isExternalRegistry) {
if (!options.availability.isAvailable && !options.availability.isUnknown && !pkg.private && !isExternalRegistry) {
chinesedfan marked this conversation as resolved.
Show resolved Hide resolved
tasks.add([
{
title: 'Enabling two-factor authentication',
Expand Down
33 changes: 32 additions & 1 deletion test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
import test from 'ava';
import sinon from 'sinon';
import proxyquire from 'proxyquire';
import np from '../source';

const defaultOptions = {
cleanup: true,
tests: true,
publish: true,
runPublish: true
runPublish: true,
availability: {
isAvailable: false,
isUnknown: false
}
};

test('version is invalid', async t => {
Expand All @@ -28,3 +34,28 @@ test('errors on too low version', async t => {
await t.throwsAsync(np('1.0.0', defaultOptions), /New version `1\.0\.0` should be higher than current version `\d+\.\d+\.\d+`/);
await t.throwsAsync(np('1.0.0-beta', defaultOptions), /New version `1\.0\.0-beta` should be higher than current version `\d+\.\d+\.\d+`/);
});

test('should not enable 2fa if the package exists', async t => {
chinesedfan marked this conversation as resolved.
Show resolved Hide resolved
const enable2faStub = sinon.stub();
const np = proxyquire('../source', {
del: sinon.stub(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need all of these stubs? Can't we run np with --yolo and remove the need for most of them? While I do want the tests to reflect actual stability, I think it's more important in this case to be as specific as possible, and only test what we need to verify - that we skip enabling 2FA when not needed to do so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

--yolo can only remove del and execa. I used to want to use --preview, but it would skip the enable2fa task totally. And I don't think adding stubs will affect the correctness of the test case. Maybe because I consider it as unit test, while you treat it as end-to-end test.

execa: sinon.stub().returns({pipe: sinon.stub()}),
'./prerequisite-tasks': sinon.stub(),
'./git-tasks': sinon.stub(),
'./git-util': {
hasUpstream: sinon.stub().returns(true),
push: sinon.stub()
},
'./npm/enable-2fa': enable2faStub,
'./npm/publish': sinon.stub().returns({pipe: sinon.stub()})
});

await t.notThrowsAsync(np('1.0.0', {
...defaultOptions,
availability: {
isAvailable: true,
isUnknown: false
}
}));
t.is(enable2faStub.notCalled, true);
});