Removes non-primitive types from NavigationBar

[NejcZdovc]

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.

Resolves #9791

Auditors: @bridiver @bsclifton

Test Plan:

• try to add a bookmark (star)
• try to remove bookmark

Reviewer Checklist:

Tests

• Adequate test coverage exists to prevent regressions
• Tests should be independent and work correctly when run individually or as a suite ref
• New files have MPL2 license header

[bsclifton]

Feedback left 😄

[bsclifton]

This is definitely something we want to avoid (calling an action in the handler for an action). If it can't be avoided, I think it would be preferable to call doAction like so:

doAction({
  actionType: windowConstants.WINDOW_SET_BOOKMARK_DETAIL,
  currentDetail: siteDetail,
  originalDetail: siteDetail,
  destinationDetail: null,
  shouldShowLocation: action.isBookmarked,
  isBookmarkHanger: true
})

cc: @bridiver

[NejcZdovc]

Yeah, we want to remove this, but sadly we need to keep it for now. You will see this pattern in other PR's as well. For this particular case we will remove it in #9710, because we will remove setBookmarkDetail as per our discussion.

[bsclifton]

this logic here would be better if it was contained inside siteUtil.getDetailFromFrame method, IMO (especially since it can be tested). There are other places that call this method and they too should be having the parentFolderId and customTitle set (in fact, if the other calls are not setting this, it may be causing a bug / bugs)

[NejcZdovc]

Not sure why this logic should be in siteUtil.getDetailFromFrame. In this if statement we are getting data from appStore and adding them to the siteDetail. Not sure where else would we be using this? We use this function in 13 different places. We only want to access appStore in reducers and not in components.

[bsclifton]

the data it gets from appStore is the site which matches... getDetailFromFrame should be returning the entry from the sites map (which is what siteUtil does; it's the wrapper around the sites map which is stored in appStore)

In this case, getDetailFromFrame is just taking data from the frame and returning it (without transforming it)... rather than doing a lookup and fetching the already existing one. This code here then copies the data from the existing one (which is a mistake IMO, since it could have just used the existing reference)

[bsclifton]

Here's a quick explanation of why this is a problem:
if you remove this line:
siteDetail = siteDetail.set('parentFolderId', site.get('parentFolderId'))

The code would then create a NEW sites entry (which is wrong). The reason why it creates a new one is because the key has the folderId in it. The windowStore shouldn't care about this logic- it should just use the object it gets back. Instead, we are having business logic in the windowStore about a siteDetails entry to avoid a bug (instead of returning an existing reference- the one which the key matches)

[bsclifton]

this would fix the problem, although it's a little more sloppy:

siteDetail = appStoreRenderer.state.getIn(['sites', key], Immutable.Map())
// siteDetail = siteDetail.set('parentFolderId', site.get('parentFolderId'))
// siteDetail = siteDetail.set('customTitle', site.get('customTitle'))

[NejcZdovc]

Not sure that we can to that, because now in your example you overrode sites from window state, which means that bookmark will not have favicon, tags, themeColor etc. It will only have parentFolderId, title and customTitle

[NejcZdovc]

we are only missing parentFolderId in this example, I just checked and everything else is there. But I think this whole PR, will be redundant when I do #9710 😃

[bsclifton]

that's true- maybe it's OK as-is (for this PR) and that would be the place to tackle it 😄

The example above should have everything (including parentFolderId). In the current code, whatever is found is copied from appState. So instead of copying it, just use the entire object (which like you said, has favicon, tags, themeColor, etc). This is great because if we needed to expose those we could. By not using the real object, we're opening ourselves up to bugs. For example, what if parentFolderId is changed to parentDirectoryId. If we properly handled it, we'd only need to update in siteUtil. But because this code is copying the object, we'd have to update in 2 (or more) places

[NejcZdovc]

Closing in favor of #9710