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 a custom_domain attribute for single-user blogs #340

Closed
wants to merge 8 commits into from
Closed

add a custom_domain attribute for single-user blogs #340

wants to merge 8 commits into from

Conversation

igalic
Copy link
Contributor

@igalic igalic commented Dec 8, 2018

We want to be able to serve a blog from a custom domain

this PR tries to follow what @BaptisteGelez's describes in #241 #94

@codecov
Copy link

codecov bot commented Dec 8, 2018

Codecov Report

Merging #340 into master will increase coverage by 0.32%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #340      +/-   ##
==========================================
+ Coverage   28.29%   28.62%   +0.32%     
==========================================
  Files          62       62              
  Lines        6545     6603      +58     
==========================================
+ Hits         1852     1890      +38     
- Misses       4693     4713      +20
Impacted Files Coverage Δ
plume-models/src/schema.rs 24.32% <ø> (ø) ⬆️
src/routes/user.rs 0% <0%> (ø) ⬆️
plume-cli/src/users.rs 0% <0%> (ø) ⬆️
plume-models/src/users.rs 39.58% <88.88%> (+0.29%) ⬆️
plume-common/src/utils.rs 91.42% <0%> (-0.66%) ⬇️
src/template_utils.rs 0% <0%> (ø) ⬆️
src/routes/search.rs 0% <0%> (ø) ⬆️

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 3830220...ff28e01. Read the comment docs.

@@ -183,6 +183,7 @@ table! {
is_admin -> Bool,
summary -> Text,
email -> Nullable<Text>,
custom_domain -> Nullable<Text>,
Copy link
Contributor

@trinity-1686a trinity-1686a Dec 8, 2018

Choose a reason for hiding this comment

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

Why is custom_domain NOT NULL in migrations but Nullable here?

Edit: I shall have commented migrations, this should definitely be nullable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does this mean we can resolve this?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can either leave this open or resolve and mark this and that instead

@@ -11,6 +11,7 @@ <h1>@i18n!(ctx.1, "Create an account")</h1>
<form method="post">
@input!(ctx.1, username (text), "Username", form, errors.clone(), "minlenght=\"1\"")
@input!(ctx.1, email (text), "Email", form, errors.clone())
@input!(ctx.1, email (text), "Email", form, errors.clone(), "")
Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't ask for a custom domain when registering (I guess that's what you wanted to do here, even if it looks like you forgot to update what you copied)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the catch, i changed this to:

            @input!(ctx.1, custom_domain (optional text), "custom_domain", form, errors.clone(), "")

and now i get this joyful error:

   --> src/template_utils.rs:127:30                                                                                                                                           
    |                                                                                                                                                                         
127 |                 val = escape(&$form.$name),                                                                                                                             
    |                              ^^^^^^ expected str, found enum `std::option::Option`                                                                                      
    |                                                                                                                                                                         
   ::: /home/meena/src/ap/Plume/target/debug/build/plume-7e8607967e0d30ae/out/templates/users/template_new.rs:21:1                                                            
    |                                                                                                                                                                         
21  | input!(ctx.1, custom_domain (optional text), "custom_domain", form, errors.clone(), "").to_html(out)?;                                                                  
    | --------------------------------------------------------------------------------------- in this macro invocation                                                        
    |                                                                                                                                                                         
    = note: expected type `&str`                                                                                                                                              
               found type `&std::option::Option<std::string::String>`                                                                                                         
                                                                                                                                                                              
error: aborting due to previous error                                                                                                                                         
                                                                                                                                                                              
For more information about this error, try `rustc --explain E0308`.                                                                                                           
error: Could not compile `plume`.                                                                                                                                             

Copy link
Contributor

Choose a reason for hiding this comment

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

input! don't support Option, make custom_domain (in NewUserForm) not optional, and map it to None whenever it's empty in create(...)

Copy link
Contributor Author

@igalic igalic left a comment

Choose a reason for hiding this comment

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

🎉

@@ -0,0 +1,31 @@
-- This file should undo anything in `up.sql`
PRAGMA foreign_keys = ON;
CREATE TABLE migrate_users_custom_domain (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i realized we can give our temporary tables sensible names!

@igalic
Copy link
Contributor Author

igalic commented Dec 18, 2018

With the conclusions that emerged from the discussion (Custom Blog domains and where to attach them) i've decided to close this, and with a better understanding, open a new pull request.

Thank you all for helping me with this!

@igalic igalic closed this Dec 18, 2018
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.

3 participants