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

#1090: Notification to the consumer when the product is back in stock #1091

Merged
merged 1 commit into from Mar 10, 2024

Conversation

s04v
Copy link
Contributor

@s04v s04v commented Mar 9, 2024

#1090 Implementation

<div class="back-in-stock-subscribe">
<form action="api/stocks/back-in-stock">
<div>
@if (SignInManager.IsSignedIn(User))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@if (SignInManager.IsSignedIn(User))
@if (User.Identity.IsAuthenticated)

Comment on lines +36 to +40
public string GetCurrentHostName()
{
return _httpContext.Request.Host.Value;
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public string GetCurrentHostName()
{
return _httpContext.Request.Host.Value;
}
public string GetCurrentHostName() => _httpContext.Request.Host.Value;

Comment on lines +147 to +149
if (await _backInStockSubscriptionRepository.Query()
.Where(o => o.ProductId == productId && o.CustomerEmail == customerEmail)
.AnyAsync())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (await _backInStockSubscriptionRepository.Query()
.Where(o => o.ProductId == productId && o.CustomerEmail == customerEmail)
.AnyAsync())
if (await _backInStockSubscriptionRepository.Query()
.AnyAsync(o => o.ProductId == productId && o.CustomerEmail == customerEmail))


namespace SimplCommerce.Module.Inventory.Event
{
public class BackInStock : INotification
Copy link
Member

Choose a reason for hiding this comment

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

ProductBackInStock might more meaningful

Comment on lines +16 to +19
public BackInStockSendEmailHandler(IStockSubscriptionService stockSubscriptionService)
{
_stockSubscriptionService = stockSubscriptionService;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public BackInStockSendEmailHandler(IStockSubscriptionService stockSubscriptionService)
{
_stockSubscriptionService = stockSubscriptionService;
}
public BackInStockSendEmailHandler(IStockSubscriptionService stockSubscriptionService)
=> _stockSubscriptionService = stockSubscriptionService;

Comment on lines +21 to +24
public async Task Handle(BackInStock notification, CancellationToken cancellationToken)
{
await _stockSubscriptionService.BackInStockSendNotificationsAsync(notification.ProductId);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public async Task Handle(BackInStock notification, CancellationToken cancellationToken)
{
await _stockSubscriptionService.BackInStockSendNotificationsAsync(notification.ProductId);
}
public async Task Handle(BackInStock notification, CancellationToken cancellationToken)
=> await _stockSubscriptionService.BackInStockSendNotificationsAsync(notification.ProductId);

{
public class BackInStockSubscription : EntityBase
{
public long ProductId { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public long ProductId { get; set; }
public long ProductId { get; set; }

@@ -12,6 +16,9 @@ public class ModuleInitializer : IModuleInitializer
public void ConfigureServices(IServiceCollection serviceCollection)
{
serviceCollection.AddTransient<IStockService, StockService>();
serviceCollection.AddTransient<IStockSubscriptionService, StockSubscriptionService>();
serviceCollection.AddTransient<INotificationHandler<BackInStock>, BackInStockSendEmailHandler>();

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@@ -44,6 +48,8 @@ public async Task UpdateStock(StockUpdateRequest stockUpdateRequest)
var product = await _productRepository.Query().FirstOrDefaultAsync(x => x.Id == stockUpdateRequest.ProductId);
var stock = await _stockRepository.Query().FirstOrDefaultAsync(x => x.ProductId == stockUpdateRequest.ProductId && x.WarehouseId == stockUpdateRequest.WarehouseId);

var prevStockQuantity = product.StockQuantity;

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

};

_backInStockSubscriptionRepository.Add(subscription);
await _backInStockSubscriptionRepository.SaveChangesAsync();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await _backInStockSubscriptionRepository.SaveChangesAsync();
await _backInStockSubscriptionRepository.SaveChangesAsync();

@thiennn thiennn merged commit 4565b57 into simplcommerce:master Mar 10, 2024
8 checks passed
@hishamco
Copy link
Member

@thiennn I suggested some improvements to this PR also, there 're some non meaningful names, but seems you ignored all the my code suggestions

@s04v
Copy link
Contributor Author

s04v commented Mar 10, 2024

@hishamco I can make your improvements in the new PR

@hishamco
Copy link
Member

Believe it or not, it doesn't require a new PR, that's why the reviewing is helpful otherwise it's a waste of time if no one consider it

Don't get me wrong

@thiennn
Copy link
Contributor

thiennn commented Mar 11, 2024

Hi @hishamco , I am sorry. I was on a mobile and I didn't see your suggestion.

@s04v Yes, please help me make the new PR, Thanks

@hishamco
Copy link
Member

No problem it happens :)

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.

None yet

3 participants