AG - added column called username to leaderboard that maps user id to the correct username #37

archit-gpt · 2022-11-28T03:27:03Z

Overview

when a user joins a commons, the UserCommon's repository now saves a username parameter that is the current user's google full name (this can be easily changed to email, firstname, or last name).
a username column was added to the leaderboard page that displays all users in common with their username from (1).

Changes

Backend

UserCommons.java: added username to UserCommons
CommonsController.java: username gets initialized as current user's name, UserCommons gets built when a player joins
UserCommonsControllerTests.java: updated all tests
CommonsControllerTests.java: updated all tests

Frontend

LeaderboardTable.js : added the username column to the table
LeaderboardTable.test.js: updated all tests
leaderboardFixtures.js: updated fixtures

…correct username

RAILGUN1124

i approve

nathanmathew02

Merge Conflict

tkumar07

LGTM

nathanmathew02

LGTM

ruiqiRichard

LGTM

RAILGUN1124

lgtm

pconrad

This PR has many issues that need to be addressed before it is mergeable.

It does work... I tested it on QA. But it may very well undo several other changes, and therefore break things that were already fixed.

And it includes huge formatting changes that make it very difficult to review. Those would be considered quite a nuisance in a professional shop. Formatting updates are welcome, but should be isolated in their own separate PRs.

pconrad · 2022-11-29T00:04:34Z

frontend/src/main/components/OurTable.js

+// var tableStyle = {
+//   "background": "white"
+// };


Why are we commenting out these three lines?

This is undoing someone else's work. Please don't do that.

pconrad · 2022-11-29T00:04:52Z

frontend/src/main/components/OurTable.js

