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

[ENG-4077] Fix accessing component's registration #2001

Conversation

chth0n1x
Copy link
Contributor

@chth0n1x chth0n1x commented Sep 15, 2023

Purpose

The purpose of these changes is to fix accessing a component's registration from the component's project registrations page.

Summary of Changes

  • Updated the return value of the overview index route's root function
  • Updated the overview page's OsfLink models array value if it is the root
  • Updated the link text for the registry overview page to display on the registration

Screenshot(s)

Pasted Graphic 24

Figure 1: OsfLink now displays on the registries overview page of linked projects

Side Effects

There should be no side effects.

QA Notes

-Does the link text now properly display?
-Does the link route you to the appropriate component?
-Does the rest of the registration summary now display when opened from the project registration page?

lib/registries/addon/overview/index/controller.ts Outdated Show resolved Hide resolved
lib/registries/addon/overview/index/template.hbs Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Sep 15, 2023

Pull Request Test Coverage Report for Build 6510376027

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 71.215%

Files with Coverage Reduction New Missed Lines %
lib/registries/addon/overview/-components/diff-manager/component.ts 2 67.8%
Totals Coverage Status
Change from base Build 6483532224: -0.02%
Covered Lines: 5941
Relevant Lines: 8129

💛 - Coveralls

Copy link
Contributor

@adlius adlius left a comment

Choose a reason for hiding this comment

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

This does fix the problem, but adds an unnecessary AJAX call to fetch the asynchronous root relationship.
There is a reason why the bug only happens when you transition from the project's registration list view to the registration overview page by clicking the link, but not when you go to the registration overview page directly. There is some race condition going on here and the proper fix would be to address the race condition.

lib/registries/addon/overview/index/controller.ts Outdated Show resolved Hide resolved
@chth0n1x chth0n1x force-pushed the fix-accessing-components-registration branch from 2394e6b to 3910a2a Compare September 28, 2023 15:50
Copy link
Contributor

@adlius adlius left a comment

Choose a reason for hiding this comment

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

Tested locally and it seems it's not quite doing what it's supposed to do.

The correct behavior of the page:

  1. When the registration is a root registration, it should not show any link.
  2. When the registration is a non-root registration, it should show a link pointing to the root registration

However the behavior of the page after these changes would be:

  1. When the registration is a root registration, it shows a link pointing to itself.
  2. When the registration is a non-root registration, it doesn't show any link.

lib/registries/addon/overview/index/template.hbs Outdated Show resolved Hide resolved
app/resolve-guid/guid-route.ts Outdated Show resolved Hide resolved
lib/registries/addon/overview/index/controller.ts Outdated Show resolved Hide resolved
@chth0n1x chth0n1x force-pushed the fix-accessing-components-registration branch from eac946c to d63cb61 Compare September 28, 2023 21:56
Copy link
Contributor

@adlius adlius left a comment

Choose a reason for hiding this comment

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

Checked locally again. The link is missing after transitioning from the registration list to the registration overview page.
Screenshot from 2023-09-29 08-19-41

I'd like to expand more on the cause of the bug. Ember has asynchronous routing feature. That means, if the route model hook of a particular route returns a promise (as is the case with guid-route route), the route transition would happen after the promise returned by the model hook is resolved. In this case, the bug happens because the promise returned by the model hook does resolve, but the root relationship of the registration has still yet to be loaded. Note that this happens only when your transition from the registration list view to the registration overview route. The fix to the bug requires understanding why the promise returned by the model hook resolves but the root relationship is still not loaded.

lib/registries/addon/overview/index/controller.ts Outdated Show resolved Hide resolved
@chth0n1x chth0n1x force-pushed the fix-accessing-components-registration branch from 6816c64 to 7defae4 Compare October 5, 2023 16:50
@chth0n1x chth0n1x force-pushed the fix-accessing-components-registration branch from 7814715 to 26ecfad Compare October 5, 2023 19:22
@brianjgeiger brianjgeiger merged commit 0b88341 into CenterForOpenScience:develop Oct 13, 2023
9 checks passed
@chth0n1x chth0n1x deleted the fix-accessing-components-registration branch October 20, 2023 09:40
@futa-ikeda futa-ikeda added this to the 23.13.0 milestone Oct 23, 2023
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.

5 participants