Skip to content

Migrate 'Failed to delete account' alert from Vuetify to Kolibri Des… - #5079

Merged
MisRob merged 1 commit into
learningequality:unstablefrom
Abhishek-Punhani:Issue5062
Jun 4, 2025
Merged

MisRob merged 1 commit into
learningequality:unstablefrom
Abhishek-Punhani:Issue5062

Conversation

@Abhishek-Punhani

Copy link
Copy Markdown
Member

Summary

  • Replaced Vuetify Alert with K-Modal
  • No UI changed
    …

References

#5062
…

Reviewer guidance

  • Login as user@a.com
  • Go to Settings > Account
  • In code, temporarily modify deletionFailed to display the alert

Screenshot from 2025-05-30 17-54-28

…

@Abhishek-Punhani
Abhishek-Punhani force-pushed the Issue5062 branch 3 times, most recently from 9145779 to f6f1da0 Compare May 30, 2025 12:43
@MisRob
MisRob self-requested a review May 30, 2025 13:18
@MisRob MisRob self-assigned this May 30, 2025

@MisRob MisRob left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @Abhishek-Punhani, thanks, implementation is as expected and manual testing passes!

One possible area to address on this opportunity would be to update should show alert if account deletion fails test case. Instead of expect(wrapper.vm.deletionFailed).toBe(true);, we rather want to check that KModal component is displayed with the correct text. You don't need to test everything about how that modal looks like since that's covered by KDS, just a basic check that it shows.

This will also cleanup a bit of our old tech debt - previous use of deletionFailed value to test whether the alert is really displayed or not is fragile approach.

It's not blocking. Let me know if that's something you'd like to addres.

@Abhishek-Punhani

Copy link
Copy Markdown
Member Author

Sure I will update the unit tests as well

@Abhishek-Punhani
Abhishek-Punhani force-pushed the Issue5062 branch 2 times, most recently from 9506e36 to 4f6a3de Compare June 2, 2025 18:19
@Abhishek-Punhani

Copy link
Copy Markdown
Member Author

Hi @Abhishek-Punhani, thanks, implementation is as expected and manual testing passes!

One possible area to address on this opportunity would be to update should show alert if account deletion fails test case. Instead of expect(wrapper.vm.deletionFailed).toBe(true);, we rather want to check that KModal component is displayed with the correct text. You don't need to test everything about how that modal looks like since that's covered by KDS, just a basic check that it shows.

This will also cleanup a bit of our old tech debt - previous use of deletionFailed value to test whether the alert is really displayed or not is fragile approach.

It's not blocking. Let me know if that's something you'd like to addres.

@MisRob I have updated the PR , please check at your convenience

@MisRob MisRob left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@MisRob
MisRob merged commit efb74db into learningequality:unstable Jun 4, 2025
@MisRob MisRob linked an issue Jun 13, 2025 that may be closed by this pull request
11 tasks
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.

[Remove Vuetify from Studio] 'Failed to delete account' alert in Settings - Account

2 participants