Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: circle interest for next opening #152

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

radl97
Copy link

@radl97 radl97 commented Jun 7, 2022

Adds a UI element tied back to the CircleEntity. Anyone can increment this counter. (any number of times)

The basic idea is to show interest in a possible opening for motivating the provider.

Note: Probably this needs migrations or other things I do not know about (: . Also I could not test the code. I am happy to help, and discuss ideas :)

Adds a UI element tied back to the CircleEntity.
Anyone can increment this counter. (any number of times)

The basic idea is to show interest in a possible opening for
motivating the provider.
@radl97 radl97 changed the title Add Interest feature Feature: circle interest for next opening Jun 7, 2022
Copy link
Member

@Gerviba Gerviba left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
I just reviewed your code, if you need help, contact me here or in pm.

@@ -79,6 +79,9 @@ data class CircleEntity(
@Column
var virGroupId: Long? = null

@Column
Copy link
Member

Choose a reason for hiding this comment

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

It requires a column definistion with '... default 0' parameter in order to make DB migration automatic

Copy link
Author

Choose a reason for hiding this comment

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

I've found this: https://stackoverflow.com/a/42150940 as an alternative to this: https://stackoverflow.com/a/375202
I think you were mentioning the latter.

However, can you check whether the former is valid? It seems a better solution, as you already use org.hibernate.

Copy link
Author

Choose a reason for hiding this comment

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

I've implemented the one using org.hibernate's ColumnDefault for now

@@ -336,6 +336,22 @@ open class ApiController(

private val trashpandaVoters: ConcurrentHashMap<String, Int> = ConcurrentHashMap<String, Int>()

@PostMapping("/provider/{circle}/increaseInterest")
@ResponseBody
fun circleInterestPlusPlus(@PathVariable circle: String, model: Model, request: HttpServletRequest): String {
Copy link
Member

Choose a reason for hiding this comment

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

This should be in a transaction

Copy link
Author

Choose a reason for hiding this comment

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

I'll pull the transactional part to service layer, as it is recommended (as I see it, and as it was pointed out here: https://stackoverflow.com/a/18637119 ).

Copy link
Author

Choose a reason for hiding this comment

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

(Done)

@@ -116,6 +122,23 @@ <h4 th:text="*{name}">Full Name</h4>
<span class="list-end" id="no-results"></span>

<div th:replace="MainLayout :: footer"></div>
<script>
function increaseInterest() {
return fetch("/api/provider/(Burgir)/increaseInterest", { // TODO hook in provider?
Copy link
Member

Choose a reason for hiding this comment

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

I might use sg. like:

function increaseInterest(provider) {
    return fetch(`/api/provider/${provider}/increaseInterest`, {

Copy link
Author

Choose a reason for hiding this comment

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

This gets really ugly (alias is injected as JavaScript from a template, and I do not like the sound of it :/ ). Or would it be okay? Or you had something else in mind?

For reference, this is how I understood your comment:

diff --git a/src/main/resources/templates/circleProfile.html b/src/main/resources/templates/circleProfile.html
index 7bc09ae..af6b985 100644
--- a/src/main/resources/templates/circleProfile.html
+++ b/src/main/resources/templates/circleProfile.html
@@ -58,7 +58,7 @@
                         <tr>
                             <td th:text="#{lang.profile.open}">Please open!</td>
                             <td>
-                                <button id="interest-counter" class="action-button" onclick="increaseInterest(1); return false" th:text="${selectedCircle?.interestCounter}" class="action-button">0</button>
+                                <button id="interest-counter" class="action-button" th:onclick="increaseInterest(${selectedCircle.alias}); return false" th:text="${selectedCircle?.interestCounter}" class="action-button">0</button>
                             </td>
                         </tr>
                         <tr>
@@ -123,8 +123,8 @@

     <div th:replace="MainLayout :: footer"></div>
     <script>
-        function increaseInterest() {
-            return fetch("/api/provider/(Burgir)/increaseInterest", { // TODO hook in provider?
+        function increaseInterest(provider) {
+            return fetch(`/api/provider/${provider}/increaseInterest`, {
                 method: 'POST',
                 cache: 'no-cache',
                 credentials: 'same-origin',

Copy link
Member

Choose a reason for hiding this comment

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

In a normal scenario you could write sg. like:

th:onclick="|increaseInterest(${selectedCircle.alias}); return false|"

Notice the string template literal vertical bars.

But in case of th:onclick you are not able to use variables, so you have two options:

<script th:inline="javascript">
const provider = /*[[${selectedCircle.alias}]]*/ '';
</script>

Copy link
Author

Choose a reason for hiding this comment

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

I forgot to reflect the change in the comment.

Copy link
Author

Choose a reason for hiding this comment

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

I have used data attributes in the changes, can you check if it works?

Copy link
Author

Choose a reason for hiding this comment

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

for you :D

@Gerviba Gerviba marked this pull request as draft June 8, 2022 07:33
@Gerviba Gerviba marked this pull request as draft June 8, 2022 07:33
Moves the logic inside service layer.
@sonarcloud
Copy link

sonarcloud bot commented Jun 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@radl97 radl97 requested a review from Gerviba June 8, 2022 09:15
@radl97
Copy link
Author

radl97 commented Jun 8, 2022

Thank you for looking into this PR!
I think I've addressed all your comments.

Feel free to add nitpicks. I am not really confident with my Spring :D

@radl97 radl97 marked this pull request as ready for review June 11, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants