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 flash message with info after login #1952

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

Mordorreal
Copy link

@Mordorreal Mordorreal commented Dec 14, 2019

  • Add new types for flash message
  • Show info type flash message after login via github
  • Add missed tests

Small initial step to address #19

- Add new types for flash message
- Show info type flash message after login via github
- Add missed tests

Small initial step to address rust-lang#19
@rust-highfive
Copy link

@rust-highfive rust-highfive commented Dec 14, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jtgeibel (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

const messageAfterLogin = htmlSafe(
"Welcome to crates.io! Visit <a href='me'>account settings</a> to verify your email address and create an API token!",
);
this.flashMessages.show(messageAfterLogin, { type: 'info' });
Copy link
Member

@Turbo87 Turbo87 Dec 16, 2019

Choose a reason for hiding this comment

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

do you want to show this message to all users? even if they have verified their data already? 🤔

Copy link
Author

@Mordorreal Mordorreal Dec 16, 2019

Choose a reason for hiding this comment

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

For first version yes. Currently trying to understand how I can pass data from backend and what parameters it will be. I think about { "email_verified": false, "api_key_verified": true } but backed currently is a real mess for me. No good structure and abstractions after rails. Still trying to learn how to handle that.

Copy link
Member

@carols10cents carols10cents Dec 18, 2019

Choose a reason for hiding this comment

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

Hi @Mordorreal! Thank you for this pull request!

I'm sorry our backend is such a mess; there isn't really a Rails for Rust yet and crates.io was written even before any of the experimental web frameworks existed!

There is a router.rs file that is similar (in purpose at least) to Rails' routes.rb. There are two backend routes that will be relevant for this PR:

The data returned currently is defined by the "view" struct EncodableMe. Because the backend is only a JSON API, there are no HTML views, so all the views are structs that get serialized to JSON with serde.

EncodableMe contains an EncodablePrivateUser, which has an email_verified field, so you should be able to use that!

We can add a field to EncodablePrivateUser called has_tokens or similar that is true if the user has at least one token and false if they have none. We'd need to add a query to user::me::me that gets that information. The query should use ApiToken::belonging_to sort of like this code does and diesel's exists function sort of like this code does. I'm actually not sure off the top of my head exactly how to combine those two! Diesel's docs is where I'd look to figure that out. Want to give that a try?

Let me know if you have more questions, I hope that helps to pick the relevant pieces out of our mess at least!

Copy link
Author

@Mordorreal Mordorreal Dec 19, 2019

Choose a reason for hiding this comment

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

Hi @carols10cents! Thank you for very detailed answer! You're awesome! I'll try to continue on the issue in this PR.

Copy link
Member

@carols10cents carols10cents left a comment

The flash message is looking great!! Thank you so much!!

Screen Shot of the flash message

Because we only merge changes that are deployable, I think we need to go in one of two directions with this PR before we can merge it:

  • Disable the flash messages entirely, so this PR is only adding the ability to have a flash message without actually using that ability
  • Add the attribute needed in the backend API to tell whether the user has any tokens or not, so that we can show the flash message only when a user needs to do those things (if we go in the first direction, this can be a future PR)

Let me know what you think and if you have any questions. Thank you so much!

@bors
Copy link
Contributor

@bors bors commented Dec 25, 2019

The latest upstream changes (presumably #2052) made this pull request unmergeable. Please resolve the merge conflicts.

@Mordorreal
Copy link
Author

@Mordorreal Mordorreal commented Dec 26, 2019

@carols10cents Update with all fixes and improvements. Feedback is welcome!

@Mordorreal
Copy link
Author

@Mordorreal Mordorreal commented Dec 26, 2019

Can someone with permissions also restart CI? Looks like there some problems with testing environment.

@bors
Copy link
Contributor

@bors bors commented Dec 28, 2019

The latest upstream changes (presumably #2067) made this pull request unmergeable. Please resolve the merge conflicts.

app/routes/me/index.js Show resolved Hide resolved
@@ -1,4 +1,4 @@
<p id='flash' class='shown'>Oops, that route doesn't exist!</p>
<p id='flash' class='show'>Oops, that route doesn't exist!</p>
Copy link
Member

@Turbo87 Turbo87 Dec 29, 2019

Choose a reason for hiding this comment

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

why did you change the class name?

Copy link
Author

@Mordorreal Mordorreal Dec 31, 2019

Choose a reason for hiding this comment

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

For me, the noun is made more sense here, because this is what this css class doing. But English is not my native language and maybe I'm wrong.

tests/integration/components/welcome-message-test.js Outdated Show resolved Hide resolved
this.set('message', message);
this.set('options', options);
},
Copy link
Member

@Turbo87 Turbo87 Dec 29, 2019

Choose a reason for hiding this comment

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

are these changes still necessary? it looks like we're not using flash messages anymore

Copy link
Author

@Mordorreal Mordorreal Dec 31, 2019

Choose a reason for hiding this comment

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

If the project has no plan to bring better user exp with right info or success messages then I can remove it.

app/routes/login.js Outdated Show resolved Hide resolved
].filter(e => !!e);

return textArray.join('');
}),
Copy link
Member

@Turbo87 Turbo87 Dec 29, 2019

Choose a reason for hiding this comment

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

I think I would prefer it if we moved that code into the index controller and not instantiate the component at all if we don't have anything to display

Copy link
Author

@Mordorreal Mordorreal Dec 31, 2019

Choose a reason for hiding this comment

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

This logic has nothing in common with the index controller. This is common arch mistake in all MVC frameworks. Trying to move business logic inside of controllers fail by design.

!user.email_verified && !user.has_tokens && ' and ',
!user.has_tokens && 'create an API token',
'!',
].filter(e => !!e);
Copy link
Member

@Turbo87 Turbo87 Dec 29, 2019

Choose a reason for hiding this comment

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

the code logic here seems a bit hard to read. do you think it could be simplified in some way?

Copy link
Author

@Mordorreal Mordorreal Dec 31, 2019

Choose a reason for hiding this comment

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

I tried some variants before come to this. For example, a variant with ternary operators looks more ugly:

const verifyText = user.email_verified ? '' : 'verify your email address ';
const andText = user.email_verified && user.has_tokens ? '' : 'and ';
const tokenText = user.has_tokens ? '' : 'create an API token';

return `${verifyText}${andText}{$tokenText}`;

Moving this calculation logic to template is not right.
I'm open to any ideas on how to improve this code.

@bors
Copy link
Contributor

@bors bors commented Jan 6, 2020

The latest upstream changes (presumably #2096) made this pull request unmergeable. Please resolve the merge conflicts.

@Mordorreal Mordorreal requested a review from Turbo87 Jan 9, 2020
@Mordorreal
Copy link
Author

@Mordorreal Mordorreal commented Jan 13, 2020

@carols10cents Sorry for disturbing but can you please review the pr?

@Mordorreal
Copy link
Author

@Mordorreal Mordorreal commented Jan 28, 2020

@carols10cents, @Turbo87 Hi. Can you please review? I don't know why label is saying that pr waiting for the author because it's not true. PR is waiting for review.

@locks
Copy link
Contributor

@locks locks commented Jan 31, 2020

@Mordorreal you have multiple conflicts at the moment. Can you rebase and resolve them?

denis_savchuk@Deniss-MacBook-Pro.local (RSA) added 4 commits Feb 4, 2020
- Faker in model the best way to generate random values for better
testing.
- Remove check by classes because css module generate random class
name now and there no good ways how to use that names in tests.
- Refactor some asserts to check real value instead of custom one.
@Mordorreal
Copy link
Author

@Mordorreal Mordorreal commented Feb 5, 2020

@locks Ohhhh. Rebased. That was hard. Can you please review pr? Not sure that I will have willing to rebase this pr again after one month of silence.

@bors
Copy link
Contributor

@bors bors commented Mar 21, 2020

The latest upstream changes (presumably #2290) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

@bors bors commented Mar 24, 2020

The latest upstream changes (presumably #2308) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87 Turbo87 added C-enhancement and removed S-waiting-on-author labels Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants