Add flash message with info after login #1952

Mordorreal · 2019-12-14T18:56:08Z

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 · 2019-12-14T18:56:12Z

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.

Turbo87 · 2019-12-16T08:50:56Z

app/routes/login.js

+      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' });


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

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.

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:

/api/private/session/authorize is how the frontend gets information about the current user from the backend immediately after logging in with GitHub. It calls the user::session::authorize controller function. After interacting with info from GitHub, it calls the user::me::me controller function (discussed more next)

/api/v1/me is how the frontend gets information about the current user from the backend if there's already an existing session for a logged in user. It also calls the user::me::me controller function, so this URL returns the same data as the session authorize URL.

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!

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

carols10cents

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

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 · 2019-12-25T13:10:40Z

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

Trying to avoid over fetching on current user model change.

Mordorreal · 2019-12-26T16:10:54Z

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

Mordorreal · 2019-12-26T16:40:06Z

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

bors · 2019-12-28T16:33:26Z

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

app/routes/me/index.js

Turbo87 · 2019-12-29T15:02:15Z

app/templates/catch-all.hbs

@@ -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>


why did you change the class name?

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

Turbo87 · 2019-12-29T15:05:16Z

app/services/flash-messages.js

    this.set('message', message);
+    this.set('options', options);
  },


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

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

Turbo87 · 2019-12-29T15:07:15Z

app/components/welcome-message.js

+    ].filter(e => !!e);
+
+    return textArray.join('');
+  }),


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

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.

Turbo87 · 2019-12-29T15:09:29Z

app/components/welcome-message.js

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


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

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 · 2020-01-06T23:39:56Z

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

Mordorreal · 2020-01-13T11:52:41Z

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

Mordorreal · 2020-01-28T22:26:37Z

@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 · 2020-01-31T09:54:19Z

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

- 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 · 2020-02-05T01:28:54Z

@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 · 2020-03-21T15:42:04Z

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

bors · 2020-03-24T17:47:35Z

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

Mordorreal · 2023-05-19T09:36:09Z

@Turbo87 Are this PR still needed? If no, please close. If yes, I have time for it now.

Turbo87 · 2024-01-29T09:53:41Z

closing due to #19 (comment) :)

Add flash message with info after login

6bd0143

- 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 assigned jtgeibel Dec 14, 2019

rust-highfive added the S-waiting-on-review label Dec 14, 2019

Fix prettier errors

93d8dbb

Turbo87 reviewed Dec 16, 2019

View reviewed changes

carols10cents requested changes Dec 18, 2019

View reviewed changes

carols10cents added S-waiting-on-author and removed S-waiting-on-review labels Dec 18, 2019

dsavchuk-vineti added 8 commits December 26, 2019 16:19

fix: Remove clean to fix error in console

93105e7

feat: Add has_token field to user

d7ac6d7

fix(component): Move test to right folder

e6403a9

chore: Add welcome-message component

d006ee9

fix: Remove flash message after user login

56ed394

fix: Update user fields after add/remove api tokens

220e90f

Trying to avoid over fetching on current user model change.

Merge upstream/master into feature/add-alert-with-info-after-login

ce44d31

fix: Fixes after merge from current master

9117b74

dsavchuk-vineti added 2 commits December 26, 2019 20:13

fix: Fix linter errors

9656756

fix: Fix Rust linter errors

b9c2856

dsavchuk-vineti added 2 commits December 28, 2019 22:11

Merge upstream/auto into feature/add-alert-with-info-after-login

6e0936f

fix: Change class name in test after rebase

4eb0e95

Mordorreal requested review from carols10cents and Turbo87 December 29, 2019 14:30

Turbo87 reviewed Dec 29, 2019

View reviewed changes

dsavchuk-vineti added 2 commits December 31, 2019 12:36

fix: Remove unneeded space

ff50263

fix: Change tests to use qunit-dom

46ab6ee

Mordorreal requested a review from Turbo87 January 9, 2020 14:29

denis_savchuk@Deniss-MacBook-Pro.local (RSA) added 4 commits February 4, 2020 13:54

Merge upstream/auto into feature/add-alert-with-info-after-login

ba7db55

fix: Return flash message component to default state

a64fc2a

fix: Update welcome message component to be align after rebase

b8c9d25

fix: Fix linter errors

d7b5a54

Merge branch 'master' into feature/add-alert-with-info-after-login

dbe7ed4

jtgeibel mentioned this pull request Apr 3, 2020

crates.io meeting agenda: 2020-04-03 14:30 UTC (Discord, 30 min) rust-lang/crates-io-cargo-teams#78

Closed

Turbo87 mentioned this pull request Dec 4, 2020

Add token_count to GET /api/v1/me response #3071

Closed

Turbo87 unassigned jtgeibel Sep 10, 2021

Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works and removed S-waiting-on-author labels Sep 10, 2021

Turbo87 closed this Jan 29, 2024

		@@ -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>

Add flash message with info after login #1952

Add flash message with info after login #1952

Uh oh!

Conversation

Mordorreal commented Dec 14, 2019

Uh oh!

rust-highfive commented Dec 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carols10cents left a comment

Choose a reason for hiding this comment

Uh oh!

bors commented Dec 25, 2019

Uh oh!

Mordorreal commented Dec 26, 2019

Uh oh!

Mordorreal commented Dec 26, 2019

Uh oh!

bors commented Dec 28, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jan 6, 2020

Uh oh!

Mordorreal commented Jan 13, 2020

Uh oh!

Mordorreal commented Jan 28, 2020

Uh oh!

locks commented Jan 31, 2020

Uh oh!

Mordorreal commented Feb 5, 2020

Uh oh!

bors commented Mar 21, 2020

Uh oh!

bors commented Mar 24, 2020

Uh oh!

Mordorreal commented May 19, 2023

Uh oh!

Turbo87 commented Jan 29, 2024

Uh oh!

Uh oh!