@@ -26,7 +26,7 @@ export default function OurTable({ columns, data, testid = "testid", ...rest })
  }, useSortBy)

  return (
-    <Table style={tableStyle} {...getTableProps()} striped bordered hover >
+    <Table {...getTableProps()} striped bordered hover >


Why are we undoing someone else's work here? Please don't!

pconrad · 2022-11-29T00:05:28Z

frontend/src/main/pages/PlayPage.js

-	// Thats why only need to compare the current totalWealth to the cowPrice, because this implies that 
-	// totalWealth - cowPrice aka current totalWealth will not be enough to buy a cow.
-	if(!(userCommons.totalWealth < commons.cowPrice)) toast(`Cow bought!`);
-	else toast(`You can't buy a cow because you don't have enough money`)
+    toast(`Cow bought!`);


Again.. undoing other people's work is not acceptable.

I think you might need to merge in the latest changes from main.

pconrad · 2022-11-29T00:07:34Z

src/main/java/edu/ucsb/cs156/happiercows/entities/UserCommons.java


-  @Column(name="user_id")
-  private long userId;  
+  private String username;


This is not how this should be done. Instead, you should look up the username from the user's table. This is storing redundant data, which violates the principles of good database design.

pconrad · 2022-11-29T00:12:37Z

src/test/java/edu/ucsb/cs156/happiercows/controllers/CommonsControllerTests.java

 @AutoConfigureDataJpa
 public class CommonsControllerTests extends ControllerTestCase {

-  @MockBean
-  UserCommonsRepository userCommonsRepository;
-
-  @MockBean
-  UserRepository userRepository;
-
-  @MockBean
-  CommonsRepository commonsRepository;
-
-  @Autowired
-  private ObjectMapper objectMapper;
-
-  @WithMockUser(roles = { "ADMIN" })
-  @Test
-  public void createCommonsTest() throws Exception {
-    LocalDateTime someTime = LocalDateTime.parse("2022-03-05T15:50:10");
-    LocalDateTime someOtherTime = LocalDateTime.parse("2022-04-20T15:50:10");
-
-    Commons commons = Commons.builder()
-        .name("Jackson's Commons")
-        .cowPrice(500.99)
-        .milkPrice(8.99)
-        .startingBalance(1020.10)
-        .startingDate(someTime)
-        .endingDate(someOtherTime)
-        .degradationRate(50.0)
-        .showLeaderboard(false)
-        .build();
-
-    CreateCommonsParams parameters = CreateCommonsParams.builder()
-        .name("Jackson's Commons")
-        .cowPrice(500.99)
-        .milkPrice(8.99)
-        .startingBalance(1020.10)
-        .startingDate(someTime)
-        .endingDate(someOtherTime)
-        .degradationRate(50.0)
-        .showLeaderboard(false)
-        .build();
-
-    String requestBody = objectMapper.writeValueAsString(parameters);
-    String expectedResponse = objectMapper.writeValueAsString(commons);
-
-    when(commonsRepository.save(commons))
-        .thenReturn(commons);
-
-    MvcResult response = mockMvc
-        .perform(post("/api/commons/new").with(csrf())
-            .contentType(MediaType.APPLICATION_JSON)
-            .characterEncoding("utf-8")
-            .content(requestBody))
-        .andExpect(status().isOk())
-        .andReturn();
-
-    verify(commonsRepository, times(1)).save(commons);
-
-    String actualResponse = response.getResponse().getContentAsString();
-    assertEquals(expectedResponse, actualResponse);
-  }
+    @MockBean
+    UserCommonsRepository userCommonsRepository;
+
+    @MockBean


If this is only whitespace/formatting changes, PLEASE DON'T DO THIS.

It makes your PR super hard to code review, because we have to look at every blessed line to make sure you didn't delete or add something.

Fixing formatting should be done in a separate PR that ONLY focuses on fixing formatting.

pconrad · 2022-11-29T00:13:13Z

src/test/java/edu/ucsb/cs156/happiercows/controllers/UserCommonsControllerTests.java

@@ -44,40 +44,42 @@
 @AutoConfigureDataJpa


If this is only whitespace/formatting changes, PLEASE DON'T DO THIS.

It makes your PR super hard to code review, because we have to look at every blessed line to make sure you didn't delete or add something.

Fixing formatting should be done in a separate PR that ONLY focuses on fixing formatting.

ruiqiRichard · 2022-11-30T12:11:16Z

issues get fixed in #65

added column called username to leaderboard that maps user id to the …

0bc4b81

…correct username

archit-gpt self-assigned this Nov 28, 2022

archit-gpt requested review from pconrad, minhbui27, nathanmathew02, phtcon, RAILGUN1124, ruiqiRichard, tkumar07 and vivianross06 and removed request for pconrad November 28, 2022 03:27

archit-gpt changed the title ~~added column called username to leaderboard that maps user id to the correct username~~ AG - added column called username to leaderboard that maps user id to the correct username Nov 28, 2022

pconrad added FIXME: Peer CR Please get a peer code review FIXME: Add Team Label Please add the label for your team, e.g. f22-4pm-3 labels Nov 28, 2022

archit-gpt added f22-7pm-4 f22-7pm-4 and removed FIXME: Add Team Label Please add the label for your team, e.g. f22-4pm-3 labels Nov 28, 2022

RAILGUN1124 previously approved these changes Nov 28, 2022

View reviewed changes

pconrad added STAFF: ready for review green on CI, has peer CR, no other obvious issues and removed FIXME: Peer CR Please get a peer code review labels Nov 28, 2022

nathanmathew02 requested changes Nov 28, 2022

View reviewed changes

tkumar07 previously approved these changes Nov 28, 2022

View reviewed changes

resolved merge conflict

b3081cf

archit-gpt dismissed stale reviews from tkumar07 and RAILGUN1124 via b3081cf November 28, 2022 22:23

commented out unused lines

9a68f52

nathanmathew02 self-requested a review November 28, 2022 23:09

nathanmathew02 approved these changes Nov 28, 2022

View reviewed changes

ruiqiRichard approved these changes Nov 28, 2022

View reviewed changes

RAILGUN1124 self-requested a review November 28, 2022 23:40

RAILGUN1124 approved these changes Nov 28, 2022

View reviewed changes

pconrad temporarily deployed to f22-7pm-4-happycows November 28, 2022 23:55 Inactive

pconrad requested changes Nov 29, 2022

View reviewed changes

pconrad added FIXME: Changes Requested Please address the changes specified in the code review and removed STAFF: ready for review green on CI, has peer CR, no other obvious issues labels Nov 29, 2022

This was referenced Nov 29, 2022

Archit_RY-username col leaderboard 2 fix #51

Closed

Archit_RY-username col leaderboard fix #52

Closed

pc/ry/ag - add userCommons plus endpoint to make username available on leaderboard #65

Merged

ruiqiRichard closed this Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AG - added column called username to leaderboard that maps user id to the correct username #37

AG - added column called username to leaderboard that maps user id to the correct username #37

archit-gpt commented Nov 28, 2022

RAILGUN1124 left a comment

nathanmathew02 left a comment

tkumar07 left a comment

nathanmathew02 left a comment

ruiqiRichard left a comment

RAILGUN1124 left a comment

pconrad left a comment

pconrad Nov 29, 2022

pconrad Nov 29, 2022

pconrad Nov 29, 2022

pconrad Nov 29, 2022

pconrad Nov 29, 2022

pconrad Nov 29, 2022

ruiqiRichard commented Nov 30, 2022

AG - added column called username to leaderboard that maps user id to the correct username #37

AG - added column called username to leaderboard that maps user id to the correct username #37

Conversation

archit-gpt commented Nov 28, 2022

Overview

Changes

Backend

Frontend

RAILGUN1124 left a comment

Choose a reason for hiding this comment

nathanmathew02 left a comment

Choose a reason for hiding this comment

tkumar07 left a comment

Choose a reason for hiding this comment

nathanmathew02 left a comment

Choose a reason for hiding this comment

ruiqiRichard left a comment

Choose a reason for hiding this comment

RAILGUN1124 left a comment

Choose a reason for hiding this comment

pconrad left a comment

Choose a reason for hiding this comment

pconrad Nov 29, 2022

Choose a reason for hiding this comment

pconrad Nov 29, 2022

Choose a reason for hiding this comment

pconrad Nov 29, 2022

Choose a reason for hiding this comment

pconrad Nov 29, 2022

Choose a reason for hiding this comment

pconrad Nov 29, 2022

Choose a reason for hiding this comment

pconrad Nov 29, 2022

Choose a reason for hiding this comment

ruiqiRichard commented Nov 30, 2022