Skip to content
This repository has been archived by the owner on Jan 16, 2022. It is now read-only.

content-item-datasource companion PR #425

Merged
merged 12 commits into from
Jul 21, 2021
Merged

Conversation

vinnyjth
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #425 (7b0589f) into master (01bdbaf) will decrease coverage by 1.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
- Coverage   59.81%   58.73%   -1.09%     
==========================================
  Files          44        7      -37     
  Lines         443      126     -317     
  Branches       87       27      -60     
==========================================
- Hits          265       74     -191     
+ Misses        165       50     -115     
+ Partials       13        2      -11     
Impacted Files Coverage Δ
...olloschurchapp/src/user-settings/ChangePassword.js 45.83% <ø> (-0.33%) ⬇️
apolloschurchapp/src/user-settings/index.js 35.48% <ø> (+3.66%) ⬆️
apolloschurchapp/src/event/EventConnected.js 100.00% <100.00%> (ø)
...lloschurchapp/src/user-settings/PersonalDetails.js 54.05% <100.00%> (+5.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 123d4a3...7b0589f. Read the comment docs.

@vinnyjth vinnyjth changed the title Keep track of changes needed for postgres content-item datasource content-item-datasource companion PR Jul 15, 2021
@redreceipt redreceipt marked this pull request as draft July 16, 2021 19:58
@vinnyjth vinnyjth marked this pull request as ready for review July 19, 2021 17:39
@vinnyjth
Copy link
Member Author

@redreceipt this is ready to merge

@redreceipt
Copy link
Member

redreceipt commented Jul 19, 2021

got a migration error, and also images and content aren't coming through

Screen Shot 2021-07-19 at 2 40 05 PM

Kapture 2021-07-19 at 14 41 02

@vinnyjth
Copy link
Member Author

@redreceipt the failing migration is a migration that existed way before this body of work :/ I would recommend wiping out your postgres development server and starting from scratch.

I'll preempt your question and argue that this is okay ;)

If it works from scratch, that will match where most clients are starting from. All of our clients that I checked (newspring and rivervalley) haven't run that migration yet, but also don't have the personId field. They will not run into issues.

As for

images and content aren't coming through

You are pointing at your local postgres right? You'll also need to run the shovel locally. Or you can swap out your database url for the production postgres URL in the shared.env.enc

@redreceipt
Copy link
Member

redreceipt commented Jul 19, 2021 via email

@vinnyjth
Copy link
Member Author

@redreceipt Ah - that makes more sense actually. The DB was in a weird state since we were deving with it. I reset it and you should be gtg now.

@vinnyjth
Copy link
Member Author

image

@redreceipt
Copy link
Member

it's kicking me out immediately after login

Screen Shot 2021-07-19 at 3 10 39 PM

Kapture 2021-07-19 at 15 11 26

@vinnyjth
Copy link
Member Author

@redreceipt My bad again 😬 I didn't backfill people after I wiped the DB, give it another go. No need to change again, just try logging in again.

@redreceipt
Copy link
Member

closer

  • the continue feature on the home screen is broken
  • event images aren't coming through
  • nothing is on the read tab
  • the watch tab is broken

Screen Shot 2021-07-19 at 3 22 31 PM

Screen Shot 2021-07-19 at 3 22 35 PM

Screen Shot 2021-07-19 at 3 22 42 PM

Screen Shot 2021-07-19 at 3 22 44 PM

@vinnyjth
Copy link
Member Author

haha - I broke it when I wiped the DB (since the config.yml points at IDS in the DB now)

Let me fix and confirm and then I'll circle back

@vinnyjth
Copy link
Member Author

@redreceipt Ready for review again. A few notes...

  • Event images are broken, I'm considering that a won't fix for this PR. We will handle that by shoveling events in the future.
  • ButtonLink feature is broken. I want to merge this without a working ButtonLink feature, but will be working on a fast follow with the fix.

@vinnyjth
Copy link
Member Author

vinnyjth commented Jul 19, 2021

Continue feature being broken ... that's a tough one :/ we'll likely do a follow up PR that fixes that specific case. We will be able to come up with a really robust fix when we port in Interactions into postgres.

Edit - by "a tough one", I mean that it's a result of you having completed some items that are now referenced by a different ID. I think we need to filter those results out entirely instead of showing this weird state.

@redreceipt
Copy link
Member

So safe to assume churches that use Rock events (Willow may be the only one???) can't use the DB until we fix events?

@vinnyjth
Copy link
Member Author

vinnyjth commented Jul 19, 2021

So safe to assume churches that use Rock events (Willow may be the only one???) can't use the DB until we fix events?

Correct! Well actually Willow might be fine since their implementation of events is largely custom anyway.

I've captured the bugs here in this todo list: https://3.basecamp.com/3926363/buckets/6947926/todolists/3971672104 Anything captured in this list I hope to finish this cycle.

I really want to get this PR merged so we have some solid ground to work off of, once this is merged we'll be able to go back to smaller focused PR's instead of trying to work around the monolith :)

@redreceipt
Copy link
Member

I think it's important we treat this template as a working app with working features. The only reason I'm being a stickler is with more and more apps, the upgrade process needs to be smooth and with some features not working on the DB in the template, that could get very difficult to track. I'm committing to upgrading and releasing an app every eight weeks and need to be able to count on the template app not having accepted bugs in it.

@redreceipt redreceipt marked this pull request as draft July 19, 2021 20:23
@redreceipt
Copy link
Member

we decided to fix the obvious bugs before merging this PR, more updates to come...

@vinnyjth
Copy link
Member Author

Summary of slack huddle:

Setting this to draft
Fixing bugs in this
When no obvious bugs are remaining, we look at a way to push this PR out in a way that doesn't inhibit our ability to update non content-item Postgres apps

@vinnyjth
Copy link
Member Author

@redreceipt "Series in Progress" empty state is broken on master as well

image

It's actually a bug with the content card. The API is sending over the same response every time, but the front end doesn't render the message type correctly I think?

{
  "data": {
    "node": {
      "id": "HorizontalCardListFeature:9724110384a5109532f35a4980ea9b7710c5d31e1ac7d657197c0b74fd401df69286cd1d18f9eee3d1c46520b8c4da91fa5f2d1ff423886173e0bdb7d612c7d714e28f3cb6472cdce5ea3ac72c4f6a35ec354b9d680a119775a33f4969995f3f07a8d14629ff99d33c7651baccf3e8f34f382889a21013916015f009776eeb84",
      "title": null,
      "subtitle": "Continue",
      "cards": [
        {
          "action": null,
          "title": null,
          "hyphenatedTitle": null,
          "hasAction": null,
          "actionIcon": null,
          "labelText": null,
          "summary": null,
          "coverImage": null,
          "relatedNode": {
            "id": "Message:99511d5ba7917b86d61382a46ad38396",
            "message": "All caught up!",
            "__typename": "Message"
          },
          "__typename": "CardListItem"
        }
      ],
      "primaryAction": null,
      "__typename": "HorizontalCardListFeature"
    }
  }
}

@redreceipt
Copy link
Member

well dang. I hated that feature as I was writing it...

@vinnyjth
Copy link
Member Author

@redreceipt Once this release finishes (https://github.com/ApollosProject/apollos-apps/actions/runs/1050109938) this PR will be ready for review again. Taking it back out of draft.

@vinnyjth vinnyjth marked this pull request as ready for review July 20, 2021 19:45
@redreceipt
Copy link
Member

error when you're not using PG (DATABASE_URL is blank)

Screen Shot 2021-07-21 at 11 55 38 AM

@vinnyjth
Copy link
Member Author

@redreceipt yahhh this is going to be problamatic. The old config.yml and the new postgres config.yml use different formats. Should we create a config.postgres.yml?

@redreceipt
Copy link
Member

no it's a network error, I'll track it down. you probably can't reproduce unless you log out in between. i bet it's the config yml thing

@vinnyjth
Copy link
Member Author

@redreceipt I can reproduce, and it looks like an error in Rock caused by trying to filter by a guid coming from the config.yml

@vinnyjth vinnyjth merged commit bb2bf15 into master Jul 21, 2021
@vinnyjth vinnyjth deleted the content-item-datasource branch July 21, 2021 18:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants