Skip to content

Cli: Create working group commands related to openings and applications#707

Merged
mnaamani merged 16 commits intoJoystream:nicaeafrom
Lezek123:cli-working-groups-opening
Jul 7, 2020
Merged

Cli: Create working group commands related to openings and applications#707
mnaamani merged 16 commits intoJoystream:nicaeafrom
Lezek123:cli-working-groups-opening

Conversation

@Lezek123
Copy link
Contributor

@Lezek123 Lezek123 commented Jun 16, 2020

Implements #599 - the functionality to create a working group opening and save it as a reusable draft.

Examples:

  • ./cli/bin/run working-groups:createOpening --help
  • ./cli/bin/run working-groups:createOpening - create an opening from scratch (requires lead account to be selected via account:choose). User will be asked to provide all addWorkerOpening extrinsic parameters along with human_readable_text parameters (based on schema), for which I created approperiate Structs to "simulate" identical behavior that is implemented when asking for normal struct parameters (like OpeningPolicyCommitment) that may have nested Vec, Option or other Structs inside. Opening created this way can be saved as draft after for later reuse.
  • ./cli/bin/run working-groups:createOpening -d - when providing -d or --useDraft flag without -n, the user is prompted to choose one of the previosly created drafts. When creating opening from draft, all the prompts will have default values equal to those provided when the draft was created, but all values can still be overriden along the way if needed.
  • ./cli/bin/run working-groups:createOpening -d -n="Some draft name" - in this example the draft name is provided using -n or --draftName flag (it's case insensitive).
  • ./cli/bin/run working-groups:createOpening -c - using -c or --createDraftOnly flag allows creating an opening draft without executing the actual extrinsic. This flag can also be specified with -d flag to create a draft based on an existing draft, but it's exculsive with -s (described below)
  • ./cli/bin/run working-groups:createOpening -d -s - when -s or --skipPrompts flag is provided (which also requires -d), all the extrinsic parameters' prompts will be skipped and the extrinsic will be sent with the exact same parameters as provided in a draft. This basically equals to executing ./cli/bin/run working-groups:createOpening -d and pressing [Enter] all the way until the extrinsic is sent (which would cause all prompts to "assume" the values provided as defaults). The -s flag is exclusive with -c for obvious reasons.

While implementing this functionality I was trying to do it in the most reusable way possible. Thanks to the fact that prompting for all (or almost all?) possible types of substrate arguments is now implemented (along with the functionality to fetch arguments' types based on extrinsic module and method), this can be easily re-used to handle input for any extrinsic. That will allow us to later implement commands like api:extrinsic (similar to Pioneer's Extrinsics tab) and greatly simplify the creation of future commands that will at least partially rely on extrinsics.

Prompting for JSON values that are provided to the extrinsic as Bytes, but "backed" by an existing schema (like human_readable_text) is also something that was implemented in a pretty reusable way. The only thing that's required in order to handle those correctly is creating approperiate Structs based on the schema. If needed in the future - some script may be created that will convert schemas into Structs (it's a pretty mindless process)

@Lezek123 Lezek123 marked this pull request as draft June 16, 2020 16:02
@Lezek123 Lezek123 added CLI Command Line Interface Tool nicaea labels Jun 16, 2020
@Lezek123 Lezek123 force-pushed the cli-working-groups-opening branch from d479798 to 98b059f Compare June 18, 2020 09:18
@Lezek123 Lezek123 marked this pull request as ready for review June 18, 2020 09:19
@Lezek123 Lezek123 requested a review from mnaamani June 18, 2020 09:19
@Lezek123
Copy link
Contributor Author

Fixed the conflicts and changed the CLI dependency from "@joystream/types": "./types" to "@joystream/types": "^0.11.0".

@Lezek123
Copy link
Contributor Author

Lezek123 commented Jun 23, 2020

Updated the PR to include new commands based on issue #600:

  • working-groups:openings - lists all openings ever created in given working group (optionally specified with -g or --group which always defaults to storageProviders for all working-groups commands). This only includes IDs and some basic information about the current status of the opening.
  • working-groups:opening WORKER_OPENING_ID - displays detailed information about the opening by WORKER_OPENING_ID (which can be obtained using the previous command). Those details include: formatted human readable text (json), detailed status, information about required stakes, list of applications.
  • working-groups:application WORKER_APPLICATION_ID - displays detailed information about working group opening application by WORKER_APPLICATION_ID (which can be obtained using the previous command).

@Lezek123 Lezek123 changed the title Cli: Create working group opening command Cli: Create working group opening and openings/applications listing and details commands Jun 23, 2020
@Lezek123 Lezek123 marked this pull request as draft June 24, 2020 08:37
@Lezek123 Lezek123 changed the title Cli: Create working group opening and openings/applications listing and details commands Cli: Create working group commands related to openings and applications Jun 24, 2020
@Lezek123 Lezek123 marked this pull request as ready for review June 24, 2020 08:48
@Lezek123
Copy link
Contributor Author

Updated to include commands from #601:

  • working-groups:startAcceptingApplications WORKER_OPENING_ID - allows changing the opening status from Waiting to Begin to Accepting Applications
  • working-groups:startReviewPeriod WORKER_OPENING_ID - allows changing the opening status from Accepting Applications to In Review
  • working-groups:terminateApplication WORKER_APPLICATION_ID - allows terminating an active applicaition.
  • working-groups:fillOpening WORKER_OPENING_ID - allows filling an opening if it's currently in the Review stage. User (lead) can select succesful applications from the list containing active worker application IDs with related member handles. The review process will probably include running commands like working-groups:opening (displays the list of applications among other details) and working-groups:application (displays application details) before selecting succesful applicants. User can also optionally specify the recurring reward parameters.

cli/src/Api.ts Outdated
stageBlock: number | undefined,
stageDate: Date | undefined;

if (stage.type === OpeningStageKeys.WaitingToBegin) {
Copy link
Member

Choose a reason for hiding this comment

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

I think a better typesafe way of doing the comparison would be by using a helper methods or getters on the OpeningStage type to look something like:

## bool property
stage.isWaitingToBegin

or

# I think the Enum type has an eq method
stage.eq(OpeningStage.WaitingToBeing)

# WaitingToBeing would be a static method on OpeningStage:
static WaitingToBeing() : OpeningStage  {
  return new OpeningStage('WaitingToBeing')
}

I've seen multiple instances where the comparison of the enum variant is done in this way.

Copy link
Member

Choose a reason for hiding this comment

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

of course I'm simplifying here to express the idea, clearly the variant has encapsulated data as well which I'm disregarding

Copy link
Member

Choose a reason for hiding this comment

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

if I remember correctly, we might even get some of these helper properties as part of the Enum type. I've seen some use of it in the polkadot-js/apps code base.

cli/src/Api.ts Outdated
async groupOpening(group: WorkingGroups, workerOpeningId: number): Promise<GroupOpening> {
const nextId = ((await this.workingGroupApiQuery(group).nextWorkerOpeningId()) as WorkerOpeningId).toNumber();

if (workerOpeningId < 0 || workerOpeningId >= nextId) {
Copy link
Member

Choose a reason for hiding this comment

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

I've used this strategy as well and as a guard it works well as longs as the map has all keys in the range.
This is not the case for the WorkerById map for example. For that map I had to deal with it differently in the storage-node package

Basically we need a way to check if a key exists in a map.
I've seen you in pioneer code base rely on .isEmpty property for example for a linked value to determine if the value actually existed in the map. That works perfectly for the value type in that case, but it can't really work in general if indeed a struct type will all fields with the "zero" value is actually a valid value.

So we should probably have a common interface/helper methods for reading map, linked_map, double_map state

default: defaultValue ? entries.length < defaultValue.length : false
});
const defaultEntryValue = defaultValue && defaultValue[entries.length];
if (addAnother) entries.push(await this.promptForParam(subtype.type, subtype.name, defaultEntryValue));
Copy link
Member

Choose a reason for hiding this comment

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

Its good defensive style to always use curly braces with if else statemetns

typeDef.name = forcedName;
}

if (rawTypeDef.info === TypeDefInfo.Option) return await this.promptForOption(typeDef, defaultValue as Option<Codec>);
Copy link
Member

Choose a reason for hiding this comment

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

I highly recommend using curly braces to avoid future bugs when editing this code, or even using a switch statement if possible?

@Lezek123 Lezek123 mentioned this pull request Jun 26, 2020
@Lezek123 Lezek123 marked this pull request as draft June 26, 2020 12:02
@Lezek123 Lezek123 force-pushed the cli-working-groups-opening branch from 44f83f5 to 37266eb Compare June 26, 2020 12:19
@Lezek123
Copy link
Contributor Author

Lezek123 commented Jun 26, 2020

Updated to include #728, #823 and related adjustments. It should be possible to test how it would work after the new runtime upgrade (#728) now.

@Lezek123
Copy link
Contributor Author

Added fixes based on review comments.

Introduced new JoyEnum helper in order to provide a better, TypeScript-compatible interface for Enum types similar to Enum.with() (which unfortunately isn't TypeScript-compatible).

The only minor difference is that instead of getters like isActive we use isOfType('Active') (types will be auto-suggested) and instead of asActive - asType('Active') (which would also be auto-suggested and result in correctly typed value)

@Lezek123 Lezek123 marked this pull request as ready for review June 29, 2020 16:25
@mnaamani
Copy link
Member

mnaamani commented Jul 7, 2020

When prompted for exact block I just pressed enter without entering a value. It wasn't clear to me what value was used.
So as per your comment: "all the prompts will have default values equal to those provided when the draft was created,"

So it would be good to show what actual value will be used in the transaction. I see that happens for
Provide value for max_review_period_length 1000 for example because its just a number, but not for structs.

$ /Users/mokhtar/joystream/joystream/node_modules/.bin/joystream-cli working-groups:createOpening -d
 Select a draft Draft 2
? Your account's password [hidden]
  Choose value for activate_at: ExactBlock
  Provide value for ExactBlock 

Similarly for entering an amount for stake amount:

    Do you want to provide the optional application_staking_policy parameter? Yes
      Providing values for {
        amount:u128
        amount_mode:{"_enum":["AtLeast","Exact"]}
        crowded_out_unstaking_period_length:Option<u32>
        review_period_expired_unstaking_period_length:Option<u32>
      } struct:
        Provide value for amount 
        Choose value for amount_mode: Exact

Last suggestion would be to add a final summary of the values that will be sent in the transaction and prompting user to confirm.

I have to say creating the opening is really nice through the command line now, really nice job.

@mnaamani
Copy link
Member

mnaamani commented Jul 7, 2020

Please include those UX/Feedback suggestions in upcoming cli PRs,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLI Command Line Interface Tool nicaea

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants