Introduce constants for 'magic strings' #340
Comments
|
I've used https://github.com/T4MVC/T4MVC in the past for MVC projects. It works great for getting rid of magic strings of controllers and views. Not sure if it's been updated to support tag helpers |
|
@MisterJames and I had this conversation a couple of times. We discussed creating a static class named something like "EnumsAndStructs" or "WellKnownStrings", something along those lines. The idea is that some of these once-magic strings may be useful in other areas besides just views. |
|
Don't forget about the new |
|
Sorry for the spam... I didn't realize that referencing issues on my own fork would hit the issue on origin. Still learning the flow of git/hub :) I figured it wouldn't hit the issue log until my PR. |
|
Don't worry about it. We don't have established guidelines for when to reference issues in commits. Actually, I think the information is helpful because it shows an issue is being worked on even if a PR is not imminent. |
Eliminated a few magic strings in controllers #340
tonysurma
modified the milestones:
Technical Backlog,
Private Preview Release 1 UAT Issues
|
Hi all I'm looking for a jump-in issue. Is this still alive? (And if so, does anyone have a better method of finding all string literals than regular expression "Find In Files") Cheers, |
|
@ultrabert yes still alive, thx in advance |
|
I'd like to take a stab at this as well. To avoid duplication of effort, I'm letting everyone know I'm working on the ManageController and will be referencing the issue during commits, as per mheggesth's comment from 12/13/15. |
… constant for email subjects. HTBox#340
|
Some things to note, Not sure how thorough we want to be in converting these magic strings, but Controller names can't take advantage of the nameof operator as a consequence of MVC's convention for expecting the Controller's name without the "Controller". Others have addressed this with extension methods: http://stackoverflow.com/questions/27444121/how-to-use-c-sharp-nameof-with-asp-net-mvc-url-action. There's a lot of usage of ViewData throughout the controllers and views, any reason for not using viewmodels instead? If there is going to be extensive use of ViewData, JoelHulen's suggestion of implementing a static class with enums and consts would mitigate typos when keying into ViewData. |
|
Are you still working on this issue, @dangle1? |
|
@ultrabert I think he only did it for ManageController so you can jump in for another controller/area where the strings exist - just a note in this issue so others don't jump on the same set. Thanks! |
|
Thanks, @tonysurma. I may tackle magic strings together with unit tests for an area, but I'm hesitant to claim one, as my opportunities to contribute are somewhat sporadic, and I wouldn't want to lock anyone out of something they wanted to do. :) |
|
Hi team, apologies ahead if I've done something wrong - my first commit/push on git. The update is tiny, but I want to make sure you are ok with the approach before I go ahead with it. Please have a look and let me know what you think. Thanks for the patience. |
|
@nedruk Thanks for your first contribution. Small, focused updates are good. It makes reviewing and merging easier for the project owners. I've taken a quick look and it seems you've followed the correct approach based on the requirements for this. It might be worth making a formal PR with your code and then it can be reviewed there (might get a bit lost down the bottom of this issue). A couple of initial points from a quick look...
I hope that all makes sense. If you need any advise on submitting a PR let us know. |
|
Thanks for the detailed feedback, @stevejgordon. Re Constant class - my idea was to propose this as an approach - store constants in a single place instead of having them scattered around the solution. But i see your point, better to follow the general pattern. Re config change - that shouldn't have been committed. My bad. Thanks for advice about PR. There is still quite a learning curve ahead of me here on git.. |
|
@nedruk You're very welcome. I was new to it all about 7 months ago. There's a little learning curve but it starts to sync in pretty quickly. I wrote up a blog post about my early experience at http://stevejgordon.co.uk/contributing-to-allready which might help. @dpaquette also put up a really handy post on PR's http://www.davepaquette.com/archive/2016/01/24/Submitting-Your-First-Pull-request.aspx |
|
Looks like this one is up for grabs if other volunteers would like to pick it up |
|
@EmilyLuijbregts - I will add this to our code-a-thon project and perhaps someone can pick it up tomorrow. |
|
Hey sorry for my late response. I still have some changed files locally. I've been really struggling with Git(Hub). Perhaps I can send someone my files? ... I know it's not ideal, but then I'm not the bottleneck (and my changes are not lost) |
|
Hey Jowen. You can send it to @bmaluijb and he'll upload it tomorrow :-) |
|
FYI: I'm going to address a few of these and submit PRs as I finish logical chunks. |
changing strings to use nameof() #340 (partial)
change runtime UserType name lookups to compile-time nameof() lookup #340 (partial)
|
If there are still strings that need to be converted I'd love to use this as an opportunity to begin contributing. |
|
@joshuahysong, you'd need to search through the codebase for them. I know @johnmwright knocked out a lot of these (especially when it came to claims and helper/extensions for claims operations). If you want to float a couple ideas by us on this thread, to make sure we think they need to be changed before you start to contribute, that might be a good way to go here. Thanks |
|
@joshuahysong another way to contribute is to filter on the issues and pick the "jump-in" label...
|
|
Thanks, I'll take a look at the jump-in labeled issues. |
* Adds IAuthorizableTask and related classes * Add AuthorizableTask query,handler and fake * Implemented test: UpdateUserProfileCompletenessSendsRemoveUserProfileIncompleteClaimCommandWithCorrectUserIdWhenUsersProfileIsComplete() * Implemented Test: UpdateUserProfileCompletenessInvokesRefreshSignInAsyncWithCorrectUserWhenUsersProfileIsComplete() * Implemented test: ResendEmailConfirmationInvokesGenerateEmailConfirmationTokenAsyncWithCorrectUser() * Implemented tests: ResendEmailConfirmationInvokesUrlActionWithCorrectParameters() * Implemented test ResendEmailConfirmationSendsSendConfirmAccountEmailAsyncWithCorrectData() * Implemented tests: ResendEmailConfirmationRedirectsToCorrectAction() ResendEmailConfirmationHasHttpPostAttribute() ResendEmailConfirmationHasHttpValidateAntiForgeryTokenAttribute() EmailConfirmationSentReturnsAView() * test * Implemented tests: EnableTwoFactorAuthenticationInvokesSetTwoFactorEnabledAsyncWhenUserIsNotNull() EnableTwoFactorAuthenticationInvokesSignInAsyncWhenUserIsNotNull() RemoveLoginHasHttpPostAttribute() RemoveLoginHasValidateAntiForgeryTokenAttribute() * Implemented Test: EnableTwoFactorAuthenticationSendsUserByUserIdQueryWithCorrectUserId() * Implemented Tests: EnableTwoFactorAuthenticationRedirectsToCorrectAction() EnbaleTwoFactorAuthenticationHasHttpPostAttribute() EnableTwoFactorAuthenticationHasValidateAntiForgeryTokenAttribute() * Implemented all tests for DisableTwoFactorAuthentication() Controller Action * Implemented Tests for GetChangePassword Action. * Implemented tests: ChangePasswordPostSendsUserByUserIdQueryWithCorrectUserId() ChangePasswordPostInvokesChangePasswordAsyncWithCorrectParametersWhenUserIsNotNull() * Fix admin menu in integrations test * Fixes - Duplicate event button showing for event managers * Updates EventController to use AuthorizableEvent Fixes related EventController unit tests * Updates EventController create methods to use AuthorizableCampaign Adds AuthorizableCampaignQuery Adds FakeAuthorizableCampaign Exposes FakeAuthorizableEvent * Updates ItineraryController to use AuthorizableItinerary * Updates RequestController to use AuthorizableRequest * Updates TaskController to use AuthorizableTask * Fixes RequestController and TaskController creation error with new Authorization model * Fixes EventController - no need to check for authorization to be null * Fixes image deleted when image is updated for CampaignController * Fixes image deleted when image is updated for EventController Fixes wrong parameters sent to UploadEventImageAsync method * Updates CampaignController to use AuthorizableCampaign * Adds AuthorizableOrganization * Adds AuthorizableOrganization tests * Updates CampaignController to use AuthorizableOrganization * first commit at trying to improve the image/task attachment azure blob code - added BlockBlockServiceBase abstrct class. - it uses the template method to force the implemenation of the ContainerName property on classes that inherit from it. - ImageService and TaskAttachmentService inherit from it. - this class will allow the testing of the correct blob name being constructed before uploading it to blob storage. - this class also allows the user to set a whitelist and/or blacklist of extensions that will allow or disallow a particular file extension to be uploaded. Althought this is nice, we really should be whitelisting/blacklisting at the point we receive data from the UI using ViewModels and the ModelBinder. - ImageService and TaskAttachmentService now allow the testing of the correct blobPath being built before passing it to the inherited UploadAsync method - added BlockBlob class which implements IBlockBlob. IBlockBlob is injected into BlockBlobServiceBase via its implementors - the purpose of BlockBlox is to isolate the azure specific api. this class is only responsible for doing azure specific calls to authenticate, get the container, the blockBlob and either upload or delete the blob. * this commit removes the concept of the BlockBlobServiceBase as it was not really needed. - the ImageService and TaskAttachmentService have returned to owning their container names as well as taking care of any file validations (whitelist/blacklist) based on what they need to do - both classes now directly use IBlockBlob which encapsulates the azure blob storage calls - this should make both ImagesSerivce adn TaskAttachment service testable to make sure they're building the correct blob name before invoking IBlockBlob API calls and/or validating that files with the wrong extension are not uploaded. * code cleanup/renaming * Added a unit test for SendRequestStatusToGetASmokeAlarmHandler. All paths are handled but depending on feedback I could split up the tests verifying the various mapping combinations into separate tests. This would introduce some duplication though so I'm not sure that'd be the best approach * Some general cleanup. Sorted the using statements and removed "this." qualifiers as per the code style applied to the other tests I found. * Added a check for the providerRequestId as well. Renamed the method to clarify what we're asserting for. * Forgot to remove some more "this." goodness. * Added basic unit test to cover the SendInProgressStatusToGetASmokeAlarmHandler * - restore original value for nableAzureBlobImageService in config.json - delete unused SendGridDevelopmentWeb class file * Added some ignore rules for ncrunch project and solution extensions * Added a check to ensure the right method call is being enqueued. * Changed the GasaStatus "Enumeration" to expose constants instead of statics, allowing for their usage in the test cases. Replaced raw string values with the GasaStatus Enum, to ensure renames/refactorings do not inadvertedly break the test. * Moved !-circle to be before user name when logged in with incomplete profile to not cover caret * Updating UserAuthorizationService to expose IsEventManager, IsCampaignManager, IsTeamLead * Unit Tests * changing strings to use nameof() #340 (partial) action names, model property names (in error messages) and changing ResourceAdminController.Create from nameof() to a string b/c it was referencing the wrong class (a model and not the controller) * Updated to work with the latest changes on master branch * Fixed using statements in VolunteerTaskAttachmentService * change runtime UserType name lookups to compile-time nameof() lookup #340 (partial) * Renam FileImageService to FakeImageService * Issue 1603 - Unit Test for GetMyEventsQueryHandler * Remove events from locked or not published campaigns from start page * Changed locale on the datetime picker to match the culture set in startup.cs * adds view and action for compaign manager * adds query to retrieve the campaign that campaign manager authorized to manage * adds logic to action and change the view to show the campaign manager assigned campaign and direct user to campaign admin page * adds test for AuthorizedCampaignsQueryHandler * adds unit test for Campaign controller ManagerCampaign action * changes the campaign controller tests to reflect the new changes in campaign constructor * changes the implementation that show the manage button to the user on campaign index and adds test for it * changes manage campaign view model to work with the campaign.js ko model * changes the campaign controller test to reflect the new changes * delete the campaign js copy that causes the build to fail * changes the campaign controller test and removes the fake user manager class, also change the manage campaign view model to reflect the requested changes * Remove FakUserManager class fully from UserAuthorizationServiceTests - half-way finished with removal from ItineraryAdminControllerTests * Remove FakeUserManager and FakeUserManagerForBasicUser classes from UserAuthorizationServiceTests. - split MockHelper into two separate classes; SignInManagerMockHelper and UserManagerMockHelper and move the each method inMockHelper to the appropriately named class - the class names are more descriptive as to what they do and they should be easier to find. - all the other files affected by the MockHelper split * Change any MockHelper references left in CampaignControllerTests to UserManagerMockHelper * Fix two skipped CampaignAdminControllerTests tests marked as broken; (#2006) DeleteConfirmedSendsDeleteCampaignCommandWithCorrectCampaignId PublishConfirmedSendsPublishCampaignCommandWithCorrectCampaignId * Change name of TaskAttachmentService to AttachmentService (#1995) * Rename ITaskAttachmentService/TaskAttachmentService to IAttachmentService/AttachmentService - change method names to specify uploading an attachment for a given resource - aka, so UploadAsync becomes up UploadTaskAttachmentAsync and DeleteAsync becomes DeleteAttachmentAsync * code clean up * Renamed task views and view models (#1987) * Renamed "Task" to "VolunteerTask". Adjusted the namespaces as well, which cascaded to quite a few classes. Sorted out the namespace includes as well, based on the established convention. * Renamed Testing namespace and test class for ViewModels\Validators\VolunteerTask\* to match the rename from Task to VolunteerTask. * Task Attachment Renames #1939 (#1993) * Rename ITaskAttachmentService to IVolunteerAttachmentService * Rename parameter taskId to volunteerTaskId * Amend Azure path to 'volunteerTask/' * Make change to FileAttachment model * Implemented test: UpdateUserProfileCompletenessSendsRemoveUserProfileIncompleteClaimCommandWithCorrectUserIdWhenUsersProfileIsComplete() * Implemented Test: UpdateUserProfileCompletenessInvokesRefreshSignInAsyncWithCorrectUserWhenUsersProfileIsComplete() * Implemented test: ResendEmailConfirmationInvokesGenerateEmailConfirmationTokenAsyncWithCorrectUser() * Implemented tests: ResendEmailConfirmationInvokesUrlActionWithCorrectParameters() * Implemented test ResendEmailConfirmationSendsSendConfirmAccountEmailAsyncWithCorrectData() * Implemented tests: ResendEmailConfirmationRedirectsToCorrectAction() ResendEmailConfirmationHasHttpPostAttribute() ResendEmailConfirmationHasHttpValidateAntiForgeryTokenAttribute() EmailConfirmationSentReturnsAView() * test * Implemented tests: EnableTwoFactorAuthenticationInvokesSetTwoFactorEnabledAsyncWhenUserIsNotNull() EnableTwoFactorAuthenticationInvokesSignInAsyncWhenUserIsNotNull() RemoveLoginHasHttpPostAttribute() RemoveLoginHasValidateAntiForgeryTokenAttribute() * Implemented Test: EnableTwoFactorAuthenticationSendsUserByUserIdQueryWithCorrectUserId() * Implemented Tests: EnableTwoFactorAuthenticationRedirectsToCorrectAction() EnbaleTwoFactorAuthenticationHasHttpPostAttribute() EnableTwoFactorAuthenticationHasValidateAntiForgeryTokenAttribute() * Implemented all tests for DisableTwoFactorAuthentication() Controller Action * Implemented Tests for GetChangePassword Action. * Implemented tests: ChangePasswordPostSendsUserByUserIdQueryWithCorrectUserId() ChangePasswordPostInvokesChangePasswordAsyncWithCorrectParametersWhenUserIsNotNull() * Code clean up following cr
|
I'd like to pick up removing the Admin area magic strings (as part of the NDC Sydney event and learning the codebase), please let know of any concerns with that. |
|
Hello, I want to tackle the Controller names magic strings issue. I found 3 use cases:
Suggest adding a method for the general context use case and a couple of convenience methods for use from within a controller context. One for current controller and another for an other controller |
|
Here's a solution from stackoverflow |
|
Is this till open? I can try to fix it this weekend. |
@ajaychoudhary16 This project has been inactive for almost a year :). It looked like there was a possibility that it would be revived by the project leads but I haven't seen anything announced. Note: I've forked this repository because there were issues building the web app. Take a look at https://github.com/c0g1t8/allReady/tree/BrowserTests. You'll find a buildable and testable version there. |
and others
Throughout the code, we use a number of literal string constants.
It would be great to replace those globally with defined string constants. See comments on PR #332 for the start of the conversation.
The text was updated successfully, but these errors were encountered: