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

[Feat][UI] Set up UI setting component in profile page #11226

Merged
merged 26 commits into from
Oct 8, 2022

Conversation

lynn-illumio
Copy link
Contributor

@lynn-illumio lynn-illumio commented Aug 1, 2022

Purpose of the pull request

To set up a UI setting component under the profile page as a start point. Pending adding option selector to each setting, such as timer interval option.
Pending data set up in state and POST request to the backend.
This PR related: #11079

Brief change log

  • Add a util function to define static user settings in use-setting.ts
  • Add this util function to profile/index.tsx to render an additional sub-page

Verify this pull request

This pull request is code cleanup without any test coverage.

  • No Unit testing or end-to-end testing is added
  • It's just a starter PR, and pending work on the data state update / API request part

Screenshot:

Screen Shot 2022-07-31 at 8 54 55 PM

Design

Need some drop-down menu user selection, eg., a drop-down menu with a timer option (1s, 2s, 10s, etc) that allow user to select the auto fresh interval.
ui-setting-design

@github-actions github-actions bot added the UI ui and front end related label Aug 1, 2022
@songjianet songjianet added this to the 3.1.0-alpha milestone Aug 1, 2022
@songjianet songjianet linked an issue Aug 1, 2022 that may be closed by this pull request
3 tasks
@songjianet songjianet changed the title Set up UI setting component in profile page [Feat][UI] Set up UI setting component in profile page Aug 1, 2022
@lynn-illumio
Copy link
Contributor Author

lynn-illumio commented Sep 23, 2022

Hi @songjianet @EricGao888 . New commits were added to address these features/improvements:

  1. Added UI setting page where user can configure auto refresh time; available in both English and Chinese;

Screen Shot 2022-09-22 at 11 03 53 PM

2) Improved the ```getLogs``` function. Previously, we made an async call to get logs too often to confuse users that the log loading took a long time. Now it's been improved;
3) Auto refresh the log when the log result is loading, the user can configure the time interval (10s, 20s, 1 min, etc) on the UI setting page.

Screen Shot 2022-09-22 at 10 33 34 PM

@songjianet songjianet modified the milestones: 3.1.0, 3.2.0 Sep 23, 2022
@songjianet
Copy link
Member

This function has not been perfected, and the 310 has been closed, so I changed this to 320.

@songjianet
Copy link
Member

image

devosend
devosend previously approved these changes Sep 27, 2022
Copy link
Contributor

@devosend devosend left a comment

Choose a reason for hiding this comment

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

LGTM

* limitations under the License.
*/

type logTimer = number
Copy link
Member

Choose a reason for hiding this comment

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

It seems to be no need to define the logTimer type here.

variables.skipLineNum = 0
variables.logLoadingRef = true
getLogs(row, logTimer)
}, logTimer * 1000)
Copy link
Member

Choose a reason for hiding this comment

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

It's better to clear the timer before turning it on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate on this comment?
I tried clearing the timer by clearTimeout(timeoutID) but it might not be what we want.

Copy link
Member

Choose a reason for hiding this comment

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

If the getLogs function is called multiple times, multiple timers will be running.

Copy link
Contributor Author

@lynn-illumio lynn-illumio Oct 3, 2022

Choose a reason for hiding this comment

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

That's a great point! Addressed in 7782c5d

variables.skipLineNum = 0
variables.logLoadingRef = true
getLogs(row, logTimer)
}, logTimer * 1000)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

nodeVariables.limit += 1000
nodeVariables.skipLineNum += 1000
getLogs(logTimer)
}, logTimer * 1000)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in fcfe28f.

