Skip to content

Pioneer: Applying for storage working group opening and final "Working groups" tab issues#742

Merged
mnaamani merged 21 commits intoJoystream:nicaeafrom
Lezek123:storage-working-group-applying
Jul 8, 2020
Merged

Pioneer: Applying for storage working group opening and final "Working groups" tab issues#742
mnaamani merged 21 commits intoJoystream:nicaeafrom
Lezek123:storage-working-group-applying

Conversation

@Lezek123
Copy link
Contributor

Implements: #597

Note that the listing of user's active applications for storage working group etc. is not implemented yet, as there is another issue for that (#618)

ts-ignore issue
While working on this issue I removed the ts-ignore inside /joy-roles/src/flows/apply.controller.tsx as it is a very bad practice to use it, because it silences a lot of potential TypeScript errors during the build. It should have actually been caught by eslinter's ban-ts-ignore rule, but for some reason it wasn't (perhaps because it wasn't added as a valid-syntax comment, but actually a string value inside jsx).

This forced me to fix a few typing errors inside /joy-roles/src/flows/apply.tsx that were revealed once the ts-ignore was removed.

Other pre-existing UI issues
I also decided to list all the pre-existing Working Group tab issues I was able to find in #741 to allow better understanding of the problems that may occur during testing, but were not actually introduced in this PR (I'll try to tackle those once the Working Groups tab is fully adjusted to support Storage Working Group)

@Lezek123 Lezek123 requested a review from mnaamani June 19, 2020 14:28
@Lezek123 Lezek123 marked this pull request as draft June 23, 2020 13:16
@Lezek123 Lezek123 changed the title Pioneer: Applying for storage working group opening Pioneer: Applying for storage working group opening and final "Working groups" tab issues Jun 23, 2020
@Lezek123 Lezek123 force-pushed the storage-working-group-applying branch from 26b8dbf to 9d28469 Compare June 23, 2020 13:19
@Lezek123
Copy link
Contributor Author

Lezek123 commented Jun 23, 2020

Updated to include #618 and some of the fixes from #741:

working-groups/my-roles page should now be fully adjusted to work with Storage Working Group. Although in issue #618 it wasn't planned to make it fully compatible (active roles were not considered), I decided to go for it, since it was very easy to implement at this point and makes the Pioneer's support for Storage Working Group complete.

Issues described in #741:

  • Fixed the issue with neverending loading screen in case there are no openings available.
  • Fixed the applying flow for openings with variable stakes and max. number of applicants specified. User should now be able to correcty determine the minimum stake he needs to specify to be considered when applying for such openings, as well as his position in the "applicants hierarchy" during the first step of the application. Note that if both stakes are set to AtLeast, a different component handles this step than when only one of them is set to AtLeast.
  • Fixed role key name generation issue (keys with duplicate names should not be generated anymore)

@Lezek123 Lezek123 marked this pull request as ready for review June 23, 2020 13:59
@Lezek123 Lezek123 mentioned this pull request Jun 26, 2020
@Lezek123 Lezek123 force-pushed the storage-working-group-applying branch from 9d28469 to 0ca5864 Compare June 29, 2020 16:14
Copy link
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

I left a comment about using correct type in one of the tx calls.

Tested the UI. Only thing I noticed that is incorrect (perhaps its existing behavior) but when appliccations are withdrawn, the total number of applications on an opening still counts the withdrawn applications.

Otherwise all works as expected.

@Lezek123
Copy link
Contributor Author

Added fixes according to the comments.

I noticed that there was a ts-nocheck clause inside Admin.controller.tsx which silenced a lot of useful TypeScript errors. I fixed those and it seems to have simultaneously fixed a few UI issues with the opening creation form at working-groups/admin (ie. there were some problems with changing the stake type between AtLeast and Exact, changing stake values etc. and they seem to be gone now).

I added ban-ts-comment rule to eslint to possibly avoid running into this problem again. There is one more occurrence of ts-ignore in /packages/react-components/src/AddressCard.tsx (which was purposefully silenced, so I just updated the eslint-disable-next-line clause there) and 2 ts-nocheck occurrences in storybook-related files inside joy-roles (which I silenced for now with eslint-disable-next-line, but marked with TODO: FIXME:)

@Lezek123
Copy link
Contributor Author

Lezek123 commented Jun 30, 2020

Only thing I noticed that is incorrect (perhaps its existing behavior) but when appliccations are withdrawn, the total number of applications on an opening still counts the withdrawn applications.

I don't think I was chaning anything there, it looks like this number just represents total number of applications regardless of their current status (which can be Active, Unstaking or Inactive). It is also displayed for already filled openings etc., so it makes some sense, but I understand that pairing it with max_active_applications can be confusing and indicate that this number means active applications.

@Lezek123
Copy link
Contributor Author

Lezek123 commented Jun 30, 2020

I actually found another issue related to those non-active applications.

Because the status of them wasn't checked inside myApplicationRank and openingApplicationRanks methods, it caused unreliable rank perdiction and incorrect minStake value when applying in case one of the previous applicants has withdrawn its application.

Should be fixed now.

@Lezek123 Lezek123 requested a review from mnaamani June 30, 2020 14:31
@mnaamani
Copy link
Member

mnaamani commented Jul 7, 2020

I created an opening with an empty {} json for the human readable text so when I visit the my-roles page it crashes with the following error (I presume its due to not finding the expected properties on the parsed object):

Screen Shot 2020-07-07 at 10 25 42 AM

Maybe we should handle badly formatted human readable text.

@Lezek123 Lezek123 marked this pull request as draft July 7, 2020 10:19
@Lezek123
Copy link
Contributor Author

Lezek123 commented Jul 7, 2020

Updated with some UI adjustments following "Make lead hireable" changes (#726) that I already had on my branch locally.

Also merged nicaea to fix conflicts and added a method to parse human_readable_text with fallback to valid GenericJoyStreamRoleSchema object that has all required values set to Unknown in case the actual json is invalid.

@Lezek123 Lezek123 marked this pull request as ready for review July 7, 2020 11:00
'new-cap': 'off',
'@typescript-eslint/interface-name-prefix': 'off'
'@typescript-eslint/interface-name-prefix': 'off',
'@typescript-eslint/ban-ts-comment': 'error'
Copy link
Member

Choose a reason for hiding this comment

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

This is a good rule to have:

'@typescript-eslint/ban-ts-comment': 'error'

might add it to #891 cc: @Gamaranto @kdembler

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good one in general but I would use the option allow-with-description so we can use ts-ignore in specific cases. Recently I've worked with a library with stale typings and had to ignore typescript error to use some newer option.

@@ -1,3 +1,5 @@
// TODO: FIXME: Remove the ts-nocheck and fix errors!
Copy link
Member

Choose a reason for hiding this comment

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

It better to create github issues rather than TODO's in the code. We can label them with good first issue for contributors to find something simple and easy to work on.

const tx = this.api.tx.contentWorkingGroup.acceptCuratorApplications(new u32(id));
const tx = this.api.tx.contentWorkingGroup.acceptCuratorApplications(id);
// FIXME: That's a bad way to send extrinsic in Pioneer (without "queueExtrinsic" etc.)
// and probably the reason why it always appears as succesful
Copy link
Member

Choose a reason for hiding this comment

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

I'm sure here you mean that you are not handling case when the transaction returns an error, but is still finalized?

@mnaamani
Copy link
Member

mnaamani commented Jul 8, 2020

Works as expected.
One enhancement that I would suggest is in the my-roles, to show in the current roles, the worker id.
This is needed by the storage providers when running their node.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants