SimplQ / simplQ-backend

SimplQ backend, written in Java for AWS
https://simplq.me
GNU General Public License v3.0
17 stars 27 forks source link

EngDebt: Proper module name, package names etc #25

Closed daltonfury42 closed 4 years ago

daltonfury42 commented 4 years ago

Our artefact id, group id, package names, module name etc are from a sample project.

  <groupId>com.example</groupId>
  <artifactId>rest-service</artifactId>

We should refactor to me.simplq and also rename some folders

daltonfury42 commented 4 years ago

also we don't use final

dhirenmathur commented 4 years ago

Does the label mean you're not working on this anymore? Where do you want to use final?

daltonfury42 commented 4 years ago

I've added final in most places I have spotted I think. Just the package name and folder names remain.

We have a very simple backend, most of the active development is going in the frontend side. https://github.com/SimplQ/simplQ-frontend

If you are interested in taking a good item on the backend side, can you write a good integration test?

@ExtendWith(SpringExtension.class)
@SpringBootTest
@AutoConfigureMockMvc
class RegisterUseCaseIntegrationTest {

  @Autowired
  private MockMvc mockMvc;

  @Autowired
  private ObjectMapper objectMapper;

  @Autowired
  private UserRepository userRepository;

  @Test
  void registrationWorksThroughAllLayers() throws Exception {
    UserResource user = new UserResource("Zaphod", "zaphod@galaxy.net");

    mockMvc.perform(post("/forums/{forumId}/register", 42L)
            .contentType("application/json")
            .param("sendWelcomeMail", "true")
            .content(objectMapper.writeValueAsString(user)))
            .andExpect(status().isOk());

    UserEntity userEntity = userRepository.findByName("Zaphod");
    assertThat(userEntity.getEmail()).isEqualTo("zaphod@galaxy.net");
  }

}

https://reflectoring.io/spring-boot-test/

We should write one such testcase, which creates a queue, and then create a token in the queue. The in-memeory DB should work. Even asserting 200 OK is enough for now.

This would be a great value addition. We could then write a simple Github action as a pre-merge check to catch most common errors pre-merge. What do you think?

dhirenmathur commented 4 years ago

I like it, I was going to suggest unit tests first, but yes an integration test would help getting the CI set up for GitHub. Let me open a separate issue for this and I'll begin work on it.

I'm interested in continuing on the backend side, as you said it's a simple backend but the usecase is good and I'm learning from looking at your code. :)

daltonfury42 commented 4 years ago

I want to keep the backend lean, no new features unless we will use it. While you are going through the code, if you think there are any improvements possible for existing code, please raise an issue, and we can discuss.

Good tests were the next thing I wanted to take up. We automatically deploy PRs on merging, so these gates are very much needed.

I wanted to do unit tests too, but currently I don't have the bandwith, so I parked it for later.

dhirenmathur commented 4 years ago

I've raised a PR for the low-hanging refactoring fruit of module name, artefact id etc. This might impact your deployment, so please take a closer look even though the changes might be simple. I haven't crosschecked with the travis CI scripts.

While you are going through the code, if you think there are any improvements possible for existing code, please raise an issue, and we can discuss.

Definitely! Will take a closer look. :)

daltonfury42 commented 4 years ago

I just merged and deployed. I had to make a small tweak to appspec.yml (https://github.com/SimplQ/simplQ-backend/commit/975d57b7c040a656fa1513b800eec335a624834c) but otherwise everything went smooth.

dhirenmathur commented 4 years ago

Ah, good eye, in the 52 files chaos I missed that 😅