Adding the ability to Party Up! #24

mpherman · 2019-07-05T03:42:21Z

This PR will allow users to create a shared group leaderboard or 'party' where they can compare multiple grails against each other.
Each Party can be registered similarly to a Holy Grail sheet by clicking on the register button on the party home page (./party).
There is a tab to Join the party and (if you are logged in as the owner) a tab to Manage the party.
When a user Joins the Party, they are put onto a Pending User list and have to be approved by the party owner before they appear on the party scoreboard.
Once a user is accepted to the party, whenever they save their holy grail a copy will be saved to the Party database for retrieval later.
There is now a party button (indicated by a 1-2-3 ranking icon) in addition to the home button on the bottom left side of the screen to navigate between the sections of the website.
This update also includes an ItemScore value, which is a different way to compare grails rather than just items remaining. The ItemScore for any item is relative to its rarity so a rare item (i.e. Tyrael's Might) will have a high ItemScore and a common item will have a very low ItemScore. This is a work in progress, but I wanted to float the idea out there to see if it has any merit. 😄
Thanks @Nasicus for the "Party" naming idea. 😃

-Renamed 'Leaderboard' to 'Party' to keep more in theme with Diablo II gameplay 😄 -Fixed some additional issues from merging new packages -Resolved errors due to not properly sanitizing database inputs -Prettified code 😉

Nasicus · 2019-07-05T05:37:02Z

Thanks so much for this PR :) Really nice contribution!!!

Some initial things which came to my mind:

Add an own color-theme for the party pages (withRoot.tsx)
Add an own title for the party pages (App.tsx)
I would rename the join button to Party please! (for a strong diablo II reference :D, maybe even the join-route to pp ?? )
I would rename the table-view route & title to laederboard or overview (the url looks a little strange else)
Accepting users does not seem to work, no request to the server is sent
On the root page (/party) please add a little explanation, either above or below the components to write what this is is about
Add an info icon to the "Score" column and on hover write roughly down what this score means and how it's calculated
Missing "cursor"-mouse (pointer) icons on the table page for the refresh button and clicking on the table headers
Add a new version & changelog entry: client\package.json (for this you can make it 2.0.0) and Changelog.tsx or something like that

=> btw if you disagree with some stuff, just tell me, I'm always open for discussions!

One question: How did you prettify the code, i.e. did it prettify everything? (just because it seems the corresponding commit, has not a lot of changes :D )

As already discussed it will need some time until I can review this I guess :D

Thanks again 👍

mpherman · 2019-07-05T12:55:30Z

Add an own scheme for the party pages (withRoot.tsx)

Add an own title forhte party pages (App.tsx)

Would it make sense to just add an entry into (GrailMode.ts) which is Party = "party" and passdown onGrailModeChange to the party pages?
Otherwise, adding entries to the IAppState and IPassDownAppProps is the way to go?
Or should I make a separate appstate and props for just party pages?
What do you think? 😄

Accepting users does not seem to work, no request to the server is sent

Ah, well you currently need to hit the Save button (bottom right corner) before the current changes (accepted, denied, removed users) are sent to the server, maybe that was not done? I guess it doesn't make sense to buffer changes and then do them all at once anyway. Each time the 'accept', 'deny' or 'remove' buttons are clicked it can send the request straight away to avoid confusion. My only thought there was what if someone made a mistake, but perhaps thats not a big deal?

And just wondering:
How did you format the code? I would really prefer if you could reformat all your files with prettier.
I would do it something like this:

Switch to my master branch

Squash-merge your branch into it

Create a new commit on a new branch

Force-push this new branch to your master branch (or something like this)

=> because prettier will reformat on each commit

As already discussed it will need some time until I can review this I guess :D

I guess I don't really understand whats wrong here, I tried to run prettier on CLI in my root directory. Can you give an example of how this code is formatted poorly? I'm just not sure what it should look like 😄
For example here's how I ran it:

I'm pretty new to gihub/git so I will definitely try to do this

As for the other changes, they all seem straightforward so I'll get to them!

- Renamed Join button to 'Party please! - Added a short explanation to party home page to let users know what this does. Room to expand this further. - Added info icon to ItemScore heading in data table with short explanation of what it does - Added `cursor: pointer` to refresh icon and data table headers to indicate they are clickable - Changed data-table back ot being called 'leaderboard' to avoid confusion - Changed App version to 2.0.0 - Added ChangeLog info for 2.0.0 update

- Added some more info about ItemScores to the party main page - Maybe a good idea to just list all the scores? Decided to just list a few examples for now. - And started using Hooks 😄

Nasicus · 2019-07-05T21:31:42Z

Hmm let the theme and the title be for now. I will think of something, it's ugly as hell anyway atm :D

Great that you could already do all changes.

I have some more suggestions:

As you said yourself I would get rid of the "Save" button and do the calls directly on clicking approve / deny
I don't see a reason why you should have to enter your password when you join: It does never hurt you if anyone other adds you to an another party and the owner can always decline grails he doesn't want anyway... what do you think?

I hope I can do the review this weekend.. :)

Improve setting the theme & the title for the app

- Removed some unused routes and api functions - Removed password requirement for joining a party - When join is successful, change Party Please button text to => "Partied!" - When user begins typing into join address field then reset Join button image/text - Consolidate returning of party info from database controller - Removed "Save" button and made each user action in UserManagerRenderer resolve immediately. This way we can more easily handle errors for each user action. - Removed hasLocalChanges observer for parties as we no longer need to watch for these changes (due to removing Save button)

mpherman · 2019-07-06T17:15:29Z

Done:

Removed "Save" button and now does the calls directly on clicking approve / deny
Removed password requirement

Also:

Made the Party Please! button a little more interesting. Will change to Partied! on successful join and reset to Party Please! when user starts changing the address field after success (for if they want to sign multiple users up)

Todo:

Integrate new Theme and Title setting methods

- Added party theme => blue! - Added support for new theming functionality - Added a fix for switching areas through HomeButtom/PartyButton => to change theme correctly

mpherman · 2019-07-06T19:12:11Z

Integrate new Theme and Title setting methods

Ok this should work now!

Nasicus

All in all really nice 👍

The one thing you really should do is the server side changes (imho) , we can also discuss them if you think the solution is not good or if you want it to do differntly.

Whenever I wrote issue I meant to just create a github issue (task) so we have a clear todo list for stuff which we can do AFTER the initial release of your feature!

client/src/areas/grail/changeManagement/ChangeLogs.ts

client/src/areas/home/Home.tsx

client/src/areas/party/JoinFormRenderer.tsx

client/src/areas/party/PartyErrorHandler.tsx

server/src/controllers/PartyController.ts

server/src/initializers/AppInitializer.ts

server/src/initializers/DbInitializer.ts

client/src/areas/party/home/PartyHome.tsx

Nasicus · 2019-07-06T19:40:31Z

Concerning the setting of the theme an title:
The button click thing is strange, this should be done differently.
However I will probably look at this tomorrow myself.

Look into the theme setting stuff

- Changed the way party data is stored in database => move to grail entries and only keep a list of users in party db - Consolidated logic of user accept/deny/remove functions in PartyController - Pulled statistics data calculations out of GrailController into its own file - Really removed all party settings files - Fixed awkward sentences in party section and item score explanations - Tweaked the ItemScore values (Tyraels is now a nice round 1000 points) - Changed some text references from 'users' to 'members', sounds better - Cleaned up some poorly styled code (removed var use and non-camelcase)

server/src/controllers/PartyController.ts

server/src/controllers/StatisticsController.ts

server/src/controllers/PartyController.ts

Nasicus · 2019-07-07T18:21:32Z

👍👍👍 for the server changes! Exactly how I imagined it ;)

This also fixes the bug that the party theme was not set, if you refreshed `/party/` (the login screen)

- Projects only partyData and address from grail documents when building return value for party get - Renamed statisticscontroller to more accurately describe its use - Removed update count from party database

mpherman · 2019-07-08T12:41:55Z

Those last three issues should be fixed now 👍

Nasicus · 2019-07-08T13:17:45Z

Are you done with all comments (except issues)? Then I will have a quick look again this eve and probably test a little, so we're coming closer to a release :)

mpherman · 2019-07-08T13:30:32Z

Yes, only comments left will be opened as Issues after a release.

Thanks for reviewing this so quickly! 😄

Nasicus

.

client/src/areas/party/JoinFormRenderer.tsx

client/src/areas/party/UserManagerRenderer.tsx

fixes Nasicus#32

narfbg · 2019-07-18T12:05:30Z

Hey, sorry I'm late to the party (wink), but I have some thoughts on something:

I don't see a reason why you should have to enter your password when you join: It does never hurt you if anyone other adds you to an another party and the owner can always decline grails he doesn't want anyway... what do you think?

Having to enter a password is silly indeed - good call. However, while it may not technically hurt anybody, it'd be nice if the owner doesn't make decisions for you and you're able to leave a party.
I've been added to some GitHub orgs in the past, by complete strangers, and it doesn't feel nice ... Sometimes you just want to do your thing alone. :)

Nasicus · 2019-07-18T14:45:09Z

Well if someone knows your grail address he can go looking at your stuff all the time anyway.
I don't see a strong reason to change this for now, especially since it's now released like that.
If others complain too we can add a setting to grails or something like that...

narfbg · 2019-07-18T15:39:43Z

Looking is one thing. Being forced into a group is not the same.

mpherman · 2019-07-18T17:18:04Z

Well, that's mostly the reason I initially had the password requirement. To stop people from being signed up to parties they don't want to be in. For now, with no requirements to sign people up to the party, whats stopping a party leader from re-adding someone after they leave? I suppose we would have to add some type of self-blacklist to stop you being signed up to a party after you leave.

With a small community, I don't see it getting abused very much. You can always ask the party leader to remove you. Agree with @Nasicus currently, if it becomes a problem that people are abusing we can look into it.

narfbg · 2019-07-18T18:18:01Z

Maybe just make it so you can only join after requesting to do so and party admins can approve (or have a free for all policy) and maybe send invites, but not just add people on a whim.

Nasicus · 2019-07-18T19:24:06Z

At the moment there are 6 parties in the db and only 4 of theme have member...of these 4, 3 at least are tests... so there may be only one party - and that's the one from @mpherman ;)

You see I won't invest time to add this feature if it's not really required...if someone else wants to take a shot: feel free.

However if we do something we should not do something where you have to enter your PW again in my opinion.

mpherman added 4 commits July 4, 2019 11:22

Added base functionality for user leaderboards

4043505

Merge remote-tracking branch 'upstream/master'

3b7a7f4

Fixed issues with merge and new package versions

3901752

Even more fixes

e7da340

-Renamed 'Leaderboard' to 'Party' to keep more in theme with Diablo II gameplay 😄 -Fixed some additional issues from merging new packages -Resolved errors due to not properly sanitizing database inputs -Prettified code 😉

Nasicus added party enhancement New feature or request labels Jul 5, 2019

Nasicus self-assigned this Jul 5, 2019

mpherman added 2 commits July 5, 2019 10:24

Added more info about ItemScores

3b9524d

- Added some more info about ItemScores to the party main page - Maybe a good idea to just list all the scores? Decided to just list a few examples for now. - And started using Hooks 😄

Nasicus and others added 3 commits July 6, 2019 00:13

Improve setting the theme & the title for the app

b9f5ceb

Merge pull request #1 from Nasicus/feat/improve-theme-set-stuff

3107fe2

Improve setting the theme & the title for the app

Added support for new Theme switching

6f4e21b

- Added party theme => blue! - Added support for new theming functionality - Added a fix for switching areas through HomeButtom/PartyButton => to change theme correctly

Nasicus requested changes Jul 6, 2019

View reviewed changes

Nasicus reviewed Jul 6, 2019

View reviewed changes

client/src/areas/party/home/PartyHome.tsx Outdated Show resolved Hide resolved

Nasicus reviewed Jul 7, 2019

View reviewed changes

server/src/controllers/PartyController.ts Outdated Show resolved Hide resolved

Nasicus reviewed Jul 7, 2019

View reviewed changes

server/src/controllers/StatisticsController.ts Outdated Show resolved Hide resolved

Nasicus reviewed Jul 7, 2019

View reviewed changes

server/src/controllers/PartyController.ts Outdated Show resolved Hide resolved

Nasicus and others added 2 commits July 7, 2019 21:12

Improve how the app title and the theme is set (#2)

ccbeead

This also fixes the bug that the party theme was not set, if you refreshed `/party/` (the login screen)

Projecting grail data

100ecb8

- Projects only partyData and address from grail documents when building return value for party get - Renamed statisticscontroller to more accurately describe its use - Removed update count from party database

Nasicus added 3 commits July 8, 2019 20:48

some minor refactoring in PartyController.ts & minor fixes

72338e5

add to lower everywhere

e0e3735

Add note to the leaderboard what the numbers in there represent

cea08dc

Nasicus reviewed Jul 8, 2019

View reviewed changes

client/src/areas/party/JoinFormRenderer.tsx Show resolved Hide resolved

Nasicus reviewed Jul 8, 2019

View reviewed changes

client/src/areas/party/UserManagerRenderer.tsx Outdated Show resolved Hide resolved

mpherman and others added 4 commits July 8, 2019 15:48

Adding ItemScore entries for old grail seed data

e4c2690

Added short explanation after applying to join a party

97b24a3

Fix that 2 reload are required to see new pending users on your party

358f940

fixes Nasicus#32

add note about saving the grail one time

c213a40

Nasicus merged commit 65e3c5a into Nasicus:master Jul 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the ability to Party Up! #24

Adding the ability to Party Up! #24

mpherman commented Jul 5, 2019 •

edited

Loading

Nasicus commented Jul 5, 2019 •

edited

Loading

mpherman commented Jul 5, 2019 •

edited

Loading

Nasicus commented Jul 5, 2019 •

edited

Loading

mpherman commented Jul 6, 2019 •

edited by Nasicus

Loading

mpherman commented Jul 6, 2019

Nasicus left a comment

Nasicus commented Jul 6, 2019 •

edited

Loading

Nasicus commented Jul 7, 2019

mpherman commented Jul 8, 2019

Nasicus commented Jul 8, 2019

mpherman commented Jul 8, 2019

Nasicus left a comment •

edited

Loading

narfbg commented Jul 18, 2019

Nasicus commented Jul 18, 2019

narfbg commented Jul 18, 2019

mpherman commented Jul 18, 2019

narfbg commented Jul 18, 2019

Nasicus commented Jul 18, 2019

Adding the ability to Party Up! #24

Adding the ability to Party Up! #24

Conversation

mpherman commented Jul 5, 2019 • edited Loading

Nasicus commented Jul 5, 2019 • edited Loading

mpherman commented Jul 5, 2019 • edited Loading

Nasicus commented Jul 5, 2019 • edited Loading

mpherman commented Jul 6, 2019 • edited by Nasicus Loading

mpherman commented Jul 6, 2019

Nasicus left a comment

Choose a reason for hiding this comment

Nasicus commented Jul 6, 2019 • edited Loading

Nasicus commented Jul 7, 2019

mpherman commented Jul 8, 2019

Nasicus commented Jul 8, 2019

mpherman commented Jul 8, 2019

Nasicus left a comment • edited Loading

Choose a reason for hiding this comment

narfbg commented Jul 18, 2019

Nasicus commented Jul 18, 2019

narfbg commented Jul 18, 2019

mpherman commented Jul 18, 2019

narfbg commented Jul 18, 2019

Nasicus commented Jul 18, 2019

mpherman commented Jul 5, 2019 •

edited

Loading

Nasicus commented Jul 5, 2019 •

edited

Loading

mpherman commented Jul 5, 2019 •

edited

Loading

Nasicus commented Jul 5, 2019 •

edited

Loading

mpherman commented Jul 6, 2019 •

edited by Nasicus

Loading

Nasicus commented Jul 6, 2019 •

edited

Loading

Nasicus left a comment •

edited

Loading