const handleUpdateValue = (logTimer: logTimer) => {
const logTimerStore = useLogTimerStore()
logTimerStore.setLogTimer(logTimer)
cookies.set('logTimer', String(logTimer), { path: '/' })
Copy link
Member

Choose a reason for hiding this comment

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

Why set the logTimer into the cookies? Does the backend side need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi. We might not need this. I will remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might not need this. Removed in 145bfd4.

@@ -15,4 +15,4 @@

NODE_ENV=development

VITE_APP_DEV_WEB_URL='http://127.0.0.1:12345'
VITE_APP_DEV_WEB_URL='http://8.142.34.29:12345'
Copy link
Member

Choose a reason for hiding this comment

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

It's better to set to the default ip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 7782c5d.

@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2022

Codecov Report

Merging #11226 (b9040e7) into dev (25ded1e) will decrease coverage by 0.59%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev   #11226      +/-   ##
============================================
- Coverage     40.30%   39.71%   -0.60%     
+ Complexity     4854     4192     -662     
============================================
  Files           943     1016      +73     
  Lines         37055    38049     +994     
  Branches       4073     4363     +290     
============================================
+ Hits          14935    15110     +175     
- Misses        20612    21193     +581     
- Partials       1508     1746     +238     
Impacted Files Coverage Δ
...olphinscheduler/plugin/task/dvc/DvcParameters.java 0.00% <0.00%> (-100.00%) ⬇️
...heduler/plugin/task/jupyter/JupyterParameters.java 0.00% <0.00%> (-80.00%) ⬇️
...ler/server/worker/metrics/WorkerServerMetrics.java 0.00% <0.00%> (-72.92%) ⬇️
...e/dolphinscheduler/dao/entity/ProcessInstance.java 0.00% <0.00%> (-65.57%) ⬇️
...lphinscheduler/dao/entity/ProcessTaskRelation.java 0.00% <0.00%> (-62.50%) ⬇️
...ache/dolphinscheduler/dao/entity/K8sNamespace.java 18.18% <0.00%> (-57.23%) ⬇️
...scheduler/plugin/task/mlflow/MlflowParameters.java 18.18% <0.00%> (-56.05%) ⬇️
...a/org/apache/dolphinscheduler/dao/entity/User.java 18.18% <0.00%> (-55.96%) ⬇️
...apache/dolphinscheduler/dao/entity/DataSource.java 18.18% <0.00%> (-54.99%) ⬇️
...olphinscheduler/plugin/task/emr/EmrParameters.java 0.00% <0.00%> (-50.00%) ⬇️
... and 522 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sonarcloud
Copy link

sonarcloud bot commented Oct 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 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@devosend devosend left a comment

Choose a reason for hiding this comment

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

LGTM

@songjianet songjianet dismissed Amy0104’s stale review October 8, 2022 08:34

Let's deal with it like this for the time being, we won't be able to release 3.2.0 in the short term, and we will be optimizing it at that time.

Copy link
Member

@songjianet songjianet left a comment

Choose a reason for hiding this comment

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

LGTM

@songjianet songjianet merged commit 234f2e8 into apache:dev Oct 8, 2022
xdu-chenrj pushed a commit to xdu-chenrj/dolphinscheduler that referenced this pull request Oct 13, 2022
* Set up UI setting component in profile page

* Add UI setting page to the route, remove UI setting section from the profile

* Add log auto refresh timer store

* Add logTimer to pinia store and cookies

* Read logTimer from pinia store, and delay getLogs by passing the logTimer to setInterval in refreshLogs function

* Add log timer to getLogs

* Fine tune view log, add auto refresh based on time interval set in UI setting

* Add useI18n to ui setting

* Set up UI setting component in profile page

* Add UI setting page to the route, remove UI setting section from the profile

* Add log auto refresh timer store

* Add logTimer to pinia store and cookies

* Read logTimer from pinia store, and delay getLogs by passing the logTimer to setInterval in refreshLogs function

* Add log timer to getLogs

* Fine tune view log, add auto refresh based on time interval set in UI setting

* Add useI18n to ui setting

* [Feat][UI] Add UI setting page in the project.

* [Feat][UI] Add UI setting page in the project.

* Remove logTimer in cookies

* Clear timer id, set VITE_APP_DEV_WEB_URL back to default

* Clear time id in dag

* [Feat][UI] Add license header.

* [Feat][UI] Remove console.

* [Fix][UI] Fix log timer types.

Co-authored-by: songjianet <[email protected]>
fuchanghai pushed a commit to fuchanghai/dolphinscheduler that referenced this pull request Nov 16, 2022
* Set up UI setting component in profile page

* Add UI setting page to the route, remove UI setting section from the profile

* Add log auto refresh timer store

* Add logTimer to pinia store and cookies

* Read logTimer from pinia store, and delay getLogs by passing the logTimer to setInterval in refreshLogs function

* Add log timer to getLogs

* Fine tune view log, add auto refresh based on time interval set in UI setting

* Add useI18n to ui setting

* Set up UI setting component in profile page

* Add UI setting page to the route, remove UI setting section from the profile

* Add log auto refresh timer store

* Add logTimer to pinia store and cookies

* Read logTimer from pinia store, and delay getLogs by passing the logTimer to setInterval in refreshLogs function

* Add log timer to getLogs

* Fine tune view log, add auto refresh based on time interval set in UI setting

* Add useI18n to ui setting

* [Feat][UI] Add UI setting page in the project.

* [Feat][UI] Add UI setting page in the project.

* Remove logTimer in cookies

* Clear timer id, set VITE_APP_DEV_WEB_URL back to default

* Clear time id in dag

* [Feat][UI] Add license header.

* [Feat][UI] Remove console.

* [Fix][UI] Fix log timer types.

Co-authored-by: songjianet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
don't merge feature new feature first time contributor First-time contributor UI ui and front end related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature][UI] Provide users with a button to enable auto-refresh
6 participants