-
Notifications
You must be signed in to change notification settings - Fork 32
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
Implemented: Updated the DataManager Logs page to be dynamic in Job Manager(#680) #729
base: main
Are you sure you want to change the base?
Conversation
<template> | ||
<ion-content> | ||
<ion-list> | ||
<ion-list-header>{{ translate('Log Id') }}</ion-list-header> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will the data manager log ID not the text "Log Id".
<ion-item button @click="downloadLogFile('logFile')"> | ||
{{ translate('Log file') }} | ||
</ion-item> | ||
<ion-item button :lines="log?.errorRecordContentId ? undefined : 'none'" @click="downloadLogFile('uploadedFle')"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should show error item disabled instead of hiding, if there is not error content.
let dataResource = {} as any; | ||
|
||
if (type === 'logFile') { | ||
contentIdType = 'logFileContentId'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename contentIdType to contentId and fetch the contentId in the if/else block itself.
src/store/modules/job/actions.ts
Outdated
let logs = [] as any | ||
const payload = { | ||
"inputFields": { | ||
"statusId": ["SERVICE_CANCELLED", "SERVICE_CRASHED", "SERVICE_FAILED", "SERVICE_FINISHED", "SERVICE_PENDING", "SERVICE_RUNNING", "SERVICE_QUEUED"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to apply any filter on status.
src/store/modules/job/actions.ts
Outdated
}).catch((err: any) => { | ||
logger.error(err); | ||
}) | ||
commit(types.JOB_DATA_RESOURCE_IDS_UPDATED, dataResourceIds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of storing dataResourceIds separately, I think you should add the needed field in the respective data manager config record itself.
src/store/modules/job/actions.ts
Outdated
commit(types.JOB_DATA_RESOURCE_IDS_UPDATED, dataResourceIds); | ||
}, | ||
|
||
async fetchDataManagerConfig({ commit }, params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we are not storing anything to state here, we can skip using action here. Service itself should fulfil the purpose.
|
||
if (dataResource) { | ||
try { | ||
const response = await JobService.downloadCsv({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this service name to something like fetchFileData.
src/components/ImportLogsDetail.vue
Outdated
</div> | ||
|
||
<div class="ion-padding"> | ||
<ion-chip v-for="(chip, index) in chips" :key="index" outline @click="updateLogs(chip.label)"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list name should be logFilters or dataManagerLogFilters instead of chips.
src/components/ImportLogsDetail.vue
Outdated
</div> | ||
|
||
<div class="ion-padding"> | ||
<ion-chip v-for="(chip, index) in chips" :key="index" outline @click="updateLogs(chip.label)"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updateLogs should also be rename to something filterLogs or filterDataManagerLogs.
src/components/ImportLogsDetail.vue
Outdated
<p>{{ translate('Finished') }}</p> | ||
</ion-label> | ||
|
||
<ion-badge color="success" v-if="log.statusId === 'SERVICE_FINISHED'">{{ translate('Finished') }}</ion-badge> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Display the status description here. You need to fetch the status data for statusTypeId='SERVICE_STATUS' and then create uiLables for all the statuses.
src/components/ImportLogsDetail.vue
Outdated
return { | ||
configDetails: {}, | ||
selectedChip: 'All', | ||
chips: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be renamed to logFilters and it should be the array of object, each filter object with key and value. So that when apply filters you can pass the ID.
…e text refactor job actions(hotwax#680)
… and made changes in the css(hotwax#680)
} | ||
|
||
.config { | ||
.config-details { | ||
align-self: end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed. The content should instead expand to fill the remaining space. both grid columns should take equal space. The Width of the grid should be restricted to control the width of its children.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, I mixed up justify and align. Thanks for clarifying. It was correct the way you had it
Related Issues
#680
Short Description and Why It's Useful
Screenshots of Visual Changes before/after (If There Are Any)
Contribution and Currently Important Rules Acceptance