Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

fix applySiteSettingRecord error, remove unneeded setObjectId calls #8242

Merged
merged 1 commit into from
Apr 12, 2017

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Apr 12, 2017

fix #8241

applySiteSettingRecord should not need to check if there is a diff between
the existing record and the new record since that should be the job of reducers

Auditors: @ayumi

Test Plan:

  1. 'Syncing' tests should pass
  2. Set isProduction to true in js/constants/appConfig
  3. Sync to the profile starting with "duality" (sent via slack DM)
  4. You should not see any console errors
  5. Go http://expectnothing.com/ and verify that shields are down
  • 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).

Test Plan:

fix #8241

applySiteSettingRecord should not need to check if there is a diff between
the existing record and the new record; this is the job of reducers.

Auditors: @ayumi

Test Plan:
1. 'Syncing' tests should pass
2. Set isProduction to true in js/constants/appConfig
3. Sync to the profile starting with "duality" (sent via slack DM)
4. You should not see any console errors
5. Go http://expectnothing.com/ and verify that shields are down
@diracdeltas diracdeltas added this to the 0.14.2 milestone Apr 12, 2017
@diracdeltas diracdeltas self-assigned this Apr 12, 2017
@diracdeltas diracdeltas requested a review from ayumi April 12, 2017 00:49
Copy link
Contributor

@ayumi ayumi left a comment

Choose a reason for hiding this comment

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

🎃

@@ -918,8 +914,6 @@ const handleAppAction = (action) => {

if (result === undefined) {
let newSiteSettings = siteSettings.mergeSiteSetting(appState.get('siteSettings'), pattern, 'ledgerPayments', true)
const syncObject = siteUtil.setObjectId(newSiteSettings.get(pattern))
newSiteSettings = newSiteSettings.set(pattern, syncObject)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: can newSiteSettings be const?

Copy link
Member

Choose a reason for hiding this comment

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

Pls do audited commit if changing, I'll merge this in the meantime. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

syncing site settings exception
4 participants