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

Add media:thumbnail and itunes:image to generated rss feeds #65

Merged
merged 3 commits into from
Apr 2, 2022

Conversation

orthur
Copy link
Contributor

@orthur orthur commented Mar 30, 2022

PR for #64

@orthur orthur force-pushed the master branch 2 times, most recently from 14d806b to 04f2d5e Compare March 30, 2022 16:51
baseURL := s.Conf.System.BaseURL
rss.Link = baseURL + "/feed/" + feedName
rss.ItunesImage = feed.ItunesImg{
URL: baseURL + "/image/" + feedName,
Copy link
Owner

Choose a reason for hiding this comment

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

we shouldn't form this image URL automatically but rather conditionally from s.Conf.Feeds[feedName].Image (if not empty)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, fixed

Copy link
Owner

Choose a reason for hiding this comment

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

almost ;) the image url should be baseURL+s.Conf.Feeds[feedName].Image instead of generic baseURL + "/image/" + feedName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah got it now

Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

Thx, looking good.

As we touched RSS output, could you pls add a test checking generated RSS, with and without images

@orthur
Copy link
Contributor Author

orthur commented Mar 30, 2022

Ok will add

@orthur
Copy link
Contributor Author

orthur commented Apr 1, 2022

@umputun could you please guide me to the right direction with tests? since there are no existing tests for generated feeds, do I have to create tests for handler getFeedCtrl ? I would have to create test config file, test db and test server?

And it would look something like that:

// setup test db, config, server
req, _ := http.NewRequest("GET", "/rss/echo-msk", nil)
rr := httptest.NewRecorder()
s.MountHandlers(8080)
s.httpServer.Handler.ServeHTTP(rr, req)
// do checks for rr.Body

@umputun
Copy link
Owner

umputun commented Apr 2, 2022

in fact this server wasn't too testable. I have added ctx param to Run so the integration test could terminate it and extracted router() func, so you could pass it into httptest.Server

regarding the test itself - all you need to start it to make a testing boltdb and config filled with whatever you need and pass it in.

After this, you can call bolt's save for several records and should be good to go.

so, your test would look like:

  	tmpfile := filepath.Join(os.TempDir(), "test.db")
	defer os.Remove(tmpfile)
	db, err := bolt.Open(tmpfile, 0o600, &bolt.Options{Timeout: 1 * time.Second})
	require.NoError(t, err)
	boltStore := &proc.BoltDB{DB: db}

        conf := config.Conf{.....}
        srv := Server{Store: boltStore, Conf: conf}
        ts := httptest.Server(srv.router())
        defer  ts.Close()

From this point, you could send the real http request to ts.URL + "/whatever" and check the response

hope it helps

Artur added 2 commits April 2, 2022 14:07
fix rebase conflict
@orthur
Copy link
Contributor Author

orthur commented Apr 2, 2022

that helps, thanks

@umputun
Copy link
Owner

umputun commented Apr 2, 2022

wanna me to merge this pr as-is and you could add test(s) in the following pr?

@orthur
Copy link
Contributor Author

orthur commented Apr 2, 2022

yes lets do that

@umputun umputun merged commit 659665d into umputun:master Apr 2, 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.

2 participants