Skip to content

Error when clicking the Finish or Close button #3316

Merged
MisRob merged 3 commits intolearningequality:unstablefrom
d0sadata:close-button-bug
Feb 24, 2022
Merged

Error when clicking the Finish or Close button #3316
MisRob merged 3 commits intolearningequality:unstablefrom
d0sadata:close-button-bug

Conversation

@d0sadata
Copy link
Copy Markdown
Contributor

@d0sadata d0sadata commented Jan 27, 2022

Summary

Description of the change(s) you made

This PR fixes #3153 though it doesn't explain the underlying code that makes the bug properly.

After trying to narrow down the issue for a while, and consulting Marcella on it, I discovered that the ImmediateSave all function can't be called since details tab is not loaded, that is blocking the save or exit functionality of the View.

I tried multiple solutions to this issue, tested why it wasn't showing up, and upon consulting Marcella she suggested we add another check in EditView to see if details tab exists pre function call. that resolved the issue.

Manual verification steps performed

  1. Opened all possible combination of details tab editing and related view editing and clicked exit at all possible steps.

Does this introduce any tech-debt items?

We need to find the main reason details tab is not loaded when it's active in EditView, in EditModal.


Reviewer guidance

How can a reviewer test these changes?

  1. Go to https://hotfixes.studio.learningequality.org/en/accounts/#/ and sign in.
  2. Click on a channel to go to the channel editor and select the Edit details option for a resource.
  3. Click the Related tab and add a previous and/or a next step.
  4. Click the Finish button or the Close icon.

The bug appeared whenever there instance was called and an item was added or removed from a previous next step.

Contributor's Checklist

Testing:

  • Code is clean and well-commented

  • Contributor has fully tested the PR manually

  • If there are any front-end changes, before/after screenshots are included

  • Critical user journeys are covered by Gherkin stories

  • Any new interactions have been added to the QA Sheet

  • Critical and brittle code paths are covered by unit tests


Reviewer's Checklist

This section is for reviewers to fill out.

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@d0sadata d0sadata self-assigned this Jan 27, 2022
@sonarqubecloud
Copy link
Copy Markdown

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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@bjester bjester added this to the 2022Q1 Phase 1: tasks milestone Feb 15, 2022
Copy link
Copy Markdown
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Thank you, @d0sadata and @marcellamaki.

@d0sadata You're right that the <DetailsTabView> is not loaded.

We need to find the main reason details tab is not loaded when it's active in EditView, in EditModal.

The main reason is that when you navigate away from the edit modal to the page for selecting previous/next steps, all components living in the edit modal get unmounted, which means that when you return to the "Related" tab from the page for selecting previous/next steps, all those edit modal and view components, including "Details" and "Related" tab views are mounted again. However, tabs are rendered lazily, which means that the "Details" tab view is not available at the point you arrive in the "Related" tab view (causing $refs.detailsTabView to be undefined).

This can be solved by removing lazy prop from here. I think that we shouldn't need this condition after that.

I am not 100% sure if your solution may be causing some problems because we're saving updates quite often, so it's possible that skipping calling immediateSaveAll in this particular case wouldn't cause any data loss, but also would say it may be safer to make sure that immediateSaveAll has been called as planned rather than not? The whole edit modal and its save logic is quite complex, and honestly, I have trouble fathoming implications of this condition entirely.

And ideally, the save method and related data wouldn't live in <DetailsTabView> but somewhere higher in the state so that we wouldn't need to be concerned whether the tab has been loaded or not when it's not even visible to the user. You're welcome to open an issue for that if you'd like.

Fixed reference to Details Tab.
@d0sadata d0sadata requested a review from MisRob February 22, 2022 22:31
@d0sadata
Copy link
Copy Markdown
Contributor Author

I have fixed the PR and solved the issue, the core was the $refs, as the Details Tab View was named DetailsTab. Also removed lazy as per Misha's suggestion after consulting Blane as to what the Lazy prop is referencing to in studio.

Copy link
Copy Markdown
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Thank you, @d0sadata

@MisRob MisRob merged commit cdfe2b2 into learningequality:unstable Feb 24, 2022
@d0sadata d0sadata deleted the close-button-bug branch March 1, 2022 14:14
@d0sadata d0sadata removed their assignment Aug 21, 2022
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.

Error when clicking the Finish or Close button after having added a previous and/or next step to a resource

3 participants