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

#3910 - Show application edit history ministry view #4213

Merged
merged 12 commits into from
Jan 10, 2025

Conversation

lewischen-aot
Copy link
Collaborator

@lewischen-aot lewischen-aot commented Jan 8, 2025

  • Created the menu on demand, only if any historical application exists.
    • Modify application view screen to have all versions of the app history on the left hand side.
    • Each version of the app will be a expandable selection.
    • Each app version will appear in descending order based on date time stamp.
    • Each application will open its application form upon clicking.
  • Added a new API endpoint for fetching historical applications to populate the menu.
  • Changed the API to allow the overwritten application information needed to build the menu for Ministry User.
    • Ensured API would allow the overwritten applications to be retrieved.
  • Modified and added E2E tests for the changed API endpoints.

Screenshot of the historical application items
image

@lewischen-aot lewischen-aot marked this pull request as ready for review January 8, 2025 01:14
@dheepak-aot dheepak-aot self-requested a review January 8, 2025 18:20
*/
async getApplicationVersionHistory(
applicationId: number,
): Promise<ApplicationVersionAPIOutDTO[]> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please take care of DTO as per previous discussions in service and API.

<v-list-item v-bind="props" :title="application.title"></v-list-item>
</template>
<v-list-item
v-for="child in application.children"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am able to see that, when the scrollbar appears in nav(which occupies some width), the time (counter 1 in screenshot) is hidden in the group headers. If we remove the Icon(counter 2 in screenshot), it will solve this problem. Is the icon really essential for business?

image

Comment on lines +307 to +308
path: AppRoutes.ApplicationVersionDetail,
name: AESTRoutesConst.APPLICATION_VERSION_DETAILS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing the testing, finding the issue and the fix. 👍

path: AppRoutes.ApplicationVersionDetail,
name: AESTRoutesConst.APPLICATION_VERSION_DETAILS,
props: true,
component: StudentApplicationView,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that we send the versionid we have to make a minor adjustment in StudentApplicationView.vue otherwise the API call to get application data will always use the applicationId and not the versionId.

Issue:

image

My best suggestion on solution:
image

@dheepak-aot
Copy link
Collaborator

Thanks for doing the changes. Almost there added few more comments.

@@ -45,6 +45,10 @@ export default defineComponent({
type: Number,
required: true,
},
versionApplicationId: {
type: Number,
required: true,
Copy link
Collaborator

@dheepak-aot dheepak-aot Jan 10, 2025

Choose a reason for hiding this comment

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

My bad. In the screenshot I shared, I kept the required: true by mistake , but the required must be false.

Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 22.21% ( 3783 / 17033 )
Methods: 10.17% ( 219 / 2153 )
Lines: 25.65% ( 3278 / 12778 )
Branches: 13.61% ( 286 / 2102 )

Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 65.59% ( 589 / 898 )
Methods: 59.63% ( 65 / 109 )
Lines: 68.72% ( 468 / 681 )
Branches: 51.85% ( 56 / 108 )

Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 86.14% ( 1249 / 1450 )
Methods: 82.42% ( 136 / 165 )
Lines: 88.51% ( 1032 / 1166 )
Branches: 68.07% ( 81 / 119 )

Copy link
Collaborator

@dheepak-aot dheepak-aot left a comment

Choose a reason for hiding this comment

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

Thanks for doing the changes. Great work. 👍

Copy link
Collaborator

@sh16011993 sh16011993 left a comment

Choose a reason for hiding this comment

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

Great work @lewischen-aot 👍

Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 67.45% ( 5938 / 8803 )
Methods: 65.18% ( 732 / 1123 )
Lines: 71.37% ( 4657 / 6525 )
Branches: 47.53% ( 549 / 1155 )

@lewischen-aot lewischen-aot added this pull request to the merge queue Jan 10, 2025
Merged via the queue into main with commit 7e837e2 Jan 10, 2025
21 checks passed
@lewischen-aot lewischen-aot deleted the feature/#3910-show-application-edit-history branch January 10, 2025 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants