Populate submitted values in site details #1043

K-Markopoulos · 2024-10-23T14:40:10Z

Issue #1038

Status and custom spotter api token values are not displayed in site details.

Changes

Add status initial value
Change api to return the spotter api token masked (only 4 last digits) if it has custom token. Unfortunately we cannot add a new computed property in the Site entity, because the spotterApiToken is not selected and therefore accessible by default, so we select all site properties and whitelist the properties we want to return.
On UI, show the masked token. User can provide a new one by clicking on the input field - then the old token is erased and the user can start typing the new token. If they leave the input without typing anything, the old token is filled back in. If the user wants to completely remove the custom token they uncheck the "Add custom api token" checkbox.

Note the logged spotterApiToken, when null the token is deleted, when undefined it's not changed and when it has value, the value is persisted.

Screencast.from.23-10-2024.05.16.05.webm

pullrequest

✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!

What to expect from this code review:

Comments posted to any areas of potential concern or improvement.
Detailed feedback or actions needed to resolve issues that are found.
Turnaround times vary, but we aim to be swift.

@K-Markopoulos you can click here to see the review status or cancel the code review job.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 88
- 46

58% TSX
35% TypeScript
7% TypeScript (tests)

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

github-actions · 2024-10-23T14:43:33Z

Build succeeded and deployed at https://aqualink-app-1043.surge.sh
(hash ea8792d deployed at 2024-10-24T18:43:21)

pullrequest

Great work here. I didn't spot any issues with these changes. 🎉

Graham C

Reviewed with ❤️ by PullRequest

Caesarh97 · 2024-10-23T19:20:55Z

Hi @K-Markopoulos,
Thank you for working on this. When testing it out, I added a spotter (not owned by Aqualink) and added its API token. I updated the page and went back to the dashboard. You can see the screenshot below. The Status "deployed" works fine, but I can not see the "Add a custom API token" box being checked or any value in the token textbox.

Regarding the second point in your changes, I didn’t fully understand it. If it's possible to show the last values that were entered for the API token, that's great. If not, that's fine and no need to do too much work on it. This view is locked to the SuperAdmin and the Site Manager for the specific site, so there's no need to mask the token or anything fancy (unless it has already been done). The "EDIT SITE DETAILS" view is only for the site manager of the specific site and the SuperAdmins to keep track of and troubleshoot what data has been entered into the database.

pullrequest

PullRequest reviewed the updates made to #1043 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.

Reviewed by:

Graham C

K-Markopoulos · 2024-10-24T07:32:43Z

Hi @Caesarh97,
If I'm not wrong, the generated preview link (https://aqualink-app-1043.surge.sh/) includes only the frontend changes of the PR. The backend changes have to be manually deployed to be available/testable and the spotter api token fix has some backend changes. @ericboucher can you help us with the backend deployment in order to test this PR?

The 2nd point is describing some technical things about the implementation. Basically, with the current implementation, once the token is persisted, it won't be fully visible in the UI. The user will be able to see only the last 4 characters of it. If they set "123456789", they will see "****6789".

Caesarh97 · 2024-10-24T18:34:22Z

Hi @K-Markopoulos,
Got it now!

I tried it out, and it looks great! Thank you for all of your help.

pullrequest

✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!

What to expect from this code review:

Comments posted to any areas of potential concern or improvement.
Detailed feedback or actions needed to resolve issues that are found.
Turnaround times vary, but we aim to be swift.

@K-Markopoulos you can click here to see the review status or cancel the code review job.

pullrequest

PullRequest reviewed the updates made to #1043 up until the latest commit (ea8792d). No further issues were found.

Reviewed by:

Graham C

ericboucher · 2024-10-24T22:03:49Z

packages/api/src/sites/sites.entity.ts

@@ -151,10 +151,12 @@ export class Site {
  @Column({ nullable: true, select: false, type: 'character varying' })
  contactInformation?: string | null;

-  hasHobo: boolean;
+  hasHobo?: boolean;


Was this change needed?

Yes, since it was a required property in Site TS expected sites.service.ts::findeOne to return this property as well. The spread operator ...site tricked TS into thinking that hasHobo is included in the response, even though it was actually not. After removing the spread operator and explicitly defining the properties, TS throws an error that hasHobo is missing. I had to make this property optional to overcome this error and since it's not always defined, it makes sense to be optional.

Ok, the alternative would be to actually have it defined no? Since it can only be true or false?

Yes, we can calculate the value and have it defined. I didn't add it in the first place, because I didn't want to change the structure of the response (the initial response didn't have hasHobo property) and it seems like it requires another query to calculate it.

aqualink-app/packages/api/src/sites/sites.service.ts

Line 199 in 2f0e598

const hasHoboDataSet = await hasHoboDataSubQuery(this.sourceRepository);

pullrequest

PullRequest reviewed the updates made to #1043 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.

Reviewed by:

Graham C

Populate submitted values in site details

6f1886a

K-Markopoulos requested a review from ericboucher October 23, 2024 14:40

pullrequest bot reviewed Oct 23, 2024

View reviewed changes

Merge branch 'master' into populate-site-submitted-details

ea8792d

Caesarh97 closed this Oct 24, 2024

Caesarh97 reopened this Oct 24, 2024

pullrequest bot reviewed Oct 24, 2024

View reviewed changes

ericboucher approved these changes Oct 24, 2024

View reviewed changes

ericboucher reviewed Oct 24, 2024

View reviewed changes

ericboucher approved these changes Oct 24, 2024

View reviewed changes

pullrequest bot reviewed Oct 25, 2024

View reviewed changes

ericboucher merged commit 3e2be32 into master Oct 30, 2024
6 checks passed

ericboucher deleted the populate-site-submitted-details branch October 30, 2024 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Populate submitted values in site details #1043

Populate submitted values in site details #1043

K-Markopoulos commented Oct 23, 2024

pullrequest bot left a comment

pullrequest bot left a comment

github-actions bot commented Oct 23, 2024 •

edited

Loading

pullrequest bot left a comment

Caesarh97 commented Oct 23, 2024

pullrequest bot left a comment

K-Markopoulos commented Oct 24, 2024

Caesarh97 commented Oct 24, 2024

pullrequest bot left a comment

pullrequest bot left a comment

ericboucher Oct 24, 2024

K-Markopoulos Oct 25, 2024

ericboucher Oct 25, 2024

K-Markopoulos Oct 25, 2024 •

edited

Loading

pullrequest bot left a comment

Populate submitted values in site details #1043

Populate submitted values in site details #1043

Conversation

K-Markopoulos commented Oct 23, 2024

Issue #1038

Changes

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

PullRequest Breakdown

github-actions bot commented Oct 23, 2024 • edited Loading

pullrequest bot left a comment

Choose a reason for hiding this comment

Caesarh97 commented Oct 23, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

K-Markopoulos commented Oct 24, 2024

Caesarh97 commented Oct 24, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

ericboucher Oct 24, 2024

Choose a reason for hiding this comment

K-Markopoulos Oct 25, 2024

Choose a reason for hiding this comment

ericboucher Oct 25, 2024

Choose a reason for hiding this comment

K-Markopoulos Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 23, 2024 •

edited

Loading

K-Markopoulos Oct 25, 2024 •

edited

Loading